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)

All
Linux
defect
Not set
critical

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.
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 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 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 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 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+
Attachment #8717913 - Attachment is obsolete: true
Attachment #8717914 - Attachment is obsolete: true
Attachment #8718336 - Flags: review?(luke) → review+
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 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 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).
(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.
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
Whiteboard: [adv-main47-]
You need to log in before you can comment on or make changes to this bug.