Closed
Bug 1247247
Opened 8 years ago
Closed 8 years ago
Add persistent AFL support to jsshell for wasm testing
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: decoder, Assigned: decoder)
Details
(Keywords: sec-want, Whiteboard: [adv-main47-])
Attachments
(2 files, 2 obsolete files)
AFLFuzz (the tool we use for testing the wasm implementation) has a persistent testing mode. This mode involves creating a custom loop in C++ for the tool that stops the process (using SIGSTOP) when it has processed the input data, so the fuzzer can replace the input file with a new one and let the process continue. This particular style of persistent testing is very fast: A prototype of mine implemented in the JS shell has shown that it can easily reach 2000-3000 execs/s while the regular method with running the shell for each test gives 8-10 execs/s. One of the problems with my implementation is that I need to use functions from shell/OSObject.cpp to read the data while I also need to pass them to WasmEval in asmjs/Wasm.cpp. I also heard that Luke and others maybe wanted to implement another way to read and eval wasm data directly from stdin. Given that, I think it makes sense to expose WasmEval and WasmTextToBinary in Wasm.h so we can use these two functions from shell/js.cpp. Once that is done, the actual implementation of my wasmLoop function can be #ifdef'ed out, as there is a compiler macro for only activating this code when it is compiled by the AFL compiler wrapper. I'll attach two patches in a few.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8717913 -
Flags: feedback?(luke)
Attachment #8717913 -
Flags: feedback?(bbouvier)
Assignee | ||
Comment 2•8 years ago
|
||
This part needs to live in shell/js.cpp because it makes use of stuff in OSObject.cpp. However, since we might add more stuff in the future, I would love to have our own .cpp file for such fuzzing related stuff if you guys think that's a good idea and cleaner :)
Attachment #8717914 -
Flags: feedback?(luke)
Attachment #8717914 -
Flags: feedback?(bbouvier)
Comment 3•8 years ago
|
||
Comment on attachment 8717913 [details] [diff] [review] (Part 1) Factor essential wasmEval code into new public helper function DecodeAndLink Review of attachment 8717913 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/Wasm.cpp @@ +1171,5 @@ > } > > +bool > +wasm::DecodeAndLink(JSContext* cx, Handle<ArrayBufferObject*> code, > + HandleObject importObj, MutableHandleObject exportObj) nit: need to align 'H' in 'Handle' with 'J' in 'JSContext'. @@ +1212,5 @@ > +static bool > +WasmEval(JSContext* cx, unsigned argc, Value* vp) > +{ > + if (!CheckCompilerSupport(cx)) > + return false; Probably this should go in DecodeAndLink. @@ +1245,2 @@ > return false; > + } No { } for single-line then branch. ::: js/src/asmjs/Wasm.h @@ +49,5 @@ > +// and links the module's imports with the given import object. > +bool > +DecodeAndLink(JSContext* cx, JS::Handle<ArrayBufferObject*> code, > + JS::HandleObject importObj, > + JS::MutableHandleObject exportObj); How about renaming this 'Eval' (so it's wasm::Eval)? (nit: also need to align 2nd and 3rd line with 'J' in 'JSContext'.)
Attachment #8717913 -
Flags: feedback?(luke) → feedback+
Comment 4•8 years ago
|
||
Comment on attachment 8717914 [details] [diff] [review] (Part 2) Add AFL-style helper function wasmLoop for persistent fuzzing Review of attachment 8717914 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! ::: js/src/shell/js.cpp @@ +4921,5 @@ > +static bool > +WasmLoop(JSContext* cx, unsigned argc, Value* vp) > +{ > + if (!js::wasm::HasCompilerSupport(cx)) > + return false; With the movement of the HasCompilerSupport call to wasm::Eval suggested for the other patch, you could get rid of this. @@ +4955,5 @@ > + if (!filename) > + return false; > + > + while (__AFL_LOOP(1000)) { > + Rooted<js::TypedArrayObject*> typedArray(cx, &(FileAsTypedArray(cx, filename.ptr())->as<js::TypedArrayObject>()) ); FileAsTypedArray can fail, so you need to store in JSObject* first, null test, then create the Rooted<TypedArrayObject*>. Also, FileAsTypedArray appears to be a 'static' function as of this patch; I guess no compile error b/c unified compilation, but need to add 'extern' decl in OSObject.h. nit: no js:: before TypedArrayObject, no parens around 'FileAsTypedArray...'. Also general note that column width limit is 100. @@ +4957,5 @@ > + > + while (__AFL_LOOP(1000)) { > + Rooted<js::TypedArrayObject*> typedArray(cx, &(FileAsTypedArray(cx, filename.ptr())->as<js::TypedArrayObject>()) ); > + if (!TypedArrayObject::ensureHasBuffer(cx, typedArray)) { > + return false; nit: no { } around single-line then-branch.
Attachment #8717914 -
Flags: feedback?(luke) → feedback+
Comment 5•8 years ago
|
||
Comment on attachment 8717913 [details] [diff] [review] (Part 1) Factor essential wasmEval code into new public helper function DecodeAndLink Review of attachment 8717913 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/src/asmjs/Wasm.cpp @@ +1227,5 @@ > + ReportUsageError(cx, callee, "First argument must be an ArrayBuffer"); > + return false; > + } > + > + nit: two blank lines, please remove one ::: js/src/asmjs/Wasm.h @@ +19,5 @@ > #ifndef wasm_h > #define wasm_h > > #include "gc/Rooting.h" > +#include "vm/ArrayBufferObject.h" Can you instead just declare ArrayBufferObject*, to avoid including the whole header here? class ArrayBufferObject;
Attachment #8717913 -
Flags: feedback?(bbouvier) → feedback+
Comment 6•8 years ago
|
||
Comment on attachment 8717914 [details] [diff] [review] (Part 2) Add AFL-style helper function wasmLoop for persistent fuzzing Review of attachment 8717914 [details] [diff] [review]: ----------------------------------------------------------------- Great! ::: js/src/shell/js.cpp @@ +4960,5 @@ > + if (!TypedArrayObject::ensureHasBuffer(cx, typedArray)) { > + return false; > + } > + > + Rooted<ArrayBufferObject*> arrayBuffer(cx, typedArray->bufferUnshared()); typedArray->bufferUnshared() can return nullptr, can you add if (!arrayBuffer) return false; after this?
Attachment #8717914 -
Flags: feedback?(bbouvier) → feedback+
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/34527/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34527/
Attachment #8718336 -
Flags: review?(luke)
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/34529/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34529/
Attachment #8718337 -
Flags: review?(luke)
Assignee | ||
Updated•8 years ago
|
Attachment #8717913 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8717914 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8718336 -
Flags: review?(luke) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8718336 [details] MozReview Request: Bug 1247247 - Factor essential wasmEval code into public helper function. r=luke https://reviewboard.mozilla.org/r/34527/#review31213 Great!
Comment 10•8 years ago
|
||
Comment on attachment 8718337 [details] MozReview Request: Bug 1247247 - Add AFL-style wasmLoop function for persistent fuzzing. r=luke https://reviewboard.mozilla.org/r/34529/#review31209 ::: js/src/shell/OSObject.h:29 (Diff revision 1) > +JSObject* Trailing whitespace. (FYI, you can get this to show up as read in 'hg diff' by adding 'diff.trailingwhitespace = bold red_background' to a '[color]' section in .hgrc.
Attachment #8718337 -
Flags: review?(luke) → review+
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/acaeaa89ffc1 https://hg.mozilla.org/integration/mozilla-inbound/rev/54f291017f63
Comment 12•8 years ago
|
||
Comment on attachment 8717914 [details] [diff] [review] (Part 2) Add AFL-style helper function wasmLoop for persistent fuzzing Review of attachment 8717914 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/shell/js.cpp @@ +4960,5 @@ > + if (!TypedArrayObject::ensureHasBuffer(cx, typedArray)) { > + return false; > + } > + > + Rooted<ArrayBufferObject*> arrayBuffer(cx, typedArray->bufferUnshared()); typedArray->bufferUnshared() *can't* return nullptr because of the ensureHasBuffer just above here. So this is fine as-is (which appears to be how it landed, too).
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12) > > typedArray->bufferUnshared() *can't* return nullptr because of the > ensureHasBuffer just above here. So this is fine as-is (which appears to be > how it landed, too). Thanks Waldo! :) We came to the same conclusion on IRC, that's why I landed it as-is.
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/acaeaa89ffc1 https://hg.mozilla.org/mozilla-central/rev/54f291017f63
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•8 years ago
|
Whiteboard: [adv-main47-]
You need to log in
before you can comment on or make changes to this bug.
Description
•