1303757 - Add unified fuzzing interface, supporting AFL and libfuzzer

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Christian Holler (:decoder)]

The goal here is to provide a unified interface for developers, such that they can make their code fuzzable with both libfuzzer and aflfuzz at once. Several steps are required here:

1) Add --enable-fuzzing to enable fuzzing support in general
2) Detect the AFL compiler at build time to enable AFL parts of the interface
3) Remove --enable-libfuzzer and automatically enable it with --enable-fuzzing *unless* we are building with the AFL compiler

Note that libfuzzer has to remain disabled when building with the AFL compiler, as the AFL runtime and libfuzzer both implement the same ASan tracing API.

Comment 1

[Christian Holler (:decoder)]

Attachment: fuzzing-interface.patch

This patch does two things:

1) Add tools/fuzzing/interface/FuzzingInterface.h with the necessary macros for the unified interface.

2) Change the configure flags and defines as mentioned in comment 0 and add the AFL compiler detection.

Mike, I wasn't able to get the AFL compiler detection code to work anywhere else than inside of build/moz.configure/toolchain.configure, so I moved all of the configure stuff there. I guess that's not the right way to do it. Can you advise me how to either make all of the checks in toolkit/moz.configure or how to split these between the two files?

If you could take a brief look at the macros that would be nice as well, but they aren't complicated and it's just testing code.

I also have sample code for the image decoding code that makes use of this interface by implementing the two main functions (init and test function) and then calling the interface with

> FUZZING_INTERFACE_STREAM(FuzzingInitImage, FuzzingTestImage, Image);

and this is transparently expanded into the necessary libfuzzer or AFL code.

Feedback greatly appreciated.

Comment 2

[Mike Hommey [:glandium]]

Having this stuff in toolchain.configure is not really appealing.
But:
- You can't use try_compile without toolchain.configure being included.
- toolchain.configure is only included if --disable-compile-environment is not passed.
- when it is included, toolchain.configure is included *after* toolkit/moz.configure.

The second makes things a little complicated at the moment, but I have WIP that will hopefully allow to do it in with less complications.

The third is something that we need to address for various other things. I brought it up in this thread: https://groups.google.com/forum/#!topic/mozilla.dev.builds/XThvzlFdlKU but there is no answer to this need yet. This might be the right time to come up with something. Chris what do you think? (second point to last in the first message in the thread)

Comment 3

[Christian Holler (:decoder)]

Comment on attachment 8792537 [details] [diff] [review]
fuzzing-interface.patch

Nathan, could you meanwhile review the macro part of this patch (basically only what is in tools/fuzzing/interface/FuzzingInterface.h)?

This file contains a unified interface that should allow developers to implement one common entry point for both libfuzzer and aflfuzz.

Here is how it works:

It provides two macros that are supposed to be used by developers, FUZZING_INTERFACE_STREAM and FUZZING_INTERFACE_RAW. They both take an initialization function, a testing function and a module name. The init function is called by the fuzzer once to initialize everything we might need for testing. The testing function is called per input then. The macros differ only in their signature for the testing function, one takes an nsIInputStream while the other takes a raw buffer and length. These two interfaces made sense to me because I've seen uses for both so far in our codebase. The module name is a simple string to uniquely identify this fuzzing interface.

Internally, the macros expand to __LIBFUZZER_INTERFACE_* and __AFL_INTERFACE_*. The whole code is only active with --enable-fuzzing. When we compile with the AFL compiler, libfuzzer is automatically disabled and the AFL code is enabled instead. Both macros then implement what is necessary  for attaching the interface to the fuzzer: libfuzzer for example *always* expects the RAW interface, so we need wrapper code for the STREAM interface. AFLFuzz always expects to read its data from a file, so we need to wrap both RAW and STREAM to read the data from an external file.

Let me know if anything is unclear.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

