[LibFuzzer] Reimplement how the optional user functions are called.

[delcypher]

Summary

[LibFuzzer] Reimplement how the optional user functions are called.

The motivation for this change is to fix linking issues on OSX.
However this only partially fixes linking issues (the uninstrumented
tests and a few others won't succesfully link yet).

This change introduces a struct of function pointers
(`fuzzer::ExternalFuntions`) which when initialised will point to the
optional functions if they are available. Currently these
`LLVMFuzzerInitialize and LLVMFuzzerCustomMutator` functions.

Two implementations of `fuzzer::ExternalFunctions` constructor are
provided one for Linux and one for OSX.

The OSX implementation uses `dlsym()` because the prior implementation
using weak symbols does not work unless the additional flags are passed
to the linker.

The Linux implementation continues to use weak symbols because the
`dlsym()` approach does not work unless additional flags are passed
to the linker.

[kcc]

This is very sad.
Are you sure that there is no way to have this in the form of some attribute in the source code?
I do not expect libFuzzer to be used with the LLVM build system other than when fuzzing parts of LLVM,
and the build command line needs to remain simple:
clang++ -c -g -O2 -std=c++11 Fuzzer/*.cpp -IFuzzer

So, ask around at Apple and find another way that will not require extra command line params. Please.
If there is no way, I want to have this properly documented in a form of llvm bug and an accompanied rdar entry.

[mehdi_amini]

Weak linking is not supported on Darwin AFAIK. Or rather: the symbol has to be present during the static link, but don't have to be present in the dylib when loading the program.
On OS X for instance, when following the Fuzzer doc, I couldn't link with only LLVMFuzzerTestOneInput in my test, I had to implement LLVMFuzzerCustomMutator and LLVMFuzzerInitialize as well, I planned to update the doc on this aspect.

[kcc]

This utterly sucks.

[mehdi_amini]

Instead of using weak link, we could use dlsym from libFuzzer?

[mehdi_amini]

lib/Fuzzer/test/uninstrumented/CMakeLists.txt

21 ↗

(On Diff #58816)

Where is this list coming from? Why is it needed? ....

[kcc]

In D20741#442662, @mehdi_amini wrote:

Instead of using weak link, we could use dlsym from libFuzzer?

Such a change would be reasonable.
Please make a separate change for that if it helps.

(It will solve only one direction, not the other)

[delcypher]

@mehdi_amini

In D20741#442650, @mehdi_amini wrote:

Weak linking is not supported on Darwin AFAIK. Or rather: the symbol has to be present during the static link, but don't have to be present in the dylib when loading the program.

I don't think this is quite true. Weak linking can work under Darwin it's just that you have to tell the compile time linker to behave differently.

https://glandium.org/blog/?p=2764
https://developer.apple.com/library/mac/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/WeakLinking.html#//apple_ref/doc/uid/20002378-106633

So far I have not been able to find an attribute that can be used to tell the Darwin linker to treat weak symbols differently (although you still need to use `__attribute__((weak)) or __attribute__((weak_import))` to tell the compiler to mark the symbol as weak).

So far the ways of handling this I've found are

• Explicitly tell the linker about the symbols that do not to be defined at link time. This is the approach used in this patch by passing -U _theSymbol to the linker
• Pass -undefined dynamic_lookup to the linker. This is really bad because now the linker won't warn/error out on any undefined symbols.

The problem with both of these is a user of LibFuzzer has to do extra things with the linker to make things work.

@mehdi_amini suggestion of using dlsym might be the best way forward as in principle it wouldn't require the user to do anything special with the linker.

[kcc]

Let's try dlsym. Let's create a separate file for that.
BTW, I am about to add one more use of a weak __sanitizer_* function in FuzzerIO.cpp (shouldn't be a problem)

[delcypher]

lib/Fuzzer/test/uninstrumented/CMakeLists.txt

21 ↗

(On Diff #58816)

This test links libFuzzer against an uninstrumented binary. An instrumented binary would normally have all of the above function defined (coming from the ASan and sanitizer coverage runtimes in compiler-rt) which LibFuzzer calls into.

An uninstrumented binary won't have the ASan and sanitizer coverage runtimes linked in so a linking libFuzzer against the uninstrumented would in principle cause a link error because those functions are not declared as weak in compiler-rt. However FuzzerLoop.cpp in LibFuzzer redeclares these symbols as weak to avoid this at issue at link time on Linux.

[delcypher]

@kcc:

Let's try dlsym. Let's create a separate file for that.

Do you also want to use the dlsym implementation on Linux? I'm imagining a struct containing function pointers to all the functions that were previously declared weak. We could have a different implementation for initializing those function pointers for different platforms or we could have OSX and Linux both share the same implementation. What would you prefer?

[kcc]

In D20741#442890, @delcypher wrote:

@kcc:

Let's try dlsym. Let's create a separate file for that.

Do you also want to use the dlsym implementation on Linux? I'm imagining a struct containing function pointers to all the functions that were previously declared weak. We could have a different implementation for initializing those function pointers for different platforms or we could have OSX and Linux both share the same implementation. What would you prefer?

I expect dlsym to work equally well on linux, so, yes, let's make it the same on both.
+1 to struct of function pointers. Declare it in a separate header file, initialize it in a separate .cpp file.
FuzzerDlsym.cpp? FuzzerWeakSymbols.cpp? [naming is hard]

[delcypher]

WORK IN PROGRESS. Uploading for comments.

[kcc]

LG overall, add comments.
For simpler review, I;d actually say that you better finish this one w/o adding sanitizer_* functions and commit.
Then make another change for sanitizer*

lib/Fuzzer/FuzzerInternal.h

473 ↗

(On Diff #59128)

s/might/may or may not/

lib/Fuzzer/FuzzerMain.cpp

25 ↗

(On Diff #59128)

Instead of creating two EF instances it's probably reasonable to move this code inside FuzzerDriver. Please do so.

[kcc]

s/add comments/added comments

[delcypher]

@kcc : I've uploaded a work in progress patch so you can take a look to see if I'm going in the right direction with this. One thing I'm not very happy about is FuzzerMain.cpp. The implementation of main(int argc, char **argv) needs to be able to optionally call LLVMFuzzerInitialize but also the Fuzzer class needs access to bunch of other functions. So currently they both independently create a fuzzer::ExternalFunctions and call Init(). This is wasteful but I wanted to avoid changing the interface of fuzzer::fuzzerDriver and the constructor of Fuzzer which would be required if I wanted to propagate EF through the layers into an instance of the Fuzzer class. Any thoughts on how you would like this handled?

[kcc]

Ah, yea, you need to pass &argc and &argv...
Just change the interface of FuzzerDriver to accept int* and char*** -- it's not a part of public interface any more, easy to change.

[delcypher]

In D20741#444862, @kcc wrote:

Ah, yea, you need to pass &argc and &argv...
Just change the interface of FuzzerDriver to accept int* and char*** -- it's not a part of public interface any more, easy to change.

Ok great. I will go ahead and do that.

[aizatsky]

I solved this in chromium tree by specifying additional linker arguments:

"-Wl,-U,_LLVMFuzzerCustomMutator",
"-Wl,-U,_LLVMFuzzerInitialize",

https://code.google.com/p/chromium/codesearch#chromium/src/testing/libfuzzer/BUILD.gn&l=29

Is there a reason we shouldn't do the same here without changing libfuzzer code?

[mehdi_amini]

Any reason *not* to change libFuzzer to avoid that?
The original intention of the code in libFuzzer (according to the comment) was having them "weak_external". Having to modify the user linker flag is not nice.

[kcc]

Is there a reason we shouldn't do the same here without changing libfuzzer code?

The build command line for libFuzzer should remain very simple and the same across platforms.
It's easy to achieve, so we should just do it.

[delcypher]

Try to use dlsym() rather than passing flags to the linker on OSX. The result is not very satisfying.

[delcypher]

@kcc:

The build command line for libFuzzer should remain very simple and the same across platforms.
It's easy to achieve, so we should just do it.

Okay here's the result. I'm not very happy with. It seems that on Linux to get this to work the linker has to be passed -export-dynamic (otherwise dlsym() can't see the symbols we want to get) and told to link against the dl library for this to work.

A little bit ironically I don't have to tell the Darwin linker to do anything special in this patch. So in my original patch I had to tell the Darwin linker to do extra work and in this patch I have to tell GNU ld to do extra work. So now this patch causes hassle under Linux :(

Perhaps a good middle ground would be to make the Linux implementation of ExternalFunctions::Init() use weak symbols (as a way of getting the addresses to assign to the function pointers) that way no special linking is required on either platform. Doing this though will either require some ifdefs in FuzzerExtFunctions.cpp or require two different versions FuzzerExtFunctionsLinux.cpp and FuzzerExtFunctionsDarwin.cpp.

Thoughts?

[delcypher]

Upload correct patch (Sorry the old one was wrong).

[kcc]

No extra linker flags, please.
No ifdefs, other than for an entire file, please.
I am ok with this approach:
Create FuzzerExtFunctionsWeak.cpp and FuzzerExtFunctionsDlsym.cpp,
have weak functions in the first one and dlsym in the second one.
Have a file scope ifdefs in these files (#if LIBFUZZER_LINUX in the first one, #if LIBFUZZER_APPLE in the second one)

lib/Fuzzer/FuzzerInternal.h

475 ↗

(On Diff #59160)

Does it have to be passed as a parameter?
Why not just this?
ExternalFunctions EF;

lib/Fuzzer/test/FuzzerUnittest.cpp

417 ↗

(On Diff #59160)

I wouldn't worry too much, dlsym is not that slow.
And if you follow my other comment you won't need any of this.

[mehdi_amini]

In D20741#445133, @kcc wrote:

No extra linker flags, please.
No ifdefs, other than for an entire file, please.
I am ok with this approach:
Create FuzzerExtFunctionsWeak.cpp and FuzzerExtFunctionsDlsym.cpp,
have weak functions in the first one and dlsym in the second one.
Have a file scope ifdefs in these files (#if LIBFUZZER_LINUX in the first one, #if LIBFUZZER_APPLE in the second one)

There is nothing specific to Linux or Apple in these file, having such a ifdef does not seem right to me. You can handle the different in the CMake (linking one or the other for instance).

[kcc]

You can handle the different in the CMake (linking one or the other for instance).

No.
The build command line for libFuzzer outside of LLVM build system should remain what it is now

[mehdi_amini]

Sure, but ifdef linux and ifdef Apple are still not correct, so back to the whiteboard...

[kcc]

In D20741#445154, @mehdi_amini wrote:

Sure, but ifdef linux and ifdef Apple are still not correct,

Maybe not.
Although: weak is not properly supported on Mac, and dlsym does not work easy enough for linux in this case.
So maybe yes.

so back to the whiteboard...

No objections :)

[mehdi_amini]

Talked to Nick about the linker on Darwin, and he's confirming that we shouldn't rely on weak external, or even on weak override as it is too easy to get wrong.
He's suggesting that the most reliable way is to handle this at the API level, i.e. something like:

int MyTestInput(const uint8_t *data, size_t size);

struct FuzzerAPI {
  int (*TestOneInput)(const uint8_t *data, size_t size);
//  ... other overridable APIs
};
extern "C" void LLVMFuzzerRegistration(FuzzerAPI *API) {
  API->TestOneInput = MyTestInput;
}

That adds a few lines of boilerplate on the client side...

[kcc]

No. The user interface needs to remain as is.

[delcypher]

@kcc @mehdi_amini

so back to the whiteboard...

No objections :)

So am I going with @kcc 's proposal?

I think it's probably fine for now but I don't think the goal of having a really simple compilation command line for LibFuzzer is sustainable long term (especially if someone decides to add support for Windows). It may be necessary in the future to divorce LibFuzzer from the LLVM build system and give LibFuzzer its own build system to do the work of compiling for different platforms (instead of having a simple command line to build the library).

lib/Fuzzer/FuzzerInternal.h

475 ↗

(On Diff #59160)

That is also fine. As a rule of thumb I try to avoid passing structs by value when possible but if you would rather it was passed by value I can do that.

lib/Fuzzer/test/FuzzerUnittest.cpp

417 ↗

(On Diff #59160)

Which comment are you referring to? Changing to passing the struct by value won't change having to call ExternalFunctions::Init(). I can drop this comment if you'd like though.

[kcc]

I think it's probably fine for now but I don't think the goal of having a really simple compilation command line for LibFuzzer is sustainable long term

I am not convinced. Worked well so far. We have a very similar code structure for the sanitizers, btw,
and they, at least asan, are ported to more platforms than the rest of LLVM is.

[delcypher]

In D20741#445216, @kcc wrote:

I think it's probably fine for now but I don't think the goal of having a really simple compilation command line for LibFuzzer is sustainable long term

I am not convinced. Worked well so far. We have a very similar code structure for the sanitizers, btw,
and they, at least asan, are ported to more platforms than the rest of LLVM is.

But aren't the sanitizers a little different? They are part of compiler-rt which AFAIK is always built using clang (or maybe gcc?). LibFuzzer however could conceivably be built with MSVC (which is a very different beast) one day. Of course the code that needs to be instrumented has to be built with clang so my argument here is a little weak.

[mehdi_amini]

In D20741#445216, @kcc wrote:

I think it's probably fine for now but I don't think the goal of having a really simple compilation command line for LibFuzzer is sustainable long term

I am not convinced. Worked well so far.

Well actually I'm not sure it did work that well: nobody could follow the doc and build on OSX.

We have a very similar code structure for the sanitizers, btw,
and they, at least asan, are ported to more platforms than the rest of LLVM is.

Any other precedent on relying on the user to provide/override some APIs in any sanitizer?

[zaks.anna]

In D20741#445224, @mehdi_amini wrote:

In D20741#445216, @kcc wrote:

I think it's probably fine for now but I don't think the goal of having a really simple compilation command line for LibFuzzer is sustainable long term

I am not convinced. Worked well so far.

Well actually I'm not sure it did work that well: nobody could follow the doc and build on OSX.

LibFuzzer is not supported on OS X, so there was no expectation that following the instructions would work.

Simple user interface (command line and API) should be the highest priority here. I believe using dlsym on the Mac and weak linking on Linux is the best option. (Sanitizers implement platform specific behavior as well.)

[mehdi_amini]

In D20741#445249, @zaks.anna wrote:

In D20741#445224, @mehdi_amini wrote:

In D20741#445216, @kcc wrote:

I think it's probably fine for now but I don't think the goal of having a really simple compilation command line for LibFuzzer is sustainable long term

I am not convinced. Worked well so far.

Well actually I'm not sure it did work that well: nobody could follow the doc and build on OSX.

LibFuzzer is not supported on OS X, so there was no expectation that following the instructions would work.

The doc only mentions Windows as unsupported because "The sanitizer coverage support does not work on Windows either as of 01/2015" (see http://llvm.org/pre-releases/3.8.0/rc2/docs/LibFuzzer.html#q-what-about-windows-then-the-fuzzer-contains-code-that-does-not-build-on-windows).
So it seem to me to be a reasonable expectation to have it build on most platform that can build LLVM.

[delcypher]

Use different implementation for initializing fuzzer::ExternalFunctions on Linux and OSX

[mehdi_amini]

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

15 ↗

(On Diff #59173)

Again, there is nothing Apple specific here AFAIK, please rename.

[delcypher]

@kcc : This is my attempt at implementing what you wanted. Please review when you have time.

[delcypher]

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

15 ↗

(On Diff #59173)

The comments above explain why this is the implementation we want to use on Apple platforms. Although this code compiles on Linux it does not work as intended unless an extra flag is passed to the linker by the client when linking against LibFuzzer. If we assume for the moment that we want LibFuzzer to be used with a platform's default linking behavior then this implementation is specific to the behavior of the Darwin linker (and possibly other non GNU linkers). If later it turns out we also need to use this implementation for other platforms (e.g. the *BSDs) then the #ifdef can be appropriately modified in the future.

If I had it my way I wouldn't use #ifdefs and would tell CMake to compile the appropriate file based on the host platform but that option is not available to me.

[zaks.anna]

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

15 ↗

(On Diff #59173)

This code is currently only executed on "Apple" platforms so, in IMHO, spelling out which platform this code will be running on is better for readability than using some abstract name.

On the other hand, should we use the names consistent with what is declared in lib/sanitizer_common/sanitizer_platform.h? Ex: LIBFUZZER_MAC instead if LIBFUZZER_APPLE... LIBFUZZER_MAC is a horrible name (for including OSX and iOS), but consistency might be more important.

[mehdi_amini]

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

15 ↗

(On Diff #59173)

None of what you're writing here is justifying the naming IMO, so again it should be something like:

#ifdef LIBFUZZER_USE_DLOPEN
...
#endif

I'm more flexible on how you get the default, but I'd rather write it in a way that can be overridden through a build setting, i.e. something along these lines (haven't checked the syntax):

#ifndef LIBFUZZER_USE_DLOPEN
  #if LIBFUZZER_LINUX
    #define LIBFUZZER_USE_WEAKLINK
  #else
    #define LIBFUZZER_USE_DLOPEN
  #endif
#else
  #ifdef LIBFUZZER_USE_WEAKLINK
    #error "Can't set both LIBFUZZER_USE_DLOPEN and LIBFUZZER_USE_WEAKLINK"
  #endif
#endif

[mehdi_amini]

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

15 ↗

(On Diff #59173)

(I was replying to delcypher and not Anna as the email thread suggests).

[zaks.anna]

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

15 ↗

(On Diff #59173)

I find this much less readable.

[mehdi_amini]

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

15 ↗

(On Diff #59173)

You're saying that

#ifdef LIBFUZZER_USE_DLOPEN
...
#endif

is less readable than

#ifdef LIBFUZZER_APPLE
...
#endif

I'm missing your point here.

[kcc]

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

15 ↗

(On Diff #59173)

Either is fine for me here.
directly using #ifdef *_APPLE and *_LINUX is shorter.
Using a proxy macros *_USE_* shows the intent more clear but adds clatter.

Decide between yourselves. :)

[kcc]

lib/Fuzzer/FuzzerDriver.cpp

422 ↗

(On Diff #59173)

move all of this into the other FuzzerDriver.
This one is an atavism of my poor design choice in the past, let's not resurrect it.

lib/Fuzzer/FuzzerInternal.h

475 ↗

(On Diff #59173)

I did not suggest to pass it by value.
Can't it be created inside Fuzzer::Fuzzer with a default CTOR?

[zaks.anna]

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

15 ↗

(On Diff #59173)

You're saying that
#ifdef LIBFUZZER_USE_DLOPEN
...
#endif
is less readable than
#ifdef LIBFUZZER_APPLE
...
#endif
I'm missing your point here.

Yes, it would be more difficult to understand if code is executed on the given platform or not as you are introducing a level of indirection.

I'd rather write it in a way that can be overridden through a build setting,

Overriding a build setting would add yet another level of indirection. Debugging this won't be fun especially given all the different ways in which this could be built by various parties.

Unless there is a real reason we want to override it, I'd rather keep things simple.

[mehdi_amini]

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

15 ↗

(On Diff #59173)

I disagree with all of what you wrote above Anna.
But at the same time I don't work on sanitizers usually so I won't weight any more on this.

[delcypher]

lib/Fuzzer/FuzzerDriver.cpp

422 ↗

(On Diff #59173)

A problem with your suggestion is it makes calling the LLVMFuzzerInitialize(int *argc, char ***argv) function difficult because we don't have the argv anymore (inside static int FuzzerDriver(const std::vector<std::string> &Args, UserCallback Callback, fuzzer::ExternalFunctions EF) ). We can emulate argc using Args.size() but argv is more of pain because we have std::vector<std::string> not std::vector<const char*>.

Also if the intention of LLVMFuzzerInitialize(int *argc, char ***argv) is to allow the command line arguments to be changed then what you're proposing doesn't work because the user needs to make the modifications before we create the std::vector<std::string> not after we create it.

lib/Fuzzer/FuzzerInternal.h

475 ↗

(On Diff #59173)

We can create an instance of ExternalFunctions inside the Fuzzer class in the default constructor but then we have the problem of also needing the call LLVMFuzzerInitialize which requires an instance of ExternalFunctions and that has to be done before we create the Fuzzer class.

So either we need to make two instances of ExternalFunctions and initialize both of them (one inside the Fuzzer class and one outside) OR a single instance of ExternalFunctions which we initialize and pass to the Fuzzer class.

I can do either. Decide which you'd prefer and I will implement that.

[kcc]

lib/Fuzzer/FuzzerDriver.cpp

422 ↗

(On Diff #59173)

Can you simply merge these two functions leaving just this one?
FuzzerDriver(int *argc, char ***argv, UserCallback Callback);
using std::vector there is unnecessary.

lib/Fuzzer/FuzzerExtFunctions.h

26 ↗

(On Diff #59173)

We still have a duplication between here and FuzzerExtFunction.def
Can you use a bit more macro tricks to eliminate FuzzerExtFunction.def?

lib/Fuzzer/FuzzerInternal.h

475 ↗

(On Diff #59173)

I'd prefer to have two instances of EF. That's a tiny bit slower at startup but leaves the code simpler.

[delcypher]

• Remove static int FuzzerDriver(const std::vector<std::string> &Args, UserCallback Callback).
• Initialize a new ExternalFunctions inside the Fuzzer class.
• Rename INIT_FUNC macro to EXT_FUNC to better reflect how its being used.
• Use INIT_FUNC macro to add function pointer declaration to the ExternalFunctions macro to reduce redundancy.

[delcypher]

lib/Fuzzer/FuzzerExtFunctions.h

27 ↗

(On Diff #59323)

I've done this. One place where there is technically still duplication is in FuzzerExtFunctionsWeak.cpp with the declaration of the weak functions but I'd rather leave that how it is currently because I don't want too much macro magic.

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

16 ↗

(On Diff #59323)

In interest of a simple implementation I'm sticking with the LIBFUZZER_APPLE and LIBFUZZER_LINUX for now. We can revisit this in the future if it really becomes a problem.

[kcc]

we are converging! :)

lib/Fuzzer/FuzzerExtFunctions.def

19 ↗

(On Diff #59323)

s/ ,false/, false

lib/Fuzzer/FuzzerExtFunctions.h

22 ↗

(On Diff #59323)

Why not ExternalFunctions::ExternalFunctions?

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

48 ↗

(On Diff #59323)

} // namespace fuzzer

lib/Fuzzer/FuzzerExtFunctionsWeak.cpp

21 ↗

(On Diff #59323)

can you also use FuzzerExtFunctions.def here?

lib/Fuzzer/FuzzerMain.cpp

12 ↗

(On Diff #59323)

do you still need this?

lib/Fuzzer/test/FuzzerUnittest.cpp

4 ↗

(On Diff #59323)

do you still need this?

[delcypher]

lib/Fuzzer/FuzzerExtFunctions.h

22 ↗

(On Diff #59323)

My original thinking here is that you might not want to initialize immediately after allocating memory for an instance of ExternalFunctions. My worry is that if ExternalFunctions was ever created in some early start up code (i.e. before main() is called) then we might not want to call dlsym() at this time.

This is quite a narrow use case (that's not even required right now) so perhaps it would be better to just use a normal class initializer?

lib/Fuzzer/FuzzerExtFunctionsWeak.cpp

21 ↗

(On Diff #59323)

Doing so would reduce clarity but I can if you want.

lib/Fuzzer/FuzzerMain.cpp

12 ↗

(On Diff #59323)

Nope. Good catch.

lib/Fuzzer/test/FuzzerUnittest.cpp

4 ↗

(On Diff #59323)

Nope. Good catch.

[delcypher]

• Add closing namespace comments
• Fix minor white space issue
• Drop unused includes

[delcypher]

lib/Fuzzer/FuzzerExtFunctions.h

23 ↗

(On Diff #59334)

@kcc This (not using ExternalFunctions::ExternalFunctions) is the only issue you raised that I haven't tackled. What would you like to do here?

[kcc]

lib/Fuzzer/FuzzerExtFunctions.def

19 ↗

(On Diff #59334)

80 chars per line

lib/Fuzzer/FuzzerExtFunctions.h

23 ↗

(On Diff #59334)

Use a CTOR

[delcypher]

• Remove 80 column violation in FuzzerExtFunctions.def
• Use CTOR for ExternalFunctions rather than explicit Init() function.

[kcc]

LGTM with 3 nits.
Ok to commit once you fix them (assuming all tests still work on Linux)
Thanks!

lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

1 ↗

(On Diff #59335)

Fix the file name in the comment

lib/Fuzzer/FuzzerExtFunctionsWeak.cpp

1 ↗

(On Diff #59335)

fix file name

lib/Fuzzer/FuzzerLoop.cpp

150 ↗

(On Diff #59335)

Why do you need this now?

[delcypher]

• Fix incorrect filenames in comments
• Remove unnecessary initialization in initializer list.

[delcypher]

@kcc

In D20741#446582, @kcc wrote:

LGTM with 3 nits.
Ok to commit once you fix them (assuming all tests still work on Linux)
Thanks!

Nits fixed. I've rebased and the tests have broken under Linux. Looks like I need to upgrade to newer compiler-rt due to the introduction of a dependence on __sanitizer_print_memory_profile (r271465). I'm upgrading now and if tests pass after building a newer clang with the latest compiler-rt then I'll commit this.

[kcc]

Looks like I need to upgrade to newer compiler-rt

You always should.

llvm/trunk/lib/Fuzzer/CMakeLists.txt

 set(LIBFUZZER_FLAGS_BASE "${CMAKE_CXX_FLAGS}")
 # Disable the coverage and sanitizer instrumentation for the fuzzer itself.
 set(CMAKE_CXX_FLAGS "${LIBFUZZER_FLAGS_BASE} -fno-sanitize=all -fno-sanitize-coverage=edge,trace-cmp,indirect-calls,8bit-counters -Werror")
 if( LLVM_USE_SANITIZE_COVERAGE )
   if(NOT "${LLVM_USE_SANITIZER}" STREQUAL "Address")
     message(FATAL_ERROR
       "LibFuzzer and its tests require LLVM_USE_SANITIZER=Address and "
       "LLVM_USE_SANITIZE_COVERAGE=YES to be set."
       )
   endif()
   add_library(LLVMFuzzerNoMainObjects OBJECT
     FuzzerCrossOver.cpp
     FuzzerTraceState.cpp
     FuzzerDriver.cpp
+    FuzzerExtFunctionsDlsym.cpp
+    FuzzerExtFunctionsWeak.cpp
     FuzzerIO.cpp
     FuzzerLoop.cpp
     FuzzerMutate.cpp
     FuzzerSHA1.cpp
     FuzzerTracePC.cpp
     FuzzerUtil.cpp
     )
   add_library(LLVMFuzzerNoMain STATIC

llvm/trunk/lib/Fuzzer/FuzzerDriver.cpp

 static bool AllInputsAreFiles() {
   if (Inputs->empty()) return false;
   for (auto &Path : *Inputs)
     if (!IsFile(Path))
       return false;
   return true;
 }
 
-static int FuzzerDriver(const std::vector<std::string> &Args,
-                        UserCallback Callback) {
+int FuzzerDriver(int *argc, char ***argv, UserCallback Callback) {
   using namespace fuzzer;
+  assert(argc && argv && "Argument pointers cannot be nullptr");
+  fuzzer::ExternalFunctions EF;
+  if (EF.LLVMFuzzerInitialize)
+    EF.LLVMFuzzerInitialize(argc, argv);
+  const std::vector<std::string> Args(*argv, *argv + *argc);
   assert(!Args.empty());
   ProgName = new std::string(Args[0]);
   ParseFlags(Args);
   if (Flags.help) {
     PrintHelp();
     return 0;
   }
 
int FuzzerDriver(int *argc, char ***argv, UserCallback Callback) {
 
   if (Flags.verbosity)
     Printf("Done %d runs in %zd second(s)\n", F.getTotalNumberOfRuns(),
            F.secondsSinceProcessStartUp());
   F.PrintFinalStats();
 
   exit(0);  // Don't let F destroy itself.
 }
-
-int FuzzerDriver(int argc, char **argv, UserCallback Callback) {
-  std::vector<std::string> Args(argv, argv + argc);
-  return FuzzerDriver(Args, Callback);
-}
-
 }  // namespace fuzzer

llvm/trunk/lib/Fuzzer/FuzzerExtFunctions.h

+//===- FuzzerExtFunctions.h - Interface to external functions ---*- C++ -* ===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+// Defines an interface to (possibly optional) functions.
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_FUZZER_EXT_FUNCTIONS_H
+#define LLVM_FUZZER_EXT_FUNCTIONS_H
+
+#include <stddef.h>
+#include <stdint.h>
+
+namespace fuzzer {
+
+struct ExternalFunctions {
+  // Initialize function pointers. Functions that are not available
+  // will be set to nullptr.
+  ExternalFunctions();
+
+#define EXT_FUNC(NAME, RETURN_TYPE, FUNC_SIG, WARN)                            \
+  RETURN_TYPE(*NAME) FUNC_SIG = nullptr
+
+#include "FuzzerExtFunctions.def"
+
+#undef EXT_FUNC
+};
+} // namespace fuzzer
+#endif

llvm/trunk/lib/Fuzzer/FuzzerExtFunctions.def

+//===- FuzzerExtFunctions.def - External functions --------------*- C++ -* ===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+// This defines the external function pointers that
+// ``fuzzer::ExternalFunctions`` should contain and try to initialize.  The
+// EXT_FUNC macro must be defined at the point of inclusion. The signature of
+// the macro is:
+//
+// EXT_FUNC(<name>, <return_type>, <function_signature>, <warn_if_missing>)
+//===----------------------------------------------------------------------===//
+
+// Optional user functions
+EXT_FUNC(LLVMFuzzerInitialize, int, (int *argc, char ***argv), false);
+EXT_FUNC(LLVMFuzzerCustomMutator, size_t,
+         (uint8_t * Data, size_t Size, size_t MaxSize, unsigned int Seed),
+         false);
+
+// TODO: Sanitizer functions

llvm/trunk/lib/Fuzzer/FuzzerExtFunctionsDlsym.cpp

+//===- FuzzerExtFunctionsDlsym.cpp - Interface to external functions ------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+// Implementation for operating systems that support dlsym(). We only use it on
+// Apple platforms for now. We don't use this approach on Linux because it
+// requires that clients of LibFuzzer pass ``--export-dynamic`` to the linker.
+// That is a complication we don't wish to expose to clients right now.
+//===----------------------------------------------------------------------===//
+#include "FuzzerInternal.h"
+#if LIBFUZZER_APPLE
+
+#include "FuzzerExtFunctions.h"
+#include <dlfcn.h>
+
+using namespace fuzzer;
+
+template <typename T>
+static T GetFnPtr(const char *FnName, bool WarnIfMissing) {
+  dlerror(); // Clear any previous errors.
+  void *Fn = dlsym(RTLD_DEFAULT, FnName);
+  if (Fn == nullptr) {
+    if (WarnIfMissing) {
+      const char *ErrorMsg = dlerror();
+      Printf("WARNING: Failed to find function \"%s\".", FnName);
+      if (ErrorMsg)
+        Printf(" Reason %s.", ErrorMsg);
+      Printf("\n");
+    }
+  }
+  return reinterpret_cast<T>(Fn);
+}
+
+namespace fuzzer {
+
+ExternalFunctions::ExternalFunctions() {
+#define EXT_FUNC(NAME, RETURN_TYPE, FUNC_SIG, WARN)                            \
+  this->NAME = GetFnPtr<decltype(ExternalFunctions::NAME)>(#NAME, WARN)
+
+#include "FuzzerExtFunctions.def"
+
+#undef EXT_FUNC
+}
+} // namespace fuzzer
+#endif // LIBFUZZER_APPLE

llvm/trunk/lib/Fuzzer/FuzzerExtFunctionsWeak.cpp

+//===- FuzzerExtFunctionsWeak.cpp - Interface to external functions -------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+// Implementation for Linux. This relies on the linker's support for weak
+// symbols. We don't use this approach on Apple platforms because it requires
+// clients of LibFuzzer to pass ``-U _<symbol_name>`` to the linker to allow
+// weak symbols to be undefined. That is a complication we don't want to expose
+// to clients right now.
+//===----------------------------------------------------------------------===//
+#include "FuzzerInternal.h"
+#if LIBFUZZER_LINUX
+
+#include "FuzzerExtFunctions.h"
+
+extern "C" {
+// Declare these symbols as weak to allow them to be optionally defined.
+#define EXT_FUNC(NAME, RETURN_TYPE, FUNC_SIG, WARN)                            \
+  __attribute__((weak)) RETURN_TYPE NAME FUNC_SIG
+
+#include "FuzzerExtFunctions.def"
+
+#undef EXT_FUNC
+}
+
+using namespace fuzzer;
+
+static void CheckFnPtr(void *FnPtr, const char *FnName, bool WarnIfMissing) {
+  if (FnPtr == nullptr && WarnIfMissing) {
+    Printf("WARNING: Failed to find function \"%s\".\n", FnName);
+  }
+}
+
+namespace fuzzer {
+
+ExternalFunctions::ExternalFunctions() {
+#define EXT_FUNC(NAME, RETURN_TYPE, FUNC_SIG, WARN)                            \
+  this->NAME = ::NAME;                                                         \
+  CheckFnPtr((void *)::NAME, #NAME, WARN);
+
+#include "FuzzerExtFunctions.def"
+
+#undef EXT_FUNC
+}
+} // namespace fuzzer
+#endif // LIBFUZZER_LINUX

llvm/trunk/lib/Fuzzer/FuzzerInternal.h

 #include <cstddef>
 #include <cstdlib>
 #include <random>
 #include <string.h>
 #include <string>
 #include <unordered_set>
 #include <vector>
 
+#include "FuzzerExtFunctions.h"
 #include "FuzzerInterface.h"
 #include "FuzzerTracePC.h"
 
 // Platform detection.
 #ifdef __linux__
 #define LIBFUZZER_LINUX 1
 #define LIBFUZZER_APPLE 0
 #elif __APPLE__
 #define LIBFUZZER_LINUX 0
 #define LIBFUZZER_APPLE 1
 #else
 #error "Support for your platform has not been implemented"
 #endif
 
 namespace fuzzer {
 
 typedef int (*UserCallback)(const uint8_t *Data, size_t Size);
-int FuzzerDriver(int argc, char **argv, UserCallback Callback);
+int FuzzerDriver(int *argc, char ***argv, UserCallback Callback);
 
 using namespace std::chrono;
 typedef std::vector<uint8_t> Unit;
 typedef std::vector<Unit> UnitVector;
 
 // A simple POD sized array of bytes.
 template <size_t kMaxSize> class FixedWord {
 public:
private:
   long TimeOfLongestUnitInSeconds = 0;
   long EpochOfLastReadOfOutputCorpus = 0;
 
   // Maximum recorded coverage.
   Coverage MaxCoverage;
 
   // Need to know our own thread.
   static thread_local bool IsMyThread;
+
+  // Interface to functions that may or may not be available.
+  ExternalFunctions EF;
 };
 
 }; // namespace fuzzer
 
 #endif // LLVM_FUZZER_INTERNAL_H

llvm/trunk/lib/Fuzzer/FuzzerLoop.cpp

 __attribute__((weak)) void
 __sanitizer_set_death_callback(void (*callback)(void));
 __attribute__((weak)) size_t __sanitizer_get_number_of_counters();
 __attribute__((weak)) uintptr_t
 __sanitizer_update_counter_bitset_and_clear_counters(uint8_t *bitset);
 __attribute__((weak)) uintptr_t
 __sanitizer_get_coverage_pc_buffer(uintptr_t **data);
 
-__attribute__((weak)) size_t LLVMFuzzerCustomMutator(uint8_t *Data, size_t Size,
-                                                     size_t MaxSize,
-                                                     unsigned int Seed);
 __attribute__((weak)) void __sanitizer_malloc_hook(void *ptr, size_t size);
 __attribute__((weak)) void __sanitizer_free_hook(void *ptr);
 __attribute__((weak)) void __lsan_enable();
 __attribute__((weak)) void __lsan_disable();
 __attribute__((weak)) int __lsan_do_recoverable_leak_check();
 __attribute__((weak)) int __sanitizer_print_memory_profile(size_t);
 }
 
void Fuzzer::MutateAndTestOne() {
   auto &U = ChooseUnitToMutate();
   assert(CurrentUnitData);
   size_t Size = U.size();
   assert(Size <= Options.MaxLen && "Oversized Unit");
   memcpy(CurrentUnitData, U.data(), Size);
 
   for (int i = 0; i < Options.MutateDepth; i++) {
     size_t NewSize = 0;
-    if (LLVMFuzzerCustomMutator)
-      NewSize = LLVMFuzzerCustomMutator(CurrentUnitData, Size,
+    if (EF.LLVMFuzzerCustomMutator)
+      NewSize = EF.LLVMFuzzerCustomMutator(CurrentUnitData, Size,
                                            Options.MaxLen, MD.GetRand().Rand());
     else
       NewSize = MD.Mutate(CurrentUnitData, Size, Options.MaxLen);
     assert(NewSize > 0 && "Mutator returned empty unit");
     assert(NewSize <= Options.MaxLen &&
            "Mutator return overisized unit");
     Size = NewSize;
     if (Options.OnlyASCII)
       ToASCII(CurrentUnitData, Size);

llvm/trunk/lib/Fuzzer/FuzzerMain.cpp

 //===----------------------------------------------------------------------===//
 
 #include "FuzzerInterface.h"
 #include "FuzzerInternal.h"
 
 extern "C" {
 // This function should be defined by the user.
 int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size);
-// This function may optionally be defined by the user.
-__attribute__((weak)) int LLVMFuzzerInitialize(int *argc, char ***argv);
 }  // extern "C"
 
 int main(int argc, char **argv) {
-  if (LLVMFuzzerInitialize)
-    LLVMFuzzerInitialize(&argc, &argv);
-  return fuzzer::FuzzerDriver(argc, argv, LLVMFuzzerTestOneInput);
+  return fuzzer::FuzzerDriver(&argc, &argv, LLVMFuzzerTestOneInput);
 }