Closed Bug 1455593 Opened 6 years ago Closed 6 years ago

Changes to BinAST fuzzer for Multipart Reader

Categories

(Core :: JavaScript Engine, enhancement, P2)

All
Linux
enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: decoder, Assigned: decoder)

Details

(Keywords: sec-want, Whiteboard: [adv-main64-])

Attachments

(1 file, 1 obsolete file)

Now that the BinAST multipart reader code landed, we need to make minor changes to the fuzzing code so the new files are instrumented and used for fuzzing instead of the previous BinTokenReaderTester class.

Also, we found that disabling the constant matching in BinTokenReaderBase is a very easy change for fuzzing builds that would allow the fuzzer to spend less time trying to get these constants right.
Comment on attachment 8969626 [details]
Bug 1455593 - BinAST multipart fuzzing changes.

https://reviewboard.mozilla.org/r/238416/#review244172

::: js/src/frontend/BinTokenReaderBase.h:112
(Diff revision 1)
>          MOZ_ASSERT(!cx_->isExceptionPending());
>  
>          if (current_ + N - 1 > stop_)
>              return false;
>  
> +#ifndef FUZZING

Nit: A comment would be nice.
Attachment #8969626 - Flags: review?(dteller) → review+
Comment on attachment 8969626 [details]
Bug 1455593 - BinAST multipart fuzzing changes.

https://reviewboard.mozilla.org/r/238416/#review244174
Attachment #8969626 - Flags: review+
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1bcd80c9a73
BinAST multipart fuzzing changes. r=Yoric
Backed out for spidermonkey failures on testBinASTReader.cpp:235

backout: https://hg.mozilla.org/integration/autoland/rev/f39f926f02fb

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d1bcd80c9a73a647a583958bef60816ae90d3d6b&selectedJob=174767738

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174767738&repo=autoland&lineNumber=21905

[task 2018-04-20T13:46:05.322Z] AddressSanitizer:DEADLYSIGNAL
[task 2018-04-20T13:46:05.322Z] =================================================================
[task 2018-04-20T13:46:05.322Z] ==66444==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x00000071f274 bp 0x7fffffffa950 sp 0x7fffffff96e0 T0)
[task 2018-04-20T13:46:05.322Z] ==66444==The signal is caused by a WRITE memory access.
[task 2018-04-20T13:46:05.322Z] ==66444==Hint: address points to the zero page.
[task 2018-04-20T13:46:05.779Z]     #0 0x71f273 in void runTestFromPath<js::frontend::BinTokenReaderTester>(JSContext*, char const*) /builds/worker/workspace/build/src/js/src/jsapi-tests/testBinASTReader.cpp:235:13
[task 2018-04-20T13:46:05.780Z]     #1 0x718a5b in void runTestFromPath<js::frontend::BinTokenReaderTester>(JSContext*, char const*) /builds/worker/workspace/build/src/js/src/jsapi-tests/testBinASTReader.cpp:125:13
[task 2018-04-20T13:46:05.780Z]     #2 0x718a5b in void runTestFromPath<js::frontend::BinTokenReaderTester>(JSContext*, char const*) /builds/worker/workspace/build/src/js/src/jsapi-tests/testBinASTReader.cpp:125:13
[task 2018-04-20T13:46:05.780Z]     #3 0x718a5b in void runTestFromPath<js::frontend::BinTokenReaderTester>(JSContext*, char const*) /builds/worker/workspace/build/src/js/src/jsapi-tests/testBinASTReader.cpp:125:13
[task 2018-04-20T13:46:05.780Z]     #4 0x7169d6 in cls_testBinASTReaderSimpleECMAScript2::run(JS::Handle<JSObject*>) /builds/worker/workspace/build/src/js/src/jsapi-tests/testBinASTReader.cpp:304:5
[task 2018-04-20T13:46:05.853Z]     #5 0xc6fedc in main /builds/worker/workspace/build/src/js/src/jsapi-tests/tests.cpp:132:19
[task 2018-04-20T13:46:05.919Z]     #6 0x7ffff63ddeac in __libc_start_main /build/eglibc-ZYONVs/eglibc-2.13/csu/libc-start.c:244
[task 2018-04-20T13:46:05.919Z]     #7 0x47c294 in _start (/builds/worker/workspace/build/src/obj-spider/dist/bin/jsapi-tests+0x47c294)
[task 2018-04-20T13:46:05.919Z] 
[task 2018-04-20T13:46:05.919Z] AddressSanitizer can not provide additional info.
[task 2018-04-20T13:46:05.919Z] SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/build/src/js/src/jsapi-tests/testBinASTReader.cpp:235:13 in void runTestFromPath<js::frontend::BinTokenReaderTester>(JSContext*, char const*)
[task 2018-04-20T13:46:05.919Z] ==66444==ABORTING
[task 2018-04-20T13:46:05.965Z] make -C js/src check-jstests
[task 2018-04-20T13:46:06.236Z] make[1]: Entering directory '/builds/worker/workspace/build/src/obj-spider/js/src'
[task 2018-04-20T13:46:06.237Z] ../../dist/bin/run-mozilla.sh /builds/worker/workspace/build/src/obj-spider/_virtualenv/bin/python -u /builds/worker/workspace/build/src/js/src/tests/jstests.py \
[task 2018-04-20T13:46:06.237Z] 	--no-progress --format=automation --timeout 300 \
[task 2018-04-20T13:46:06.237Z] 	--jitflags=none \
[task 2018-04-20T13:46:06.237Z] 	../../dist/bin/js
Flags: needinfo?(choller)
The problem here is that the old BinTokenReaderTester also uses the matchConst function that I tried to patch here. In the new Multipart reader, it is only used for the header. Maybe we should additionally guard this by some kind of runtime check? One option would be to add a (fuzzing-only) API to the parser that allows switching off the matchConst strict checking at runtime. Does that sound ok to you Yoric, or would you prefer a simpler solution (e.g. using getenv/setenv)?
Flags: needinfo?(choller) → needinfo?(dteller)
Would it be ok if we just skipped the calls `matchConst` in the BinTokenReaderMultipart if we're in #ifdef FUZZING?
Flags: needinfo?(dteller) → needinfo?(choller)
Do you mean commenting out the checks directly in BinTokenReaderMultipart, e.g. disabling this?

https://searchfox.org/mozilla-central/source/js/src/frontend/BinTokenReaderMultipart.cpp#69

If so, I guess that would work too.
Flags: needinfo?(choller) → needinfo?(dteller)
Yeah, this seems to be the simplest way to do it, right?
Flags: needinfo?(dteller)
Priority: -- → P2
Attachment #8969626 - Attachment is obsolete: true
Comment on attachment 9009567 [details]
Bug 1455593 - BinAST multipart fuzzing changes. r?arai

Tooru Fujisawa [:arai] has approved the revision.
Attachment #9009567 - Flags: review+
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b26a70a0fe8f
BinAST multipart fuzzing changes. r=arai
https://hg.mozilla.org/mozilla-central/rev/b26a70a0fe8f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Whiteboard: [adv-main64-]
You need to log in before you can comment on or make changes to this bug.