(In reply to Mike Hommey [:glandium] from comment #2)
> Having this stuff in toolchain.configure is not really appealing.
> But:
> - You can't use try_compile without toolchain.configure being included.
> - toolchain.configure is only included if --disable-compile-environment is
> not passed.
> - when it is included, toolchain.configure is included *after*
> toolkit/moz.configure.
> 
> The second makes things a little complicated at the moment, but I have WIP
> that will hopefully allow to do it in with less complications.
> 
> The third is something that we need to address for various other things. I
> brought it up in this thread:
> https://groups.google.com/forum/#!topic/mozilla.dev.builds/XThvzlFdlKU but
> there is no answer to this need yet. This might be the right time to come up
> with something. Chris what do you think? (second point to last in the first
> message in the thread)

I'd be interested to hear more details about the proposal there (about automatically including some files from build/moz.configure/, toolkit/, and $build_project/), but in general do we have the order of some of this wrong, as in, shouldn't we just move the toolchain checks earlier, before app specific things? I know this would mean moving everything related to the compile environment and artifact builds earlier as well, but is there a downside to that I'm not thinking of?

The slightly cleaner way to do it so far has been to put things in a separate file whose include is also guarded by --disable-compile-environment, but it's not a real solution if our goal is to put this in toolkit/moz.configure.

Comment 5

[Christian Holler (:decoder)]

Forwarding comment 4 to glandium, as I cannot answer that :)

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8792537 [details] [diff] [review]
fuzzing-interface.patch

Review of attachment 8792537 [details] [diff] [review]:
-----------------------------------------------------------------

I have questions about the patch, but they're not dealbreakers.  f=me and I'll r+ a patch after discussion.

I think it makes more sense to move the bulk of the __AFL_INTERFACE_* macros to static functions in FuzzingInterface.cpp; those functions would then take a |const char*| for the filename and a |testFunc|--whatever type that has.

::: tools/fuzzing/interface/FuzzingInterface.h
@@ +24,5 @@
> +#include <fstream>
> +
> +using namespace mozilla;
> +
> +#define DUMMY_IF(x) if (x) __asm__ __volatile__("");

What is this here for?  I don't think you can use this in platform-independent code...

@@ +36,5 @@
> +      return;\
> +    }
> +
> +#define __AFL_INTERFACE_STREAM(initFunc, testFunc, moduleName) \
> +  void __afl_manual_init(void);\

What is this here for?

Also, I think the style is to line the backslashes up in whatever column is most convenient.  Emacs's C/C++ modes will do this for you automatically when you indent lines; I don't know about other editors.

@@ +48,5 @@
> +    rv = dirService->Get(NS_OS_CURRENT_WORKING_DIR,\
> +                         NS_GET_IID(nsIFile), getter_AddRefs(file));\
> +    MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));\
> +    file->AppendNative(nsDependentCString(testFilePtr));\
> +    std::string testFile(testFilePtr);\

This variable is unused AFAICT.

@@ +60,5 @@
> +                                       inputStream, 1024);\
> +        MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));\
> +        inputStream = bufStream;\
> +       }\
> +      testFunc(inputStream.forget());\

Do you really want to have testFunc take ownership of inputStream?  Why not pass in an nsIInputStream* instead?

@@ +65,5 @@
> +    }\
> +  }
> +
> +#define __AFL_INTERFACE_RAW(initFunc, testFunc, moduleName) \
> +  void __afl_manual_init(void);\

Same question here.

@@ +75,5 @@
> +      is.seekg (0, std::ios::end);\
> +      int len = is.tellg();\
> +      is.seekg (0, std::ios::beg);\
> +      MOZ_RELEASE_ASSERT(len >= 0);\
> +      char* buf = new char[len];\

I feel like you might as well try to optimize a little bit by something like:

nsTArray<uint8_t> data;
while(__AFL_LOOP(1000)) {
  ...
  int len = ...;
  if (len >= data.Length()) {
    data.SetLength(len);
  }
  is.read(data.Elements(), len);
  ...
  testFunc(data.Elements, len);
}

