Bug 223911 – Ensure that GlobalPropertyInfo is allocated on the stack.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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

[Mark Lam]

Created attachment 424615 [details]
proposed patch.

[Yusuke Suzuki]

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.

[Mark Lam]

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.

[Mark Lam]

Landed in r275212: <http://trac.webkit.org/r275212>.

[Alexey Proskuryakov]

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.

[Mark Lam]

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.

[Ryan Haddad]

Rebaselined builtins generator tests in https://trac.webkit.org/changeset/275233/webkit

[Alexey Proskuryakov]

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?

[Mark Lam]

(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.