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

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted 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 338407: Patch

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




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

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

A few comments, otherwise r=me

> Source/JavaScriptCore/ChangeLog:14
> +	   There are now bare calls to '.value()' on several paths that may
crash. It is not a problem in my opinion, since we previously crashed in every
case regardless of the path that took us to createExecutable when encountering
a stack overflow.

Can you clarify here that calling foo.value() is critical compared to using
*foo because the later is UB if the expected isn't "valued". That distinction
has surprised folks before for e.g. std::optional.

>
Source/JavaScriptCore/Scripts/builtins/builtins_generate_combined_header.py:70
> +#include <wtf/Expected.h>

🎉

> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:89
> +    if (!expectedUnlinked.has_value())\\

The patter I prefer is:

if (auto expectedUnlinked = vm.foo.far())
  return expectedUnlinked->link(...);
else
  return makeUnexpected(expectedUnlinked.error()); // or if the E types are
compatible, just return expectedUnlinked;

I'd do the here and other places in this patch.

> Source/JavaScriptCore/parser/ParserError.cpp:35
> +JSObject* ParserError::toErrorObject(JSGlobalObject* globalObject, const
SourceCode& source, int overrideLineNumber)

Add a space between namespace and function.

> Source/JavaScriptCore/parser/ParserError.cpp:43
> +	   auto line = overrideLineNumber == -1 ? m_line : overrideLineNumber;

I'd make overrideLineNumber a std::optional<int> instead, and default to
nullopt in the header.

> Source/JavaScriptCore/parser/ParserError.cpp:92
> +	   return;

My preference is to have a toString method for the enum, and then have print
call that:

const char* toString(ErrorType e)
{
  switch (e) {
  case None: return "none";
  // ...
  }
}

void print(out, e) { return out.print(toString(e)); }

> Source/JavaScriptCore/parser/ParserError.h:85
> +    JS_EXPORT_PRIVATE JSObject* throwStackOverflowOrOutOfMemory(ExecState* =
nullptr);

Why default to nullptr? It'll insta-crash if that's called.

> Source/JavaScriptCore/runtime/Lookup.h:416
> +	   reifyStaticProperty(vm, nullptr, classInfo, key, value, thisObj);

Looks like reifyStaticProperties is called in CodeGeneratorJS.pm, which has
access to exec through globalObject()->globalExec().
Otherwise there's bunch of tests in Source/WebCore/bindings/scripts/test/JS/
which we can probably assume don't throw.
You can probably pipe exec in this function too. Maybe in this patch or in a
follow-up?

> JSTests/stress/stack-overflow-while-parsing-builtin.js:1
> +function f() {

You can add the following to control stack size and redone size and make the
repro faster / trigger it:

//@ runDefault("--maxPerThreadStackUsage=1000000", "--reservedZoneSize=0")