Bug 214837

Summary: ASSERTION FAILED: isSymbol() in Source/JavaScriptCore/runtime/JSCell.cpp(188)
Product: WebKit Reporter: wmrabb
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Critical CC: bfulgham, darin, ews-watchlist, keith_miller, mark.lam, msaboff, product-security, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Linux   
Attachments:
Description Flags
The JS file that causes a crash in the built JSC binary
none
proposed patch. none

Description wmrabb 2020-07-27 11:07:03 PDT
Created attachment 405296 [details]
The JS file that causes a crash in the built JSC binary

I've attached the JS file that causes a crash on an asan-enabled JavaScriptCore binary. Here is how I built the binary (note: the commit hash for the version of WebKit I used is 3215a5ce92987ec8184b9133114e9f8213ae43ab):
=====================
git clone git://git.webkit.org/WebKit.git
cd WebKit
Tools/Scripts/set-webkit-configuration --asan --debug
Tools/Scripts/build-webkit --debug --jsc-only
=====================

Here is the output of JSC: 
=====================
ASSERTION FAILED: isSymbol()
../../Source/JavaScriptCore/runtime/JSCell.cpp(188) : JSC::JSObject* JSC::JSCell::toObjectSlow(JSC::JSGlobalObject*) const
Aborted
=====================

This is the function where it crashes (the arrow denotes the exact line):
=====================
JSObject* JSCell::toObjectSlow(JSGlobalObject* globalObject) const
{
    Integrity::auditStructureID(globalObject->vm(), structureID());
    ASSERT(!isObject());
    if (isString())
        return static_cast<const JSString*>(this)->toObject(globalObject);
    if (isHeapBigInt())
        return static_cast<const JSBigInt*>(this)->toObject(globalObject);
    =====> ASSERT(isSymbol());
    return static_cast<const Symbol*>(this)->toObject(globalObject);
}
=====================
Comment 1 Radar WebKit Bug Importer 2020-07-27 11:07:19 PDT
<rdar://problem/66171782>
Comment 2 Mark Lam 2020-07-27 19:04:19 PDT
This is not a real bug.  The test was iterating over special properties in the jsc shell that are only for testing use.  This scenario cannot manifest in any productized version of JSC because these special test properties will not be present.

I'll put together a patch to make all the special test properties non-enumerable so that they will stop tripping up people who doing blind enumeration on the jsc shell's test global object.
Comment 3 Mark Lam 2020-07-27 20:44:10 PDT
Thanks for this report.  I will upload a patch to mitigate this issue shortly.
Comment 4 Mark Lam 2020-07-27 20:44:52 PDT
Created attachment 405339 [details]
proposed patch.
Comment 5 Darin Adler 2020-07-28 09:13:03 PDT
Comment on attachment 405339 [details]
proposed patch.

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

> Source/JavaScriptCore/jsc.cpp:477
> +    static constexpr unsigned DontEnum = 0 | PropertyAttribute::DontEnum;

What is the purpose of the "0 |" here? Is it a way to convert an enumeration value to an integer? Why is it better than static_cast?
Comment 6 Mark Lam 2020-07-28 09:21:08 PDT
Comment on attachment 405339 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/jsc.cpp:477
>> +    static constexpr unsigned DontEnum = 0 | PropertyAttribute::DontEnum;
> 
> What is the purpose of the "0 |" here? Is it a way to convert an enumeration value to an integer? Why is it better than static_cast?

Yes, it's for converting the enumeration into an integer.  I chose it because it's less verbose than a static_cast<unsigned> cast.  It's better in my eyes because it concisely expresses the idea, but "better" here is subjective.
Comment 7 Darin Adler 2020-07-28 09:29:37 PDT
Comment on attachment 405339 [details]
proposed patch.

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

>>> Source/JavaScriptCore/jsc.cpp:477
>>> +    static constexpr unsigned DontEnum = 0 | PropertyAttribute::DontEnum;
>> 
>> What is the purpose of the "0 |" here? Is it a way to convert an enumeration value to an integer? Why is it better than static_cast?
> 
> Yes, it's for converting the enumeration into an integer.  I chose it because it's less verbose than a static_cast<unsigned> cast.  It's better in my eyes because it concisely expresses the idea, but "better" here is subjective.

It’s oblique. Prettier and more elegant if you understand it, but no connotation of "converting type" unless you happen to know the idiom.
Comment 8 Mark Lam 2020-07-28 09:35:42 PDT
Comment on attachment 405339 [details]
proposed patch.

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

>>>> Source/JavaScriptCore/jsc.cpp:477
>>>> +    static constexpr unsigned DontEnum = 0 | PropertyAttribute::DontEnum;
>>> 
>>> What is the purpose of the "0 |" here? Is it a way to convert an enumeration value to an integer? Why is it better than static_cast?
>> 
>> Yes, it's for converting the enumeration into an integer.  I chose it because it's less verbose than a static_cast<unsigned> cast.  It's better in my eyes because it concisely expresses the idea, but "better" here is subjective.
> 
> It’s oblique. Prettier and more elegant if you understand it, but no connotation of "converting type" unless you happen to know the idiom.

There are places where we set more than one attribute value using enums (e.g. PropertyAttribute::DontEnum | PropertyAttribute::DontDelete).  In those places, we also don't do the static_cast, and arguably, one would also have to understand the same idiom to understand why it works.  I think we should adopt this idiom.  Also, it's a very common idiom for "converting to integer" in JS code.  Given that this is JSC, the idiom should not be that foreign.
Comment 9 EWS 2020-07-28 09:38:40 PDT
Committed r264991: <https://trac.webkit.org/changeset/264991>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405339 [details].