Bug 196631 - [JSC] makeBoundFunction should not assume incoming "length" value is Int32 because it performs some calculation in bytecode
Summary: [JSC] makeBoundFunction should not assume incoming "length" value is Int32 be...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-04 14:59 PDT by Yusuke Suzuki
Modified: 2019-04-04 21:18 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.40 KB, patch)
2019-04-04 17:31 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.38 KB, patch)
2019-04-04 18:14 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2019-04-04 18:19 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (17.93 KB, patch)
2019-04-04 19:10 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-04-04 14:59:14 PDT
...
Comment 1 Yusuke Suzuki 2019-04-04 17:31:15 PDT
Created attachment 366780 [details]
Patch
Comment 2 Yusuke Suzuki 2019-04-04 17:48:37 PDT
Double prediction injection found this issue.
Comment 3 Yusuke Suzuki 2019-04-04 18:14:48 PDT
Created attachment 366783 [details]
Patch
Comment 4 Build Bot 2019-04-04 18:16:48 PDT
Attachment 366783 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Yusuke Suzuki 2019-04-04 18:19:25 PDT
Created attachment 366784 [details]
Patch
Comment 6 Yusuke Suzuki 2019-04-04 18:21:23 PDT
Comment on attachment 366784 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:257
> +    RELEASE_AND_RETURN(scope, JSValue::encode(JSBoundFunction::create(vm, exec, globalObject, target, boundThis, boundArgs.isCell() ? jsCast<JSArray*>(boundArgs) : nullptr, length, name)));

I'll change `name` part to `WTFMove(name)`
Comment 7 Build Bot 2019-04-04 18:22:04 PDT
Attachment 366784 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:15:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Saam Barati 2019-04-04 18:32:47 PDT
Comment on attachment 366784 [details]
Patch

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

> Source/JavaScriptCore/runtime/RandomizingFuzzerAgent.cpp:47
> +    if (Options::injectDoublePredictionsForNumbers()) {
> +        if (original && mergeSpeculations(original, SpecBytecodeNumber) == SpecBytecodeNumber)
> +            return SpecBytecodeDouble;
> +        return original;
> +    }

Can we make this a different fuzzer agent? It's no longer random.
Comment 9 Saam Barati 2019-04-04 18:33:36 PDT
Comment on attachment 366784 [details]
Patch

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

> Source/JavaScriptCore/runtime/Options.h:439
> +    v(bool, injectDoublePredictionsForNumbers, false, Normal, nullptr) \

Then this can be called "useDoublePredictionFuzzerAgent" or something similar.
Comment 10 Yusuke Suzuki 2019-04-04 18:38:00 PDT
Comment on attachment 366784 [details]
Patch

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

>> Source/JavaScriptCore/runtime/RandomizingFuzzerAgent.cpp:47
>> +    }
> 
> Can we make this a different fuzzer agent? It's no longer random.

Yeah, I'll change to do that.
Comment 11 Yusuke Suzuki 2019-04-04 19:10:42 PDT
Created attachment 366788 [details]
Patch
Comment 12 Build Bot 2019-04-04 19:13:35 PDT
Attachment 366788 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:15:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, fuzzer, fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Saam Barati 2019-04-04 19:18:03 PDT
Comment on attachment 366788 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:251
> +    int32_t length = lengthValue.toInt32(exec);

Should assert it's a number. Maybe even isAnyInt and that it's within 32-bits?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:252
> +    RETURN_IF_EXCEPTION(scope, { });

Should be an assert.

> JSTests/stress/make-bound-function-should-not-assume-int32-length.js:1
> +//@ runDefault("--useDoublePredictionFuzzerAgent=1")

maybe also concurrentJIT=0 to ensure we JIT?
Comment 14 Yusuke Suzuki 2019-04-04 20:08:18 PDT
Comment on attachment 366788 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:251
>> +    int32_t length = lengthValue.toInt32(exec);
> 
> Should assert it's a number. Maybe even isAnyInt and that it's within 32-bits?

Changed.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:252
>> +    RETURN_IF_EXCEPTION(scope, { });
> 
> Should be an assert.

Fixed.

>> JSTests/stress/make-bound-function-should-not-assume-int32-length.js:1
>> +//@ runDefault("--useDoublePredictionFuzzerAgent=1")
> 
> maybe also concurrentJIT=0 to ensure we JIT?

Sounds fine. Fixed.
Comment 15 Yusuke Suzuki 2019-04-04 21:17:51 PDT
Committed r243925: <https://trac.webkit.org/changeset/243925>
Comment 16 Radar WebKit Bug Importer 2019-04-04 21:18:58 PDT
<rdar://problem/49634426>