which avoids churning through memory.  Or is the idea that something like ASan + fuzzing will detect out-of-bounds reads from the allocated memory, and it would have a harder time doing that with nsTArray?

@@ +95,5 @@
> +    memcpy(buf, data, size);\
> +    nsCOMPtr<nsIStringInputStream> stringStream =\
> +      do_CreateInstance("@mozilla.org/io/string-input-stream;1");\
> +    MOZ_RELEASE_ASSERT(stringStream != nullptr);\
> +    stringStream->AdoptData((char*) buf, size);\

Instead of all this, you can just use NS_NewByteInputStream:

  nsCOMPtr<nsIInputStream> stream;
  // May want to assert that size <= INT32_MAX.
  nsresult rv = NS_NewByteInputStream(getter_AddRefs, data, size, NS_ASSIGNMENT_DEPEND);
  // ...

@@ +96,5 @@
> +    nsCOMPtr<nsIStringInputStream> stringStream =\
> +      do_CreateInstance("@mozilla.org/io/string-input-stream;1");\
> +    MOZ_RELEASE_ASSERT(stringStream != nullptr);\
> +    stringStream->AdoptData((char*) buf, size);\
> +    testFunc(stringStream.forget());\

Same question here as far as letting testFunc take ownership.

Comment 7

[Nathan Froyd [:froydnj]]

Posting the example code that actually uses the interfaces you are proposing is most helpful when reviewing patches like this as well.

Comment 8

[Christian Holler (:decoder)]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> 
> I think it makes more sense to move the bulk of the __AFL_INTERFACE_* macros
> to static functions in FuzzingInterface.cpp; those functions would then take
> a |const char*| for the filename and a |testFunc|--whatever type that has.

Agreed, sounds a little cleaner. I'll implement this in the next version of the patch.

> 
> ::: tools/fuzzing/interface/FuzzingInterface.h
> @@ +24,5 @@
> > +#include <fstream>
> > +
> > +using namespace mozilla;
> > +
> > +#define DUMMY_IF(x) if (x) __asm__ __volatile__("");
> 
> What is this here for?  I don't think you can use this in
> platform-independent code...

For now the code is supposed to work on Linux only. I was looking for a way to e.g. compare memory without those calls being removed because we're not actually using the result. This is a common pattern in fuzzing functions and you would want ASan to trigger on the reads even though they appear to be useless. Thoughts?

> 
> @@ +36,5 @@
> > +      return;\
> > +    }
> > +
> > +#define __AFL_INTERFACE_STREAM(initFunc, testFunc, moduleName) \
> > +  void __afl_manual_init(void);\
> 
> What is this here for?

I think this is a signature for AFL to pick up for deferred forkserver mode, but I think we don't need this anymore and it's a leftover from earlier versions. I'll try removing it.

> 
> Also, I think the style is to line the backslashes up in whatever column is
> most convenient.  Emacs's C/C++ modes will do this for you automatically
> when you indent lines; I don't know about other editors.

Will fix that.

> 
> @@ +48,5 @@
> > +    rv = dirService->Get(NS_OS_CURRENT_WORKING_DIR,\
> > +                         NS_GET_IID(nsIFile), getter_AddRefs(file));\
> > +    MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));\
> > +    file->AppendNative(nsDependentCString(testFilePtr));\
> > +    std::string testFile(testFilePtr);\
> 
> This variable is unused AFAICT.

+1

> 
> @@ +60,5 @@
> > +                                       inputStream, 1024);\
> > +        MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));\
> > +        inputStream = bufStream;\
> > +       }\
> > +      testFunc(inputStream.forget());\
> 
> Do you really want to have testFunc take ownership of inputStream?  Why not
> pass in an nsIInputStream* instead?

Actually, this is how the image tests do it:

