[webkit-reviews] review denied: [Bug 184074] A stack overflow in the parsing of a builtin (called by createExecutable) cause a crash instead of a catchable js exception : [Attachment 336771] Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has denied Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 184074: A stack overflow in the parsing of a builtin (called by
createExecutable) cause a crash instead of a catchable js exception
https://bugs.webkit.org/show_bug.cgi?id=184074

Attachment 336771: Patch

https://bugs.webkit.org/attachment.cgi?id=336771&action=review




--- Comment #7 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 336771
  --> https://bugs.webkit.org/attachment.cgi?id=336771
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336771&action=review

r- because I think we should limit where the scope where it's "acceptable" to
have a null ExecState since that's not true anywhere else.

> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:105
> +    return Expected<JSC::FunctionExecutable*,
JSC::ParserError>(expectedUnlinked.value()->link(vm,
vm.builtinExecutables()->codeName##Source(), std::nullopt,
s_##codeName##Intrinsic));\\

Nit: I don't think you need the Expected constructor explicitly here.

> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:117
> +    return Expected<JSC::FunctionExecutable*,
JSC::ParserError>(clientData->builtinFunctions().${objectNameLC}Builtins().code
Name##Executable().value()->link(vm,
clientData->builtinFunctions().${objectNameLC}Builtins().codeName##Source(),
std::nullopt, s_##codeName##Intrinsic)); \\

ditto.

> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:129
> +    return Expected<JSC::FunctionExecutable*,
JSC::ParserError>(clientData->builtinFunctions().${objectNameLC}Builtins().code
Name##Executable().value()->link(vm,
clientData->builtinFunctions().${objectNameLC}Builtins().codeName##Source(),
std::nullopt, s_##codeName##Intrinsic)); \\

ditto.

> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:179
> +    return Expected<JSC::UnlinkedFunctionExecutable*,
JSC::ParserError>(m_##name##Executable.get());\\

ditto.

> Source/JavaScriptCore/builtins/BuiltinExecutables.h:41
> +typedef Expected<UnlinkedFunctionExecutable*, ParserError>
ExpectedUnlinkedFunctionExecutable;

Nit: We try to use "using" these days.

> Source/JavaScriptCore/parser/ParserError.cpp:60
> +    VM& vm = exec->vm();

I think this will just crash if exec is nullptr. That sees not intended?

> Source/JavaScriptCore/runtime/Lookup.h:324
> +inline void reifyStaticProperty(VM& vm, const ClassInfo* classInfo, const
PropertyName& propertyName, const HashTableValue& value, JSObject& thisObj,
ExecState* exec = nullptr)

Nit: We don't usually set up our methods this way. I would move the ExecState
to just after vm and change the call site below to pass nullptr.

I would also change the name to execForThrowing or something.

> Source/JavaScriptCore/runtime/Lookup.h:326
> +    ASSERT(&vm == &(exec->vm()));

Won't this crash if exec is nullptr?

> Source/JavaScriptCore/runtime/Lookup.h:335
> +	       if (f.has_value())
> +		   thisObj.putDirectBuiltinFunction(vm, thisObj.globalObject(),
propertyName, f.value(), attributesForStructure(value.attributes()));
> +	       else
> +		   f.error().throwStackOverflowOrOutOfMemory(exec);

It seems like you should handle nullptr exec here rather than in the
throwStackOverflowOrOutOfMemory function.