Closed Bug 1289194 Opened 3 years ago Closed 3 years ago

Add LibFuzzer support for testing xul code

Categories

(Core :: General, defect)

Unspecified
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: decoder, Assigned: decoder)

References

Details

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

Attachments

(4 files, 4 obsolete files)

LibFuzzer [1] is an in-process coverage guided fuzzing tool that has to be linked to the library containing the code to be tested. Google makes extensive use of this fuzzer in Chrome for fuzzing APIs and we should do the same. This is important for our stability and security initiative.

I managed to come up with a design that allows us to hook LibFuzzer into our build system and use it to test all sorts of code in xul but it requires some changes to code and build configs:

1) We need a directory where we can checkout the libfuzzer code to have it built with the rest of our code by our build system. Right now, my patch uses fuzzing/libfuzzer, but we could move this to testing/ if we add a moz.build there.

2) We need to go through all of the initialization steps for Firefox and XUL just like we do for gtests. I also had to write a custom harness like gtests has one, to provide a scoped XPCOM. Once we're through all of this, we need to callback to libfuzzer.

3) We can't link libfuzzer to libxul itself, I'm getting linker errors because it uses internal asan functions for coverage etc. The only way that worked for me is linking it to a binary (e.g the firefox binary). This makes things a little more complicated because we need to callback from xul code to libfuzzer which is linked to the firefox binary, but that works fine by storing the callback somewhere.

4) We need to provide some sort of Registry where subsystems can register testing functions. LibFuzzer is meant for testing one single function but instead, we want to be able to choose from many functions which we want to test. I've implemented this in a subdirectory of fuzzing/libfuzzer, side by side with the harness for running.

5) We need a build switch (e.g. --enable-libfuzzer) to enable all of this code.

There is also a design diagram available, which doesn't cover yet the registry aspect yet though. But it shows quite well where and how things are linked and called: https://users.own-hero.net/~decoder/LinkerDiagram-v2.png


I will attach some patches shortly for initial feedback.



[1] http://llvm.org/docs/LibFuzzer.html
Attached patch libfuzzer-integration.patch (obsolete) — Splinter Review
Experimental integration patch. This patch allows us to test xul code with the most recent LibFuzzer version. I'll start by asking ted and glandium for review/feedback because I don't know who else to ask who is familiar with most of this code.

I will also shortly upload a second patch that shows how to use this one with an example from the image/ subsystem (using libFuzzer to test the image decoders).

Please let me know your thoughts and steps necessary to land this.
Attachment #8774752 - Flags: review?(ted)
Attachment #8774752 - Flags: feedback?(mh+mozilla)
This is the example how to use libFuzzer in our codebase. It adds some code copied from a gtest in the image/ subsystem to test the image decoders.

This code isn't supposed to land right now, it rather serves as an example for how to use libFuzzer. But eventually, we will want to land this and other APIs for libFuzzer to test.
Attached patch libfuzzer-cflags-filter.patch (obsolete) — Splinter Review
With Clang 3.9+ the previous patches fail to build because we're building libFuzzer with ASan. According to the manual that's not supported and it causes Clang to crash. This small patch filters out all sanitizer-related flags (e.g. both -fsanitize=address and -fsanitize-coverage=edge).

We need to land this patch after the initial libFuzzer integration patch has landed.
Attachment #8775528 - Flags: review?(ted)
Comment on attachment 8775528 [details] [diff] [review]
libfuzzer-cflags-filter.patch

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

Adding a new Makefile makes me sad, but we don't really have a better way here.
Attachment #8775528 - Flags: review?(ted) → review+
Comment on attachment 8774752 [details] [diff] [review]
libfuzzer-integration.patch

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

::: browser/app/moz.build
@@ +33,5 @@
>      'mozglue',
>  ]
>  
> +if CONFIG['LIBFUZZER']:
> +  USE_LIBS += [ 'fuzzer', 'xul' ]

gulp. I'm not sure this is desirable.

::: build/moz.configure/init.configure
@@ +804,5 @@
>      return None
>  
>  include(include_project_configure)
> +
> +include('fuzzing.configure')

