If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add persistent AFL support to jsshell for wasm testing

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: decoder, Assigned: decoder)

Tracking

(Blocks: 1 bug, {sec-want})

Trunk
mozilla47
All
Linux
sec-want
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [adv-main47-])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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

2 years ago
Created attachment 8717913 [details] [diff] [review]
(Part 1) Factor essential wasmEval code into new public helper function DecodeAndLink
Attachment #8717913 - Flags: feedback?(luke)
Attachment #8717913 - Flags: feedback?(bbouvier)
(Assignee)

Comment 2

2 years ago
Created attachment 8717914 [details] [diff] [review]
(Part 2) Add AFL-style helper function wasmLoop for persistent fuzzing

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

2 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

2 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 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+
(Assignee)

Comment 7

2 years ago
Created attachment 8718336 [details]
MozReview Request: Bug 1247247 - Factor essential wasmEval code into public helper function. r=luke

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

2 years ago
Created attachment 8718337 [details]
MozReview Request: Bug 1247247 - Add AFL-style wasmLoop function for persistent fuzzing. r=luke

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

2 years ago
Attachment #8717913 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8717914 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8718336 - Flags: review?(luke) → review+

Comment 9

2 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

2 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

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/acaeaa89ffc1
https://hg.mozilla.org/integration/mozilla-inbound/rev/54f291017f63
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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/acaeaa89ffc1
https://hg.mozilla.org/mozilla-central/rev/54f291017f63
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Whiteboard: [adv-main47-]
You need to log in before you can comment on or make changes to this bug.