https://dxr.mozilla.org/mozilla-central/source/image/test/gtest/Common.cpp?q=Common.cpp&redirect_type=direct#64

In the end, it ends up here:

https://dxr.mozilla.org/mozilla-central/source/image/test/gtest/TestDecodeToSurface.cpp#29

(And my fuzzing implementation for the image decoders is copied from the regular gtest version with fuzzing modifications).

And since the developer implements the testing function, I thought it is best to give the developer ownership of the stream in case some implementation he uses actually needs ownership of the pointer. Is that wrong thinking?

> 
> @@ +75,5 @@
> > +      is.seekg (0, std::ios::end);\
> > +      int len = is.tellg();\
> > +      is.seekg (0, std::ios::beg);\
> > +      MOZ_RELEASE_ASSERT(len >= 0);\
> > +      char* buf = new char[len];\
> 
> I feel like you might as well try to optimize a little bit by something like:
> 
> nsTArray<uint8_t> data;
> while(__AFL_LOOP(1000)) {
>   ...
>   int len = ...;
>   if (len >= data.Length()) {
>     data.SetLength(len);
>   }
>   is.read(data.Elements(), len);
>   ...
>   testFunc(data.Elements, len);
> }
> 
> which avoids churning through memory.  Or is the idea that something like
> ASan + fuzzing will detect out-of-bounds reads from the allocated memory,
> and it would have a harder time doing that with nsTArray?

I think for ASan it wouldn't matter which version you use. I will try to implement your version.

> 
> @@ +95,5 @@
> > +    memcpy(buf, data, size);\
> > +    nsCOMPtr<nsIStringInputStream> stringStream =\
> > +      do_CreateInstance("@mozilla.org/io/string-input-stream;1");\
> > +    MOZ_RELEASE_ASSERT(stringStream != nullptr);\
> > +    stringStream->AdoptData((char*) buf, size);\
> 
> Instead of all this, you can just use NS_NewByteInputStream:
> 
>   nsCOMPtr<nsIInputStream> stream;
>   // May want to assert that size <= INT32_MAX.
>   nsresult rv = NS_NewByteInputStream(getter_AddRefs, data, size,
> NS_ASSIGNMENT_DEPEND);
>   // ...

Will try this as well :)

I will also post another patch that shows how this interface is used.

Comment 9

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #2)
> Having this stuff in toolchain.configure is not really appealing.
> But:
> - You can't use try_compile without toolchain.configure being included.
> - toolchain.configure is only included if --disable-compile-environment is
> not passed.
> - when it is included, toolchain.configure is included *after*
> toolkit/moz.configure.

As of bug 1306138, toolchain.configure is included before toolkit/moz.configure. So you can now add your stuff in toolkit/moz.configure, provided you gate it on --enable-compile-environment, which at the moment will require you to use an include_when of a separate file. So, for now, create a toolkit/fuzzing.configure file and add include_when('fuzzing.configure', when='--enable-compile-environment') to toolkit/moz.configure.

Comment 10

[Christian Holler (:decoder)]

Attachment: fuzzing-interface-v2.patch

v2 of the interface patch with feedback addressed as mentioned in the last comment. I didn't change the pointer ownership thing for the reasons mentioned in the previous comments, and I changed the alloc mechanism in the RAW afl interface to use realloc as discussed on IRC. All other changes should match your suggestions and in fact the unused variable you found was a bug, it should have been used :)

I will also attach the image testing code that shows how this interface is used.

Comment 11

[Christian Holler (:decoder)]

Attachment: libfuzzer-image.patch

Example code for image decoding that implements the fuzzing interface.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8798665 [details] [diff] [review]
fuzzing-interface-v2.patch

Review of attachment 8798665 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, a few minor things.

::: tools/fuzzing/interface/FuzzingInterface.cpp
@@ +47,5 @@
> +      is.open (testFile, std::ios::binary);
> +      is.seekg (0, std::ios::end);
> +      int len = is.tellg();
> +      is.seekg (0, std::ios::beg);
> +      MOZ_RELEASE_ASSERT(len >= 0);