This looks gecko-centric. This should go in toolkit/toolkit.configure... and doesn't need to be in a separate file, actually.

::: fuzzing/libfuzzer/harness/LibFuzzerRunner.cpp
@@ +12,5 @@
> +#endif
> +
> +namespace mozilla {
> +
> +// We use a static var 'RunGTest' defined in nsAppRunner.cpp.

GTest? ;)

::: fuzzing/libfuzzer/moz.build
@@ +9,5 @@
> +DIRS += [
> +  'harness',
> +]
> +
> +SOURCES += [

UNIFIED_SOURCES maybe?

::: fuzzing/moz.build
@@ +5,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +if CONFIG['LIBFUZZER']:
> +  DIRS += [
> +    'libfuzzer',

We generally avoid moz.builds that only add DIRS.

::: moz.build
@@ +53,5 @@
>  DIRS += [
>      'config/external/fdlibm',
>      'config/external/nspr',
>      'config/external/zlib',
> +    'fuzzing',

Seems to me this should go in some subdirectory. Under tools/ would seem like a right place.

::: toolkit/xre/nsAppRunner.cpp
@@ +3785,5 @@
>  #endif /* MOZ_WIDGET_GTK */
>  
> +#ifdef LIBFUZZER
> +  if (PR_GetEnv("LIBFUZZER"))
> +    return mozilla::libFuzzerRunner->Run();

You have access to argc and argv in this function. Why do you need setLibFuzzerMain?
Attachment #8774752 - Flags: feedback?(mh+mozilla) → feedback-
(In reply to Mike Hommey [:glandium] from comment #5)
> Comment on attachment 8774752 [details] [diff] [review]
> libfuzzer-integration.patch
> 
> Review of attachment 8774752 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/app/moz.build
> @@ +33,5 @@
> >      'mozglue',
> >  ]
> >  
> > +if CONFIG['LIBFUZZER']:
> > +  USE_LIBS += [ 'fuzzer', 'xul' ]
> 
> gulp. I'm not sure this is desirable.

It was the only way for me to get the build working. We can try to change this if you can help me with that. One problem I remember was that I need to call mozilla::setLibFuzzerMain (which is defined in libxul where the libFuzzerRunner static is defined) and some functions of mozilla::LibFuzzerRegistry (which are defined in libxul because xul code needs to call it as well).

That said, this configuration actually just works. 

> 
> ::: build/moz.configure/init.configure
> @@ +804,5 @@
> >      return None
> >  
> >  include(include_project_configure)
> > +
> > +include('fuzzing.configure')
> 
> This looks gecko-centric. This should go in toolkit/toolkit.configure... and
> doesn't need to be in a separate file, actually.

I can move this to toolkit, but I picked a separate file because in the future, we might add more fuzzing specific options, not just this one.

> 
> ::: fuzzing/libfuzzer/harness/LibFuzzerRunner.cpp
> @@ +12,5 @@
> > +#endif
> > +
> > +namespace mozilla {
> > +
> > +// We use a static var 'RunGTest' defined in nsAppRunner.cpp.
> 
> GTest? ;)


I'll fix that comment. I copied this from the GTest runner because we need the same kind of mechanism with some tweaks.

> 
> ::: fuzzing/libfuzzer/moz.build
> @@ +9,5 @@
> > +DIRS += [
> > +  'harness',
> > +]
> > +
> > +SOURCES += [
> 
> UNIFIED_SOURCES maybe?

I can try using that if it builds.

> 
> ::: fuzzing/moz.build
> @@ +5,5 @@
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +if CONFIG['LIBFUZZER']:
> > +  DIRS += [
> > +    'libfuzzer',
> 
> We generally avoid moz.builds that only add DIRS.

Again, the choice here was made because we might add more directories for fuzzing in the future, not just one.

> 
> ::: moz.build
> @@ +53,5 @@
> >  DIRS += [
> >      'config/external/fdlibm',
> >      'config/external/nspr',
> >      'config/external/zlib',
> > +    'fuzzing',
> 
> Seems to me this should go in some subdirectory. Under tools/ would seem
> like a right place.

Will move it there then.

> 
> ::: toolkit/xre/nsAppRunner.cpp
> @@ +3785,5 @@
> >  #endif /* MOZ_WIDGET_GTK */
> >  
> > +#ifdef LIBFUZZER
> > +  if (PR_GetEnv("LIBFUZZER"))
> > +    return mozilla::libFuzzerRunner->Run();
> 
> You have access to argc and argv in this function. Why do you need
> setLibFuzzerMain?

I don't need it because of argc and argv, I need it because I cannot call back into libFuzzer except with a callback. The function to call is not in libxul, it's in libfuzzer, but I cannot call it in nsBrowserApp because we need the initialization in nsAppRunner first. And unlike the gtest stuff, I cannot put the libfuzzer code into libxul because I get problems/relocation errors with ASan internal functions that libfuzzer uses.
Flags: needinfo?(mh+mozilla)
ISTR you said you got things working without the libxul dependency in the firefox binary, can you attach your current patch?
Flags: needinfo?(mh+mozilla) → needinfo?(choller)
Attached patch libfuzzer-integration-v2.patch (obsolete) — Splinter Review
Here's the new patch. Changes I made:

* Moved fuzzing/ to tools/fuzzing
* Moved the configure stuff into toolkit/moz.configure
* Fixed comment in LibFuzzerRunner
* Integrated the Makefile.in hack that filters ASan flags when building libfuzzer. ted already reviewed this part.
* Made definitions of setLibFuzzerMain and some functions in LibFuzzerRegistry weak. This allows linking without explicitly linking to xul. Functions are still resolved properly.

I couldn't find any macro for marking symbols as weak, is there any in our codebase?

I still kept the fuzzing/libfuzzer directory structure because I think we might add more to this directory in the near future. Let me know if this isn't ok.
Attachment #8774752 - Attachment is obsolete: true
Attachment #8775528 - Attachment is obsolete: true
Attachment #8774752 - Flags: review?(ted)
Flags: needinfo?(choller) → needinfo?(mh+mozilla)
Attachment #8779481 - Flags: feedback?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
Comment on attachment 8779481 [details] [diff] [review]
libfuzzer-integration-v2.patch

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

::: browser/app/moz.build
@@ +33,5 @@
>      'mozglue',
>  ]
>  
> +if CONFIG['LIBFUZZER']:
> +  USE_LIBS += [ 'fuzzer' ]

You may or may not want this in mozglue instead. In which case you can set FINAL_TARGET = 'mozglue' in the libfuzzer moz.build file instead. This would make no difference on Linux, because mozglue is linked inside the program executable, but it would on OSX and Windows where it's a separate shared library that the executable is linked against. I don't know if this makes a difference for you.

::: browser/app/nsBrowserApp.cpp
@@ +67,5 @@
> +#ifdef LIBFUZZER
> +int libfuzzer_main(int argc, char **argv);
> +namespace mozilla {
> +  typedef int(*LibFuzzerMain)(int, char**);
> +  extern __attribute__ ((weak)) MOZ_EXPORT void setLibFuzzerMain(int argc, char** argv, LibFuzzerMain main);

You don't want a weak symbol here. You want to add a XRE_API function. Check out nsXULAppAPI.h and the kXULFuncs table in this file.

::: tools/fuzzing/libfuzzer/harness/LibFuzzerRegistry.h
@@ +18,5 @@
> +typedef std::pair<LibFuzzerInitFunc, LibFuzzerTestingFunc> LibFuzzerFunctions;
> +
> +class LibFuzzerRegistry {
> +    public:
> +        MOZ_EXPORT __attribute__ ((weak)) static LibFuzzerRegistry& getInstance();

Why does this need to be weak? You're not using this from outside libfuzzer, are you?

@@ +20,5 @@
> +class LibFuzzerRegistry {
> +    public:
> +        MOZ_EXPORT __attribute__ ((weak)) static LibFuzzerRegistry& getInstance();
> +        MOZ_EXPORT void registerModule(std::string moduleName, LibFuzzerInitFunc initFunc, LibFuzzerTestingFunc testingFunc);
> +        MOZ_EXPORT __attribute__ ((weak)) LibFuzzerFunctions getModuleFunctions(std::string& moduleName);

Same here.
Attachment #8779481 - Flags: feedback?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #9)
> Comment on attachment 8779481 [details] [diff] [review]
> libfuzzer-integration-v2.patch
> 
> Review of attachment 8779481 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/app/moz.build
> @@ +33,5 @@
> >      'mozglue',
> >  ]
> >  
> > +if CONFIG['LIBFUZZER']:
> > +  USE_LIBS += [ 'fuzzer' ]
> 
> You may or may not want this in mozglue instead. In which case you can set
> FINAL_TARGET = 'mozglue' in the libfuzzer moz.build file instead. This would
> make no difference on Linux, because mozglue is linked inside the program
> executable, but it would on OSX and Windows where it's a separate shared
> library that the executable is linked against. I don't know if this makes a
> difference for you.

Right now we're only targeting Linux. But I don't know about the future. Linking as a shared library isn't supported by LibFuzzer, so I don't know what would happen. Also, mozglue is also linked to binaries that do not have XUL, like the JS shell, or am I wrong there?

> 
> ::: browser/app/nsBrowserApp.cpp
> @@ +67,5 @@
> > +#ifdef LIBFUZZER
> > +int libfuzzer_main(int argc, char **argv);
> > +namespace mozilla {
> > +  typedef int(*LibFuzzerMain)(int, char**);
> > +  extern __attribute__ ((weak)) MOZ_EXPORT void setLibFuzzerMain(int argc, char** argv, LibFuzzerMain main);
> 
> You don't want a weak symbol here. You want to add a XRE_API function. Check
> out nsXULAppAPI.h and the kXULFuncs table in this file.

I will look into this.

> 
> ::: tools/fuzzing/libfuzzer/harness/LibFuzzerRegistry.h
> @@ +18,5 @@
> > +typedef std::pair<LibFuzzerInitFunc, LibFuzzerTestingFunc> LibFuzzerFunctions;
> > +
> > +class LibFuzzerRegistry {
> > +    public:
> > +        MOZ_EXPORT __attribute__ ((weak)) static LibFuzzerRegistry& getInstance();
> 
> Why does this need to be weak? You're not using this from outside libfuzzer,
> are you?

I only use these functions in LibFuzzer, but they are not in LibFuzzer, they are in libxul. So they need to be weak as well. This code is part of libxul because the respective code that contains the testing function needs to call it to register itself for testing. Shall I expose these as well using this XRE_API magic? Does that work with class methods on a class instance?
Flags: needinfo?(mh+mozilla)
(In reply to Christian Holler (:decoder) from comment #10)
> Right now we're only targeting Linux. But I don't know about the future.
> Linking as a shared library isn't supported by LibFuzzer, so I don't know
> what would happen. Also, mozglue is also linked to binaries that do not have
> XUL, like the JS shell, or am I wrong there?

That's right.

> > ::: tools/fuzzing/libfuzzer/harness/LibFuzzerRegistry.h
> > @@ +18,5 @@
> > > +typedef std::pair<LibFuzzerInitFunc, LibFuzzerTestingFunc> LibFuzzerFunctions;
> > > +
> > > +class LibFuzzerRegistry {
> > > +    public:
> > > +        MOZ_EXPORT __attribute__ ((weak)) static LibFuzzerRegistry& getInstance();
> > 
> > Why does this need to be weak? You're not using this from outside libfuzzer,
> > are you?
> 
> I only use these functions in LibFuzzer, but they are not in LibFuzzer, they
> are in libxul. So they need to be weak as well. This code is part of libxul
> because the respective code that contains the testing function needs to call
> it to register itself for testing. Shall I expose these as well using this
> XRE_API magic?

It would probably be better.

> Does that work with class methods on a class instance?

No. You need C-like functions. OTOH, considering you are exposing getInstance, which returns a singleton, and getModuleFunctions, which returns functions, you only really need one entry point that returns the two functions (and it might be preferable to avoid using std::pair there, and just use 2 out values.
Flags: needinfo?(mh+mozilla)
Attached patch libfuzzer-integration-v3.patch (obsolete) — Splinter Review
New version of the integration patch, now using the XRE API as suggested in the last review comment. I wrote C-style wrappers for the C++ calls and that seems to work just fine using the XRE methods.

I did not change the linking to mozglue because right now, the code LibFuzzer is expecting to call is in XUL as well and mozglue is linked to things that don't have XUL. Maybe we could move that code to mozglue as well (the registry code) so you could in theory use it with the JS shell, but I don't know if that would work (can XUL code call into mozglue code?).

I also tried UNIFIED_SOURCES and got intermittent build failures, so I reverted it back to SOURCES (when I resumed the build with -j1 it worked).

Apart from that, I added some comments for clarification, moved some type declarations into headers to tidy things up and removed some unnecessary declarations from earlier versions.

Let me know what you think :)
Attachment #8779481 - Attachment is obsolete: true
Attachment #8784536 - Flags: review?(mh+mozilla)
As discussed with :ted on IRC, this is a follow-up patch necessary to ensure that gtests still link properly when libfuzzer support is enabled. Since we eventually want to enable this by default in the ASan optimized builds, that is a requirement.

The previous patch uses xpcom/testing/TestHarness.h just like gtests and that breaks because this header can only be used once in the build. As a solution, ted proposed to copy the header and make its content part of an anonymous namespace to avoid collisions while linking.
Attachment #8784538 - Flags: review?(ted)
Comment on attachment 8784538 [details] [diff] [review]
libfuzzer-private-xpcom.patch

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

I would have just stuck the whole file inline in LibFuzzerRunner.cpp, honestly. If anything else winds up including LibFuzzerTestharness.h it shouldn't break, but we'd be including a whole extra copy of the code each time.
Attachment #8784538 - Flags: review?(ted) → review+
(In reply to Christian Holler (:decoder) from comment #12)
> I did not change the linking to mozglue because right now, the code
> LibFuzzer is expecting to call is in XUL as well and mozglue is linked to
> things that don't have XUL. Maybe we could move that code to mozglue as well
> (the registry code) so you could in theory use it with the JS shell, but I
> don't know if that would work (can XUL code call into mozglue code?).

That's the whole point of mozglue :)

> I also tried UNIFIED_SOURCES and got intermittent build failures, so I
> reverted it back to SOURCES (when I resumed the build with -j1 it worked).

That doesn't make sense.
Comment on attachment 8784536 [details] [diff] [review]
libfuzzer-integration-v3.patch

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

::: tools/fuzzing/libfuzzer/FuzzerCustomMain.cpp
@@ +14,5 @@
> +extern LibFuzzerInitFunc libFuzzerGetInitFunc(const char*);
> +extern LibFuzzerTestingFunc libFuzzerGetTestingFunc(const char*);
> +
> +int libfuzzer_main(int argc, char **argv) {
> +  LibFuzzerInitFunc initFunc = libFuzzerGetInitFunc(getenv("LIBFUZZER"));

You might as well call getenv only once.

::: tools/fuzzing/libfuzzer/harness/LibFuzzerRegistry.cpp
@@ +5,5 @@
> +
> +#include "LibFuzzerRegistry.h"
> +
> +extern "C" {
> +    LibFuzzerInitFunc MOZ_EXPORT XRE_LibFuzzerGetInitFunc(const char* moduleName) {

It seems wasteful to have two functions for this when you could just have one:
void XRE_LibFuzzerGetFuncs(const char* moduleName, LibFuzzerInitFunc **init, LibFuzzerTestingFunc **test)

@@ +23,5 @@
> +    static LibFuzzerRegistry instance;
> +    return instance;
> +}
> +
> +void LibFuzzerRegistry::registerModule(std::string moduleName, LibFuzzerInitFunc initFunc, LibFuzzerTestingFunc testingFunc) {

You could also look how xpcom component registration works (see xpcom/components/Module.h)
Attachment #8784536 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #16)
> Comment on attachment 8784536 [details] [diff] [review]
> libfuzzer-integration-v3.patch
> 
> Review of attachment 8784536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/fuzzing/libfuzzer/FuzzerCustomMain.cpp
> @@ +14,5 @@
> > +extern LibFuzzerInitFunc libFuzzerGetInitFunc(const char*);
> > +extern LibFuzzerTestingFunc libFuzzerGetTestingFunc(const char*);
> > +
> > +int libfuzzer_main(int argc, char **argv) {
> > +  LibFuzzerInitFunc initFunc = libFuzzerGetInitFunc(getenv("LIBFUZZER"));
> 
> You might as well call getenv only once.

Will refactor this for the next patch version.


> 
> ::: tools/fuzzing/libfuzzer/harness/LibFuzzerRegistry.cpp
> @@ +5,5 @@
> > +
> > +#include "LibFuzzerRegistry.h"
> > +
> > +extern "C" {
> > +    LibFuzzerInitFunc MOZ_EXPORT XRE_LibFuzzerGetInitFunc(const char* moduleName) {
> 
> It seems wasteful to have two functions for this when you could just have
> one:
> void XRE_LibFuzzerGetFuncs(const char* moduleName, LibFuzzerInitFunc **init,
> LibFuzzerTestingFunc **test)

Same here.

> 
> @@ +23,5 @@
> > +    static LibFuzzerRegistry instance;
> > +    return instance;
> > +}
> > +
> > +void LibFuzzerRegistry::registerModule(std::string moduleName, LibFuzzerInitFunc initFunc, LibFuzzerTestingFunc testingFunc) {
> 
> You could also look how xpcom component registration works (see
> xpcom/components/Module.h)


I do not want to complicate this any further. This part is supposed to be used by all developers and I want to keep this as easy as possible. Also I would like to keep this as independent from our own stuff as possible, because eventually, we might also want to test other (non-xul) code that is still part of our build, the same way.

(In reply to Mike Hommey [:glandium] from comment #15)
> (In reply to Christian Holler (:decoder) from comment #12)
> > I did not change the linking to mozglue because right now, the code
> > LibFuzzer is expecting to call is in XUL as well and mozglue is linked to
> > things that don't have XUL. Maybe we could move that code to mozglue as well
> > (the registry code) so you could in theory use it with the JS shell, but I
> > don't know if that would work (can XUL code call into mozglue code?).
> 
> That's the whole point of mozglue :)

I will keep this in mind for a follow-up bug as soon as we need to test more (like JS Shell support).

> 
> > I also tried UNIFIED_SOURCES and got intermittent build failures, so I
> > reverted it back to SOURCES (when I resumed the build with -j1 it worked).
> 
> That doesn't make sense.

But that's what happened. I will retry once more, but I would not want to block on this. In general I am not a big fan of UNIFIED_SOURCES and we are building third party source code here that is not supposed to be built that way. This could eventually result in build failures that require us to patch this code and I want to avoid that at all cost.
(In reply to Christian Holler (:decoder) from comment #17)
> > You could also look how xpcom component registration works (see
> > xpcom/components/Module.h)
> 
> 
> I do not want to complicate this any further. This part is supposed to be
> used by all developers and I want to keep this as easy as possible. Also I
> would like to keep this as independent from our own stuff as possible,
> because eventually, we might also want to test other (non-xul) code that is
> still part of our build, the same way.

I didn't imply you should use xpcom component registration, but you could setup something similar. Which would then only require, for developer, to declare global variables in the different modules, instead of a static initializer that calls a registration function.
(In reply to Mike Hommey [:glandium] from comment #18)
> 
> I didn't imply you should use xpcom component registration, but you could
> setup something similar. Which would then only require, for developer, to
> declare global variables in the different modules, instead of a static
> initializer that calls a registration function.

Ah thanks for the suggestion then! That's certainly an improvement I want to keep in mind, maybe also in a follow-up bug so we can get this one landed first.
I merged the two getters for init and testing function into one function as suggested by you. Since the typedefs are already pointers, no need for ** in the signatures, but apart from that it's exactly as you suggested.

With that change, there's only one call to getenv necessary in the libfuzzer main, so that problem is solved with that refactoring as well.

I tried UNIFIED_SOURCES once more and the code doesn't build with unification enabled. Some declarations inside libfuzzer for some ASan internal functions collide with some external ASan header.
Attachment #8784536 - Attachment is obsolete: true
Attachment #8787258 - Flags: review?(mh+mozilla)
Attachment #8787258 - Flags: review?(mh+mozilla) → review+
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95e68b473e91
Experimental LibFuzzer integration. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/13a770064f3e
Make LibFuzzerRunner use its own private ScopedXPCOM copy. r=ted
https://hg.mozilla.org/mozilla-central/rev/95e68b473e91
https://hg.mozilla.org/mozilla-central/rev/13a770064f3e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/027ead0b3b0b
Backed out changeset 13a770064f3e 
https://hg.mozilla.org/mozilla-central/rev/33e7ae9b3104
Backed out changeset 95e68b473e91 for failure to process moz.build file. r=backout a=backout
Backed out for failure to process moz.build file:

https://hg.mozilla.org/mozilla-central/rev/33e7ae9b3104e90ce56bbde1906efa97fb116449
https://hg.mozilla.org/mozilla-central/rev/027ead0b3b0bed97d7ec7fd01ee79c7e01848047

First failure: https://treeherder.mozilla.org/logviewer.html#?job_id=35444646&repo=mozilla-inbound

==============================
ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file:

    /home/worker/workspace/sm-package/mozjs-51.0a1.0/moz.build

The underlying problem is we referenced a path that does not exist. That path is:

    /home/worker/workspace/sm-package/mozjs-51.0a1.0/tools/fuzzing/moz.build

Either create the file if it needs to exist or do not reference it.

Traceback (most recent call last):
  File "./devtools/automation/autospider.py", line 265, in <module>
    run_command(['sh', '-c', posixpath.join(PDIR.js_src, 'configure') + ' ' + CONFIGURE_ARGS], check=True)
  File "./devtools/automation/autospider.py", line 241, in run_command
    raise subprocess.CalledProcessError(status, command, output=stderr)
subprocess.CalledProcessError: Command '['sh', '-c', u'/home/worker/workspace/sm-package/mozjs-51.0a1.0/js/src/configure --enable-optimize --enable-nspr-build --prefix=/home/worker/workspace/sm-package/mozjs-51.0a1.0/obj-spider/dist']' returned non-zero exit status 1
Status: RESOLVED → REOPENED
Flags: needinfo?(choller)
Resolution: FIXED → ---
Sounds like we may want to update the rules for SM-tc(pkg) builds a bit too (since this didn't show up on the initial push that broke it, just the first push afterwards that touched js/src). I filed bug 1301122 to track that.
I assume this breaks because we don't have a full tree when building the JS shell?

Is the right way here something like this in the toplevel moz.build?

> if not CONFIG['JS_STANDALONE']:
>    DIRS += ['tools/fuzzing']
Flags: needinfo?(choller) → needinfo?(ted)
You should probably just put that in toolkit.mozbuild:
https://dxr.mozilla.org/mozilla-central/source/toolkit/toolkit.mozbuild
Flags: needinfo?(ted)
Simple fix as suggested by :ted. This patch moves the reference to the tools/fuzzing directory from toplevel moz.build to toolkit/toolkit.mozbuild. Also it moves the define check to the same directory.

In a follow-up bug, I will also rename the --enable-libfuzzer flag and the according defines to --enable-fuzzing since we are probably going to make a unified interface for both libfuzzer and AFL.
Attachment #8789956 - Flags: review?(ted)
Blocks: 1302451
Comment on attachment 8789956 [details] [diff] [review]
libfuzzer-toolkit.patch

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

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

If libfuzzer is the only thing under /tools/fuzzing it'd be better to just put `/tools/fuzzing/libfuzzer` here and not even have a moz.build in tools/fuzzing.
Attachment #8789956 - Flags: review?(ted) → review+
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/427fea673dd0
Experimental LibFuzzer integration. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b147e7550f0
Make LibFuzzerRunner use its own private ScopedXPCOM copy. r=ted
https://hg.mozilla.org/integration/mozilla-inbound/rev/78c485305259
Move tools/fuzzing/libfuzzer reference to toolkit. r=ted
Whiteboard: [adv-main51-]
You need to log in before you can comment on or make changes to this bug.