Bug 232966 - GetMyArgumentByValOutOfBounds needs to check for negative indices
Summary: GetMyArgumentByValOutOfBounds needs to check for negative indices
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: PC Linux
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-10 13:56 PST by Lukas Bernhard
Modified: 2021-11-30 12:00 PST (History)
9 users (show)

See Also:


Attachments
Patch (6.46 KB, patch)
2021-11-29 17:36 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (6.44 KB, patch)
2021-11-29 17:42 PST, Saam Barati
ysuzuki: review+
Details | Formatted Diff | Diff
patch for landing (6.71 KB, patch)
2021-11-29 17:51 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lukas Bernhard 2021-11-10 13:56:56 PST
During differential testing of webkit I found a sample triggering a miscomputation in FTL.

JSC on git commit: 93d2e7bf275b
Build options: ./Tools/Scripts/build-jsc --jsc-only --release --cmakeargs="-ENABLE_STATIC_JSC=ON -DCMAKE_C_COMPILER='/usr/bin/clang-12' -DCMAKE_CXX_COMPILER='/usr/bin/clang++-12' -DCMAKE_CXX_FLAGS='-fsanitize-coverage=trace-pc-guard -O3 -lrt -fuse-ld=lld'"

WebKitBuild/Debug/bin/jsc --validateOptions=true --useConcurrentJIT=false --useConcurrentGC=false --thresholdForJITSoon=10 --thresholdForJITAfterWarmUp=10 --thresholdForOptimizeAfterWarmUp=100 --thresholdForOptimizeAfterLongWarmUp=100 --thresholdForOptimizeSoon=100 --thresholdForFTLOptimizeAfterWarmUp=1000 --thresholdForFTLOptimizeSoon=1000 --validateBCE=true --useFTLJIT=true sample.js

```
function main() {
    let v26;

    const v13 = [0, 0]; 
    v16 = [0];
    const v18 = v16.__proto__;

    const v23 = (v24,...v25) => {
        v26 = v25[-80887344];
    };  
    v18[-80887344] = v13;
    for (let v30 = -256; v30 < 100; v30++) {
        const v31 = v23(0);
    }   
    print(v26);  // 0,0 without FTL, undefined without FTL
}
noDFG(main);
main();
```
Comment 1 Lukas Bernhard 2021-11-10 23:53:42 PST
The comment in the sample is incorrect, instead of
```
// 0,0 without FTL, undefined without FTL
```

it should be:
```
// 0,0 without FTL, undefined with FTL. (also 0,0 in spidermonkey)
```
Comment 2 Radar WebKit Bug Importer 2021-11-17 13:57:21 PST
<rdar://problem/85519898>
Comment 3 Saam Barati 2021-11-29 17:36:51 PST
Created attachment 445366 [details]
Patch
Comment 4 Saam Barati 2021-11-29 17:42:27 PST
Created attachment 445367 [details]
patch
Comment 5 Yusuke Suzuki 2021-11-29 17:48:38 PST
Comment on attachment 445367 [details]
patch

r=me
Comment 6 Saam Barati 2021-11-29 17:50:12 PST
Comment on attachment 445367 [details]
patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5668
> +        LValue isOutOfBounds = m_out.aboveOrEqual(indexToCheck, numberOfArgs);

I'll clean up the scope of the numberOfArgumentsToSkip since we no longer use it here.
Comment 7 Saam Barati 2021-11-29 17:51:30 PST
Created attachment 445370 [details]
patch for landing
Comment 8 EWS 2021-11-30 12:00:53 PST
Committed r286312 (244671@main): <https://commits.webkit.org/244671@main>

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