[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 338584] Patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien 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 338584: Patch

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




--- Comment #28 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 338584
  --> https://bugs.webkit.org/attachment.cgi?id=338584
Patch

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

If you want to fix https://bugs.webkit.org/show_bug.cgi?id=184744 you should
also add the test from it. There's one comment that I think you need to address
though, otherwise the fix is incorrect.

>
Source/JavaScriptCore/Scripts/tests/builtins/expected/JavaScriptCore-Builtin.pr
ototype-Combined.js-result:290
> +	   return expectedUnlinked.value()->link(vm,
vm.builtinExecutables()->codeName##Source(), std::nullopt,
s_##codeName##Intrinsic);\

FWIW you  don't need ".value()" here: expected overload operator-> and forwards
to the value type already. It's UB to use operator-> if it contains an error,
but you've already checked above.

I'm not saying you should change it, it's just the style I prefer but either
approach is fine with me.

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:133
> +	   if (!f.has_value())\

Yo can do the same pattern here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3329
> +	       emitThrowRangeError("Stack overflow in the parser");

I don't think this is correct because you'll emit an exception. It'll correctly
throw when we first try to execute, but then it goes into the code cache and
will always throw, even if the stack wouldn't overflow anymore.