If |len| is allowed to be zero here...

@@ +49,5 @@
> +      int len = is.tellg();
> +      is.seekg (0, std::ios::beg);
> +      MOZ_RELEASE_ASSERT(len >= 0);
> +      buf = (char*)realloc(buf, len);
> +      MOZ_RELEASE_ASSERT(buf);

..then this buffer is allowed to be null here.  I think you should either change the above assert, remove this one entirely, or make this assert conditional on |len > 0|.

::: tools/fuzzing/interface/FuzzingInterface.h
@@ +62,5 @@
> +#define __LIBFUZZER_INTERFACE_STREAM(initFunc, testFunc, moduleName) \
> +  int LibFuzzerTest##moduleName (const uint8_t *data, size_t size) { \
> +    if (size > INT32_MAX)                                            \
> +      return 0;                                                      \
> +    uint8_t* buf = new uint8_t[size];                                \

So you allocate this buffer, and then you do nothing with it?  I think you want to remove this, since the stream below can just depend on |data|.

Comment 13

[Christian Holler (:decoder)]

(In reply to Nathan Froyd [:froydnj] from comment #12)
> Comment on attachment 8798665 [details] [diff] [review]
> fuzzing-interface-v2.patch
> 
> Review of attachment 8798665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, a few minor things.
> 
> ::: tools/fuzzing/interface/FuzzingInterface.cpp
> @@ +47,5 @@
> > +      is.open (testFile, std::ios::binary);
> > +      is.seekg (0, std::ios::end);
> > +      int len = is.tellg();
> > +      is.seekg (0, std::ios::beg);
> > +      MOZ_RELEASE_ASSERT(len >= 0);
> 
> If |len| is allowed to be zero here...
> 
> @@ +49,5 @@
> > +      int len = is.tellg();
> > +      is.seekg (0, std::ios::beg);
> > +      MOZ_RELEASE_ASSERT(len >= 0);
> > +      buf = (char*)realloc(buf, len);
> > +      MOZ_RELEASE_ASSERT(buf);
> 
> ..then this buffer is allowed to be null here.  I think you should either
> change the above assert, remove this one entirely, or make this assert
> conditional on |len > 0|.

You are right, the 0 case here isn't handled correctly, thanks for spotting this. But it can't be handled as you described. The fuzzer is allowed to truncate the file and the program must not assert when the fuzzer does. So the assert is in place only for the case that len < 0 which indicates some sort of failure in reading the file at all (which is a reason to abort). The case that len == 0 should be handled right after that by just closing the stream and issue a continue for the loop without any testing, so the fuzzer can provide a new file. I will implement that additional check.

> 
> ::: tools/fuzzing/interface/FuzzingInterface.h
> @@ +62,5 @@
> > +#define __LIBFUZZER_INTERFACE_STREAM(initFunc, testFunc, moduleName) \
> > +  int LibFuzzerTest##moduleName (const uint8_t *data, size_t size) { \
> > +    if (size > INT32_MAX)                                            \
> > +      return 0;                                                      \
> > +    uint8_t* buf = new uint8_t[size];                                \
> 
> So you allocate this buffer, and then you do nothing with it?  I think you
> want to remove this, since the stream below can just depend on |data|.

Thanks, that's a leftover from the previous version, I'll remove this.

I will also split this patch into two: One for the toolchain thing and one for the interface stuff, so they can be reviewed separately.

Comment 14

[Christian Holler (:decoder)]

Attachment: fuzzing-interface-v3.patch

Interface patch for review with changed as discussed in last comment. The build system part has been split out and I will post a separate patch for that.

Comment 15

[Christian Holler (:decoder)]

Attachment: fuzzing-interface-build-v3.patch

Thanks to glandium's refactoring in bug 1306138, we can now simply have the AFL compiler detection and all other build system related stuff of this interface in toolkit/moz.configure (yay \o/). I tested this with both build types (libfuzzer and AFL).

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8799811 [details] [diff] [review]
fuzzing-interface-build-v3.patch

Review of attachment 8799811 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/moz.configure
@@ +765,5 @@
>  set_config('SKIA_INCLUDES', skia_includes)
>  
>  # Support various fuzzing options
>  # ==============================================================
> +@depends(try_compile(body='__AFL_COMPILER;', check_msg='for AFL compiler'))

This would break --disable-compile-environment builds until bug 1296530. Until then you need something like:

include_when('fuzzing.configure', '--enable-compile-environment')

And move this to toolkit/fuzzing.configure

That being said, you also make the __AFL_COMPILER check run for everything, not only when --enable-fuzzing is passed. What you want here is to move this down and make it try_compile(..., when='--enable-fuzzing')

@@ +773,5 @@
>  
> +option('--enable-fuzzing', help='Enable fuzzing support')
> +
> +@depends('--enable-fuzzing')
> +def enable_fuzzing(value):

you don't really need this intermediate function. You can just go with:

@depends('--enable-fuzzing', try_compile(..., when='--enable-fuzzing'))
def enable_libfuzzer(fuzzing, afl):
    if fuzzing and not afl:
        return True

Comment 17

[Christian Holler (:decoder)]

(In reply to Mike Hommey [:glandium] from comment #16)
> Comment on attachment 8799811 [details] [diff] [review]
> fuzzing-interface-build-v3.patch
> 
> 
> @@ +773,5 @@
> >  
> > +option('--enable-fuzzing', help='Enable fuzzing support')
> > +
> > +@depends('--enable-fuzzing')
> > +def enable_fuzzing(value):
> 
> you don't really need this intermediate function. You can just go with:
> 
> @depends('--enable-fuzzing', try_compile(..., when='--enable-fuzzing'))
> def enable_libfuzzer(fuzzing, afl):
>     if fuzzing and not afl:
>         return True


I see why you wouldn't need the intermediate function "have_afl_compiler", but I would still need the "enable_fuzzing" function for the set_config/set_define on FUZZING, right? Did you mark the wrong function here and meant to say that I can get rid of "have_afl_compiler" instead? Thanks!

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8799809 [details] [diff] [review]
fuzzing-interface-v3.patch

Review of attachment 8799809 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay in review, and sorry for only picking up on these naming things so late in the game.  r=me with things changed as below.

::: tools/fuzzing/interface/FuzzingInterface.h
@@ +22,5 @@
> +#include "nsStringStream.h"
> +
> +#include <fstream>
> +
> +using namespace mozilla;

Header files aren't permitted to use |using namespace|.  Please wrap things up in |namespace mozilla {}| (the __afl_interface_* functions, and probably the Fuzzing* typedefs as well) and add mozilla:: qualifiers where necessary.

@@ +27,5 @@
> +
> +typedef int(*FuzzingTestFuncRaw)(const uint8_t*, size_t);
> +typedef int(*FuzzingTestFuncStream)(nsCOMPtr<nsIInputStream>);
> +
> +#ifdef __AFL_COMPILER

Names starting with underscores are reserved for the use of the standard library or the compiler, so they're technically not permitted in user code, even if they happen to work.  Please remove the double underscore prefixes throughout this header, prefixing macros with MOZ_ instead, and putting things in the mozilla namespace where necessary.

@@ +59,5 @@
> +#endif
> +
> +#ifdef LIBFUZZER
> +#define __LIBFUZZER_INTERFACE_STREAM(initFunc, testFunc, moduleName) \
> +  int LibFuzzerTest##moduleName (const uint8_t *data, size_t size) { \

This function should be |static|.

@@ +93,5 @@
> +
> +#define FUZZING_INTERFACE_RAW(initFunc, testFunc, moduleName)    \
> +  __LIBFUZZER_INTERFACE_RAW(initFunc, testFunc, moduleName);     \
> +  __AFL_INTERFACE_RAW(initFunc, testFunc, moduleName);
> + 

Nit: trailing whitespace.

Comment 19

[Mike Hommey [:glandium]]

(In reply to Christian Holler (:decoder) from comment #17)
> (In reply to Mike Hommey [:glandium] from comment #16)
> > Comment on attachment 8799811 [details] [diff] [review]
> > fuzzing-interface-build-v3.patch
> > 
> > 
> > @@ +773,5 @@
> > >  
> > > +option('--enable-fuzzing', help='Enable fuzzing support')
> > > +
> > > +@depends('--enable-fuzzing')
> > > +def enable_fuzzing(value):
> > 
> > you don't really need this intermediate function. You can just go with:
> > 
> > @depends('--enable-fuzzing', try_compile(..., when='--enable-fuzzing'))
> > def enable_libfuzzer(fuzzing, afl):
> >     if fuzzing and not afl:
> >         return True
> 
> 
> I see why you wouldn't need the intermediate function "have_afl_compiler",
> but I would still need the "enable_fuzzing" function for the
> set_config/set_define on FUZZING, right? Did you mark the wrong function
> here and meant to say that I can get rid of "have_afl_compiler" instead?
> Thanks!

I just missed the part where FUZZING needed it.

Comment 20

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #16)
> Comment on attachment 8799811 [details] [diff] [review]
> fuzzing-interface-build-v3.patch
> 
> Review of attachment 8799811 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/moz.configure
> @@ +765,5 @@
> >  set_config('SKIA_INCLUDES', skia_includes)
> >  
> >  # Support various fuzzing options
> >  # ==============================================================
> > +@depends(try_compile(body='__AFL_COMPILER;', check_msg='for AFL compiler'))
> 
> This would break --disable-compile-environment builds until bug 1296530.
> Until then you need something like:
> 
> include_when('fuzzing.configure', '--enable-compile-environment')
> 
> And move this to toolkit/fuzzing.configure

Bug 1296530 landed, so you can now do:
with only_when('--enable-compile-environment'):
    your_stuff

instead of wrapping in a separate file.

Comment 21

[Christian Holler (:decoder)]

Attachment: fuzzing-interface-build-v4.patch

New build system patch, as discussed in previous comments and on IRC.

Comment 22

[Christian Holler (:decoder)]

Attachment: fuzzing-interface-v5.patch

Patch with review comments addressed, as discussed on IRC.

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8802342 [details] [diff] [review]
fuzzing-interface-v5.patch

Review of attachment 8802342 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just two minor things.  r=me with those addressed.

::: tools/fuzzing/interface/FuzzingInterface.cpp
@@ +13,5 @@
> +
> +using namespace mozilla;
> +
> +#ifdef __AFL_COMPILER
> +void mozilla::afl_interface_stream(const char* testFile, FuzzingTestFuncStream testFunc) {

You shouldn't need the mozilla:: prefix here, due to the |using namespace mozilla| above.

@@ +38,5 @@
> +      testFunc(inputStream.forget());
> +    }
> +}
> +
> +void mozilla::afl_interface_raw(const char* testFile, FuzzingTestFuncRaw testFunc) {

Same thing here.

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8802111 [details] [diff] [review]
fuzzing-interface-build-v4.patch

Review of attachment 8802111 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/toolkit.mozbuild
@@ +193,5 @@
>                  '/media/mtransport/test',
>              ]
>  
> +if CONFIG['FUZZING']:
> +  DIRS += ['/tools/fuzzing']

4 spaces for indentation.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60a927b29635
Build system changes for unified fuzzing interface. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/f94ee24c51bd
Add unified fuzzing interface. r=froydnj

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60a927b29635
https://hg.mozilla.org/mozilla-central/rev/f94ee24c51bd