Bug 223911

Summary: Ensure that GlobalPropertyInfo is allocated on the stack.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, calvaris, ddkilzer, ews-watchlist, fpizlo, keith_miller, msaboff, rmorisset, sbarati, tzagallo, webkit-bug-importer, youennf, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch. ysuzuki: review+

Description Mark Lam 2021-03-29 21:18:56 PDT
We rely on GlobalPropertyInfo being allocated on the stack to allow its JSValue value to be scanned by the GC.  Unfortunately, an ASAN compilation would choose to allocate the GlobalPropertyInfo on a side buffer instead of directly on the stack.  This prevents the GC doing the needed scan.

We'll fix this by suppressing ASAN on the functions that allocated GlobalPropertyInfo arrays.

rdar://75865742
Comment 1 Mark Lam 2021-03-29 21:27:10 PDT
Created attachment 424615 [details]
proposed patch.
Comment 2 Yusuke Suzuki 2021-03-30 01:35:29 PDT
Comment on attachment 424615 [details]
proposed patch.

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

> Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_implementation.py:146
> +        lines = ["SUPPRESS_ASAN void JSBuiltinInternalFunctions::initialize(JSDOMGlobalObject& globalObject)",

How about putting `inline` here since this function is only called from this file.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:583
> +SUPPRESS_ASAN void JSGlobalObject::initStaticGlobals(VM& vm)

How about putting `inline` here since this function is only called from this file.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:2116
> +SUPPRESS_ASAN void JSGlobalObject::exposeDollarVM(VM& vm)

How about putting `inline` here since this function is only called from this file.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:176
> +SUPPRESS_ASAN void JSDOMGlobalObject::addBuiltinGlobals(VM& vm)

How about putting `inline` here since this function is only called from this file.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:114
> +SUPPRESS_ASAN void JSDOMWindowBase::initStaticGlobals(JSC::VM& vm)

How about putting `inline` here since this function is only called from this file.
Comment 3 Mark Lam 2021-03-30 10:17:48 PDT
Comment on attachment 424615 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_implementation.py:146
>> +        lines = ["SUPPRESS_ASAN void JSBuiltinInternalFunctions::initialize(JSDOMGlobalObject& globalObject)",
> 
> How about putting `inline` here since this function is only called from this file.

This one is not an inline function.  It's generated into WebCoreJSBuiltinsInternals.cpp and called from another module.  Hence, this one should not have an inline attribute.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:583
>> +SUPPRESS_ASAN void JSGlobalObject::initStaticGlobals(VM& vm)
> 
> How about putting `inline` here since this function is only called from this file.

Added inline here.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:2116
>> +SUPPRESS_ASAN void JSGlobalObject::exposeDollarVM(VM& vm)
> 
> How about putting `inline` here since this function is only called from this file.

This one is a sort of "slow path" kind of function (i.e. no motivation to inline), and it also needs to be exported so that it can be called from a test.  So, it doesn't make sense to inline here.

>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:176
>> +SUPPRESS_ASAN void JSDOMGlobalObject::addBuiltinGlobals(VM& vm)
> 
> How about putting `inline` here since this function is only called from this file.

This is not a new function that I introduced.  It is also called from 2 places in this file.  Since this function is large, it may not make sense to inline it.  I'll leave it as is.

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:114
>> +SUPPRESS_ASAN void JSDOMWindowBase::initStaticGlobals(JSC::VM& vm)
> 
> How about putting `inline` here since this function is only called from this file.

Added inline here.
Comment 4 Mark Lam 2021-03-30 10:21:53 PDT
Landed in r275212: <http://trac.webkit.org/r275212>.
Comment 5 Alexey Proskuryakov 2021-03-30 12:03:58 PDT
Comment on attachment 424615 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:12
> +        We rely on GlobalPropertyInfo being allocated on the stack to allow its JSValue
> +        value to be scanned by the GC.  Unfortunately, an ASAN compilation would choose
> +        to allocate the GlobalPropertyInfo on a side buffer instead of directly on the
> +        stack.  This prevents the GC from doing the needed scan.

This sounds like an ASan bug that should be fixed, is there a good reason for this behavior?

Surprised that we aren't seeing problems caused by this all the time on ASan bots.
Comment 6 Mark Lam 2021-03-30 12:22:19 PDT
Comment on attachment 424615 [details]
proposed patch.

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

>> Source/JavaScriptCore/ChangeLog:12
>> +        stack.  This prevents the GC from doing the needed scan.
> 
> This sounds like an ASan bug that should be fixed, is there a good reason for this behavior?
> 
> Surprised that we aren't seeing problems caused by this all the time on ASan bots.

My guess is that we're declaring an array of GlobalPropertyInfo, and ASAN wanted to add stuff to do bounds checking on that array.
Comment 7 Ryan Haddad 2021-03-30 14:00:07 PDT
Rebaselined builtins generator tests in https://trac.webkit.org/changeset/275233/webkit
Comment 8 Alexey Proskuryakov 2021-03-30 14:08:12 PDT
ASan works with stack arrays, and even with alloca (stack-buffer-overflow check).

Do you have a theory for what we haven't been hitting issues caused by this all the time?
Comment 9 Mark Lam 2021-03-30 14:35:28 PDT
(In reply to Alexey Proskuryakov from comment #8)
> ASan works with stack arrays, and even with alloca (stack-buffer-overflow
> check).

OK.

> Do you have a theory for what we haven't been hitting issues caused by this
> all the time?

The issue was manifesting as a GC bug where an object that should have been alive was collected.  If the relevant object in the global was never referenced / used after it was collected, then we would never have known about this.

Also, it relies on GC running at just the right time (courtesy of --collectContinuously in this case).  If GC missed the window, then this would also not manifest.

That's the best I can think of.