Bug 196530 – [JSC] Add FuzzerAgent, which has a hooks to get feedback & inject fuzz data into JSC

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

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 366587 [details]
Patch

[Yusuke Suzuki]

Created attachment 366589 [details]
Patch

[Yusuke Suzuki]

Created attachment 366590 [details]
Patch

[Yusuke Suzuki]

Maybe, randomizing part is too simple, but I'll try running this debug build this night, and see the results tomorrow morning.

[Yusuke Suzuki]

I got several test failures. Looking into them.

stress/string-equal-exception-check.js reveals missing exception check.
stress/strict-to-this-int.js reveals incorrect result.
stress/regress-153486.js fails. need to investigate it.

[Yusuke Suzuki]

** The following JSC stress test failures have been introduced:
        jsc-layout-tests.yaml/js/script-tests/dfg-double-vote-fuzz.js.layout
        stress/activation-sink-osrexit-default-value-tdz-error.js.ftl-no-cjit-small-pool
        stress/activation-sink-osrexit-default-value.js.ftl-no-cjit-small-pool
        stress/arrowfunction-lexical-this-activation-sink-osrexit.js.ftl-no-cjit-small-pool
        stress/op_bitand.js.misc-ftl-no-cjit
        stress/op_bitor.js.misc-ftl-no-cjit
        stress/op_bitxor.js.misc-ftl-no-cjit
        stress/regress-153486.js.default
        stress/regress-153486.js.dfg-eager
        stress/regress-153486.js.dfg-eager-no-cjit-validate
        stress/regress-153486.js.dfg-maximal-flush-validate-no-cjit
        stress/regress-153486.js.ftl-eager
        stress/regress-153486.js.ftl-eager-no-cjit
        stress/regress-153486.js.ftl-eager-no-cjit-b3o1
        stress/regress-153486.js.ftl-no-cjit-b3o0
        stress/regress-153486.js.ftl-no-cjit-no-put-stack-validate
        stress/regress-153486.js.ftl-no-cjit-validate-sampling-profiler
        stress/regress-153486.js.no-cjit-validate-phases
        stress/regress-153486.js.no-ftl
        stress/regress-153486.js.no-llint
        stress/rest-parameter-many-arguments.js.ftl-no-cjit-small-pool
        stress/strict-to-this-int.js.dfg-eager
        stress/strict-to-this-int.js.dfg-eager-no-cjit-validate
        stress/strict-to-this-int.js.no-cjit-validate-phases
        stress/strict-to-this-int.js.no-ftl
        stress/strict-to-this-int.js.no-llint
        stress/string-equal-exception-check.js.default
        stress/tailCallForwardArguments.js.ftl-no-cjit-small-pool
        stress/validate-int-52-ai-state.js.default

Results for JSC stress tests:
    29 failures found.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #6)
> ** The following JSC stress test failures have been introduced:
>         jsc-layout-tests.yaml/js/script-tests/dfg-double-vote-fuzz.js.layout
>        
> stress/activation-sink-osrexit-default-value-tdz-error.js.ftl-no-cjit-small-
> pool
>        
> stress/activation-sink-osrexit-default-value.js.ftl-no-cjit-small-pool
>        
> stress/arrowfunction-lexical-this-activation-sink-osrexit.js.ftl-no-cjit-
> small-pool
>         stress/op_bitand.js.misc-ftl-no-cjit
>         stress/op_bitor.js.misc-ftl-no-cjit
>         stress/op_bitxor.js.misc-ftl-no-cjit
>         stress/regress-153486.js.default
>         stress/regress-153486.js.dfg-eager
>         stress/regress-153486.js.dfg-eager-no-cjit-validate
>         stress/regress-153486.js.dfg-maximal-flush-validate-no-cjit
>         stress/regress-153486.js.ftl-eager
>         stress/regress-153486.js.ftl-eager-no-cjit
>         stress/regress-153486.js.ftl-eager-no-cjit-b3o1
>         stress/regress-153486.js.ftl-no-cjit-b3o0
>         stress/regress-153486.js.ftl-no-cjit-no-put-stack-validate
>         stress/regress-153486.js.ftl-no-cjit-validate-sampling-profiler
>         stress/regress-153486.js.no-cjit-validate-phases
>         stress/regress-153486.js.no-ftl
>         stress/regress-153486.js.no-llint
>         stress/rest-parameter-many-arguments.js.ftl-no-cjit-small-pool
>         stress/strict-to-this-int.js.dfg-eager
>         stress/strict-to-this-int.js.dfg-eager-no-cjit-validate
>         stress/strict-to-this-int.js.no-cjit-validate-phases
>         stress/strict-to-this-int.js.no-ftl
>         stress/strict-to-this-int.js.no-llint
>         stress/string-equal-exception-check.js.default
>         stress/tailCallForwardArguments.js.ftl-no-cjit-small-pool
>         stress/validate-int-52-ai-state.js.default
> 
> Results for JSC stress tests:
>     29 failures found.

Many of them are simple timeout. But it seems that some of them are real issues.

[Yusuke Suzuki]

Created attachment 366646 [details]
Patch

[EWS Watchlist]

Attachment 366646 [details] did not pass style-queue:


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


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 366646 [details]
Patch

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

r=me

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:844
> +            auto* fuzzerAgent = m_vm->fuzzerAgent();
> +            if (UNLIKELY(fuzzerAgent))
> +                return fuzzerAgent->getPrediction(codeBlock, bytecodeIndex, prediction);

nit: I like to write this as a single line:
if (UNLIKELY(auto* fuzzerAgent = m_vm->fuzzerAgent()))

> Source/JavaScriptCore/runtime/RandomizingFuzzerAgent.cpp:36
> +SpeculatedType RandomizingFuzzerAgent::getPrediction(CodeBlock*, int, SpeculatedType)

Can we enable this on some of our tests once we pass all tests with it? Maybe worth filing a bug.

[Yusuke Suzuki]

Comment on attachment 366646 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:844
>> +                return fuzzerAgent->getPrediction(codeBlock, bytecodeIndex, prediction);
> 
> nit: I like to write this as a single line:
> if (UNLIKELY(auto* fuzzerAgent = m_vm->fuzzerAgent()))

Unfortunately, we cannot do this since `auto* fuzzerAgent = m_vm->fuzzerAgent()` is not expression, so we cannot wrap it with UNLIKELY.

if (UNLIKELY(auto* fuzzerAgent = m_vm->fuzzerAgent())) => compile error

This is why we have two lines now... :(

>> Source/JavaScriptCore/runtime/RandomizingFuzzerAgent.cpp:36
>> +SpeculatedType RandomizingFuzzerAgent::getPrediction(CodeBlock*, int, SpeculatedType)
> 
> Can we enable this on some of our tests once we pass all tests with it? Maybe worth filing a bug.

Yeah, I think we can add an another variant like "-value-profiling-fuzz" in the stress tests to fuzz our engine more. I've opened the bug for this https://bugs.webkit.org/show_bug.cgi?id=196569.

[Yusuke Suzuki]

Committed r243832: <https://trac.webkit.org/changeset/243832>

[Radar WebKit Bug Importer]

<rdar://problem/49576836>