Closed Bug 1431090 Opened 2 years ago Closed 2 years ago

Add FuzzingInterface support to JS engine

Categories

(Core :: JavaScript Engine, enhancement)

All
Linux
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: decoder, Assigned: decoder)

References

Details

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

Attachments

(2 files, 1 obsolete file)

This bug will only be refactoring the tools/fuzzing/ interface and integrating that into the JS engine with example/skeleton code.
Group: javascript-core-security
Blocks: 1431439
Blocks: 1431443
(In reply to Christian Holler (:decoder) from comment #2)
> Created attachment 8943659 [details]
> Bug 1431090 - Prepare tools/fuzzing/ to be used with JS_STANDALONE.
> 
> Review commit: https://reviewboard.mozilla.org/r/214030/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/214030/

This first patch is all about adjusting tools/fuzzing/ in such a way that the relevant parts can be reused in the JS engine. Changes in detail include:

* Various JS_STANDALONE checks to exclude parts that cannot be included in those builds

* Turn LibFuzzerRegistry and LibFuzzerRunner into generic FuzzerRegistry and FuzzerRunner classes and use them for AFL as well. Previously, AFL was piggy-backing on gtests which was kind of an ugly solution anyway (besides that it can't work in JS). Now more code like registry and harness is shared between the two and they follow almost the same call paths and entry points. AFL macros in FuzzingInterface have been rewritten accordingly. This also required name changes in various places. Furthermore, this unifies the way, the fuzzing target is selected, using the FUZZER environment variable rather than LIBFUZZER (using LIBFUZZER in browser builds will give you a deprecation warning because I know some people are using this already and need time to switch). Previously, AFL target had to be selected using GTEST_FILTER, so this is also much better now.

* I had to split up FuzzingInterface* such that the STREAM parts are in a separate set of files FuzzingInterfaceStream* because they use nsStringStream which is not allowed to be included into the JS engine even in a full browser build (error: "Using XPCOM strings is limited to code linked into libxul."). I also had to pull FuzzingInterface.cpp (the RAW part only) into the header and make it static because otherwise, would have to make not only separate files but also separate libraries to statically link to the JS engine, which seemed overkill for a single small function. The streaming equivalent of the function is still in a cpp file.

* LibFuzzerRegister functions are now unique by appending the module name to avoid redefinition errors.

Tested all of this with local JS engine builds as well as local full browser builds with libfuzzer and AFL.
(In reply to Christian Holler (:decoder) from comment #3)
> Created attachment 8943660 [details]
> Bug 1431090 - Add FuzzingInterface support to JS engine.
> 
> Review commit: https://reviewboard.mozilla.org/r/214032/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/214032/

This patch adds basic support for the fuzzing interface in the JS engine on top of the last patch. This includes all the necessary code except for actual targets (just an example target skeleton).

One thing I am not 100% sure about is the way I included the necessary code parts into the JS engine. I add "tools/fuzzing" to DIRS in the JS_STANDALONE case only, because I get an error that it is registered twice otherwise in the full browser build. That forces me to explicitly mention the fuzzer-registry code in js/src/fuzz-tests/moz.build to statically link it to the JS parts in a full browser build. This works for all build cases, but another pair of eyes here would be particularly helpful.
Comment on attachment 8943660 [details]
Bug 1431090 - Add FuzzingInterface support to JS engine.

https://reviewboard.mozilla.org/r/214032/#review220008

Looks good. Mostly just style nits - I didn't review the actual code too carefully since these are just helpers for fuzzing and apparently it works fine.

The moz.build changes require review from a build system peer (maybe Nathan or glandium).

::: js/src/fuzz-tests/testExample.cpp:20
(Diff revision 1)
> +using namespace js;
> +
> +extern JS::PersistentRootedObject global;
> +extern JSContext* cx;
> +
> +int testExampleInit(int *argc, char ***argv) {

Nit:

```
static int
TestExampleInit(int* argc, char*** argv)
{
```

::: js/src/fuzz-tests/testExample.cpp:25
(Diff revision 1)
> +int testExampleInit(int *argc, char ***argv) {
> +  /* This function is called once at startup. You can use it to e.g. read
> +     environment variables to initialize additional options you might need.
> +     Note that `cx` and `global` are pre-initialized by the harness.
> +  */
> +  return 0;

Nit: the indentation here is a bit off - should be 4 spaces instead of 2.

::: js/src/fuzz-tests/testExample.cpp:28
(Diff revision 1)
> +     Note that `cx` and `global` are pre-initialized by the harness.
> +  */
> +  return 0;
> +}
> +
> +int testExampleFuzz(const uint8_t* buf, size_t size)

```
static int
TestExampleFuzz(...args...)
{
```

::: js/src/fuzz-tests/testExample.cpp:33
(Diff revision 1)
> +int testExampleFuzz(const uint8_t* buf, size_t size)
> +{
> +    {
> +        /* Add code here that processes the given buffer.
> +           While doing so, you need to follow these rules:
> +        

Nit: trailing whitespace here.

::: js/src/fuzz-tests/testExample.cpp:52
(Diff revision 1)
> +    /* If your code directly or indirectly allocates GC memory, then it makes sense
> +       to attempt and collect that after every iteration. This should detect GC issues
> +       as soon as possible (right after your iteration), rather than later when your
> +       code happens to trigger GC coincidentially. You can of course disable this code
> +       if it is not required in your use case, which will speed up fuzzing. */
> +    JS::PrepareForFullGC(cx);

So instead of this goto, you could #include "mozilla/ScopeExit.h" and then add this to the top of this function:

```
auto gcGuard = MakeScopeExit([&] {
    JS::PrepareForFullGC(cx);
    JS::GCForReason(cx, GC_NORMAL, JS::gcreason::API);
});
```

Then you can just |return 0;| everywhere instead of |goto end;| and we will GC before we return.

::: js/src/fuzz-tests/tests.cpp:22
(Diff revision 1)
> +#include "FuzzerDefs.h"
> +#endif
> +
> +JS::PersistentRootedObject global;
> +JSContext* cx = nullptr;
> +JSCompartment* oldCompartment = nullptr;

Maybe gGlobal, gCx, gOldCompartment, to make it clear these are globals. Especially |cx| usually isn't so it would avoid confusion.

::: js/src/fuzz-tests/tests.cpp:24
(Diff revision 1)
> +
> +JS::PersistentRootedObject global;
> +JSContext* cx = nullptr;
> +JSCompartment* oldCompartment = nullptr;
> +
> +static const JSClass * getGlobalClass() {

```
static const JSClass*
GetGlobalClass()
{
```

::: js/src/fuzz-tests/tests.cpp:38
(Diff revision 1)
> +        &cOps
> +    };
> +    return &c;
> +}
> +
> +static JSObject* jsfuzz_createGlobal(JSContext* cx, JSPrincipals* principals)

\n after `JSObject*` here and similar for functions below.

::: js/src/fuzz-tests/tests.cpp:68
(Diff revision 1)
> +    *cx = JS_NewContext(8L * 1024 * 1024);
> +    if (!*cx)
> +        return false;
> +
> +    const size_t MAX_STACK_SIZE =
> +#if (defined(DEBUG) && defined(__SUNPRO_CC)) || defined(__sparc__)

It's probably okay to just use 5000000 always?

::: js/src/fuzz-tests/tests.cpp:102
(Diff revision 1)
> +        JS_DestroyContext(cx);
> +        cx = nullptr;
> +    }
> +}
> +
> +using namespace mozilla;

Move this after the #include section.

::: js/src/fuzz-tests/tests.cpp:116
(Diff revision 1)
> +    if (!jsfuzz_init(&cx, &global)) {
> +        fprintf(stderr, "Error: Call to jsfuzz_init() failed\n");
> +        return 1;
> +    }
> +
> +    std::string moduleNameStr(getenv("FUZZER"));

What happens if getenv returns nullptr? Maybe we should print a message to stderr if FUZZER is not defined?

::: js/src/fuzz-tests/tests.cpp:122
(Diff revision 1)
> +
> +    FuzzerFunctions funcs = FuzzerRegistry::getInstance().getModuleFunctions(moduleNameStr);
> +    FuzzerInitFunc initFunc = funcs.first;
> +    FuzzerTestingFunc testingFunc = funcs.second;
> +    if (initFunc) {
> +      int ret = initFunc(&argc, &argv);

Nit: use 4 space instead of 2 space indent

::: js/src/fuzz-tests/tests.cpp:137
(Diff revision 1)
> +    }
> +
> +#ifdef LIBFUZZER
> +    fuzzer::FuzzerDriver(&argc, &argv, testingFunc);
> +#elif __AFL_COMPILER
> +    testingFunc(NULL, 0);

Nit: use nullptr instead of NULL everywhere.
Attachment #8943660 - Flags: review?(jdemooij)
(In reply to Christian Holler (:decoder) from comment #4)
> (In reply to Christian Holler (:decoder) from comment #2)
> > Created attachment 8943659 [details]
> > Bug 1431090 - Prepare tools/fuzzing/ to be used with JS_STANDALONE.
> > 
> > Review commit: https://reviewboard.mozilla.org/r/214030/diff/#index_header
> > See other reviews: https://reviewboard.mozilla.org/r/214030/
> 
> This first patch is all about adjusting tools/fuzzing/ in such a way that
> the relevant parts can be reused in the JS engine. Changes in detail include:

Thanks for explaining where all the moving parts wound up.  For future patches--and maybe even for this one prior to landing--can you put explanations like this in the commit message itself?  When reviewing patches in mozreview, it's helpful to have the explanation right there, rather than having to click back to the bug and find an explanation (if it exists).  For anybody who has to do code archaeology, it's also helpful to have things explained in the commit message rather than trawling through the bug.

I know it can be kind of a pain to do things this way--e.g. modifying the commit message when necessary as you make changes--but it is, as mentioned, really helpful.
Comment on attachment 8943659 [details]
Bug 1431090 - Prepare tools/fuzzing/ to be used with JS_STANDALONE.

https://reviewboard.mozilla.org/r/214030/#review220020

I think these changes are non-controversial, and they obviously work for you.  Some comments below.

::: tools/fuzzing/interface/FuzzingInterface.h:22
(Diff revision 1)
>  #ifdef __AFL_COMPILER
> -void afl_interface_stream(const char* testFile, FuzzingTestFuncStream testFunc);
> -void afl_interface_raw(const char* testFile, FuzzingTestFuncRaw testFunc);
>  
> -#define MOZ_AFL_INTERFACE_COMMON(initFunc)                                                    \
> +static int afl_interface_raw(const char* testFile, FuzzingTestFuncRaw testFunc) {

If we have `__AFL_COMPILER`, we're going to define this `static` function...but then right after this  `#ifdef` block, we're going to declare the same function again?  Why are we doing that?

::: tools/fuzzing/interface/moz.build:13
(Diff revision 1)
> +if CONFIG['JS_STANDALONE']:
> +    FINAL_LIBRARY = "js"

For ESR releases especially, we make standalone releases of the JS engine...or at least we try to.  Is `tools/fuzzing/interface/` and other relevant subdirectories going to be packaged in such releases?
Attachment #8943659 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #8)
> 
> If we have `__AFL_COMPILER`, we're going to define this `static`
> function...but then right after this  `#ifdef` block, we're going to declare
> the same function again?  Why are we doing that?

I fixed this, thanks! We used to have the implementation of this function in a separate .cpp file. Now that I had to move it to the header, I forgot to remove the declaration there.


> 
> ::: tools/fuzzing/interface/moz.build:13
> (Diff revision 1)
> > +if CONFIG['JS_STANDALONE']:
> > +    FINAL_LIBRARY = "js"
> 
> For ESR releases especially, we make standalone releases of the JS
> engine...or at least we try to.  Is `tools/fuzzing/interface/` and other
> relevant subdirectories going to be packaged in such releases?

Thanks, good point! I added this to the second patch now in this bug. The js/src/make-source-package.sh script will now copy the relevant parts of tools/fuzzing/ and bundle them. I also confirmed that the resulting standalone JS source tree builds with --enable-fuzzing.
(In reply to Jan de Mooij [:jandem] from comment #6)
> Comment on attachment 8943660 [details]
> Bug 1431090 - Add FuzzingInterface support to JS engine.
> 
> https://reviewboard.mozilla.org/r/214032/#review220008
> 
> Looks good. Mostly just style nits 

Updated the patch with all your comments addressed :)

> 
> The moz.build changes require review from a build system peer (maybe Nathan
> or glandium).

Nathan, can you review the moz.build parts and the make-source-package.sh change in this patch as well? Thanks!
Comment on attachment 8943659 [details]
Bug 1431090 - Prepare tools/fuzzing/ to be used with JS_STANDALONE.

https://reviewboard.mozilla.org/r/214030/#review220252
Attachment #8943659 - Flags: review?(nfroyd) → review+
Comment on attachment 8943660 [details]
Bug 1431090 - Add FuzzingInterface support to JS engine.

https://reviewboard.mozilla.org/r/214032/#review220242

r=me, but the below seems quite ungood.  Let's fix that before landing.

::: js/src/fuzz-tests/moz.build:52
(Diff revision 2)
> +# This is intended as a temporary workaround to enable VS2015.
> +if CONFIG['CC_TYPE'] in ('msvc', 'clang-cl'):
> +    CXXFLAGS += ['-wd4312']

Uh, this sounds like a pretty severe warning.  Why can we not fix the code that warns?
Attachment #8943660 - Flags: review?(nfroyd) → review+
Comment on attachment 8943660 [details]
Bug 1431090 - Add FuzzingInterface support to JS engine.

r+ from jandem via IRC, will try to land this now :)
Attachment #8943660 - Flags: review?(jdemooij) → review+
Depends on: 1432298
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb6acc9e44ad
Prepare tools/fuzzing/ to be used with JS_STANDALONE. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/2e98bda3f397
Add FuzzingInterface support to JS engine. r=froydnj
Attachment #8943660 - Attachment is obsolete: true
Attachment #8943660 - Flags: review?(jdemooij)
Comment on attachment 8944686 [details]
Bug 1431090 - Add FuzzingInterface support to JS engine.

This was r+'ed before, mozreview just dropped it because I forgot to push this patch in one review change.

Fix for the try bustage is in the other patch (warning failure on debug fuzzing build)
Attachment #8944686 - Flags: review?(nfroyd)
Attachment #8944686 - Flags: review?(jdemooij)
Attachment #8944686 - Flags: review+
Comment on attachment 8944686 [details]
Bug 1431090 - Add FuzzingInterface support to JS engine.

https://reviewboard.mozilla.org/r/214838/#review220506
Attachment #8944686 - Flags: review+
Comment on attachment 8944686 [details]
Bug 1431090 - Add FuzzingInterface support to JS engine.

Nit fix, use the right define in moz.build (FUZZING_INTERFACES instead of FUZZING)
Flags: needinfo?(choller)
Attachment #8944686 - Flags: review?(nfroyd)
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23cc0463fbe8
Prepare tools/fuzzing/ to be used with JS_STANDALONE. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/a89451212513
Add FuzzingInterface support to JS engine. r=jandem
https://hg.mozilla.org/mozilla-central/rev/23cc0463fbe8
https://hg.mozilla.org/mozilla-central/rev/a89451212513
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Whiteboard: [adv-main60-]
You need to log in before you can comment on or make changes to this bug.