Bug 236775 - [JSC] Substring resolving should check 8bit / 16bit again
Summary: [JSC] Substring resolving should check 8bit / 16bit again
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-17 05:44 PST by Lukas Bernhard
Modified: 2022-05-26 14:41 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.19 KB, patch)
2022-04-06 03:59 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.37 KB, patch)
2022-04-06 04:04 PDT, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lukas Bernhard 2022-02-17 05:44:57 PST
The attached sample triggers an assertion in webkit on git commit d96b38bfed8b

Build command: ./Tools/Scripts/build-jsc --jsc-only --debug --cmakeargs="-DENABLE_STATIC_JSC=ON -DCMAKE_C_COMPILER='/usr/bin/clang-12' -DCMAKE_CXX_COMPILER='/usr/bin/clang++-12' -DCMAKE_CXX_FLAGS='-O3 -lrt -latomic -fuse-ld=lld'"
Run command: build/Debug/bin/jsc --validateOptions=true --useConcurrentJIT=false --useConcurrentGC=false --thresholdForJITSoon=10 --thresholdForJITAfterWarmUp=10 --thresholdForOptimizeAfterWarmUp=100 --thresholdForOptimizeAfterLongWarmUp=100 --thresholdForOptimizeAfterLongWarmUp=100 --thresholdForFTLOptimizeAfterWarmUp=1000 --thresholdForFTLOptimizeSoon=100

// STDERR:
// ASSERTION FAILED: !is8Bit()
// WTF/Headers/wtf/text/StringImpl.h(297) : const UChar *WTF::StringImpl::characters16() const

sample.js:
```
function main() {
    for (let v27 = 0; v27 < 100; v27++) {
        const v44 = [0,0,1.1];
        const v61 = v44.toLocaleString();
        const v62 = eval(Math);
        v63 = v61.substring(v62,v27);

        function v64() {
            if (v62) {
                Math[v61] = [];
            }
            const v82 = (-1.0).__proto__;
            delete v82[v63];
        }
        v64();
    }
}
main();
```

Full backtrace:
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=140737314615232) at pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=140737314615232) at pthread_kill.c:80
#2  __GI___pthread_kill (threadid=140737314615232, signo=signo@entry=6) at pthread_kill.c:91
#3  0x00007ffff5a96476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ffff5a7c7b7 in __GI_abort () at abort.c:79
#5  0x0000000000cd501a in WTFCrashWithInfo () at WTF/Headers/wtf/Assertions.h:741
#6  0x0000000001e6d1b9 in WTF::StringImpl::characters16 (this=<optimized out>) at WTF/Headers/wtf/text/StringImpl.h:297
#7  WTF::String::characters16 (this=<optimized out>) at WTF/Headers/wtf/text/WTFString.h:129
#8  JSC::JSRopeString::resolveRopeInternal16 (this=this@entry=0x7fffaa4f0020, buffer=buffer@entry=0x7fffffffc140 u"쓼\xffff翿")
    at ../../Source/JavaScriptCore/runtime/JSString.cpp:169
#9  0x0000000001e6d42a in JSC::JSRopeString::resolveRopeToAtomString (this=0x7fffaa4f0020, globalObject=<optimized out>)
    at ../../Source/JavaScriptCore/runtime/JSString.cpp:217
#10 0x0000000000cd6a84 in JSC::JSRopeString::toIdentifier (this=0x6, this@entry=0x7fffaa4f0020,
    globalObject=globalObject@entry=0x7fffaa460a68) at ../../Source/JavaScriptCore/runtime/JSString.h:771
#11 0x0000000000cd654c in JSC::JSString::toIdentifier (this=0x7fffaa4f0020, globalObject=globalObject@entry=0x7fffaa460a68)
    at ../../Source/JavaScriptCore/runtime/JSString.h:794
#12 0x0000000000cd5498 in JSC::JSValue::toPropertyKey (this=<optimized out>, globalObject=0x7fffaa460a68)
    at ../../Source/JavaScriptCore/runtime/JSCJSValueInlines.h:808
#13 0x0000000001a72664 in JSC::deleteByVal (globalObject=globalObject@entry=0x7fffaa460a68, vm=..., slot=..., base=..., key=...,
    ecmaMode=ecmaMode@entry=...) at ../../Source/JavaScriptCore/jit/JITOperations.cpp:2614
#14 0x0000000001a71fd8 in JSC::operationDeleteByValOptimize (globalObject=0x7fffaa460a68, stubInfo=0x7fffec059ca8,
    encodedBase=140737152821896, encodedSubscript=140736050692128, ecmaMode=...) at ../../Source/JavaScriptCore/jit/JITOperations.cpp:2636
Comment 1 Radar WebKit Bug Importer 2022-02-21 13:23:53 PST
<rdar://problem/89253391>
Comment 2 Yusuke Suzuki 2022-04-06 03:59:46 PDT
Created attachment 456800 [details]
Patch
Comment 3 Yusuke Suzuki 2022-04-06 04:04:19 PDT
Created attachment 456802 [details]
Patch
Comment 4 Saam Barati 2022-04-06 09:26:03 PDT
Comment on attachment 456802 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        Substring JSString is wrapping JSString. Thus it is possible that underlying JSString's 8Bit / 16Bit status
> +        becomes different from substring JSString wrapper's bit. We should not assume they are the same.

Why does the underlying string change here?
Comment 5 Yusuke Suzuki 2022-04-06 11:44:16 PDT
Comment on attachment 456802 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:10
>> +        becomes different from substring JSString wrapper's bit. We should not assume they are the same.
> 
> Why does the underlying string change here?

One possible case is that, underlying string was 16bit rope, and after resolving that, it was converted into the existing 8bit atom string.
Comment 6 Yusuke Suzuki 2022-04-06 11:48:58 PDT
Committed r292484 (249335@trunk): <https://commits.webkit.org/249335@trunk>
Comment 7 Brent Fulgham 2022-05-26 14:41:36 PDT
This fix shipped with Safari 15.5 (all platforms).