This is an archive of the discontinued LLVM Phabricator instance.

[LibFuzzer] Declare and use sanitizer functions in `fuzzer::ExternalFunctions`
ClosedPublic

Authored by delcypher on Jun 2 2016, 6:42 PM.

Details

Summary

[LibFuzzer] Declare and use sanitizer functions in fuzzer::ExternalFunctions

This fixes linking problems on OSX.

Unfortunately it turns out we need to use an instance of the
fuzzer::ExternalFunctions object in several places so this
commit also replaces all instances with a single global instance.

It also turns out initializing a global fuzzer::ExternalFunctions
before main is entered (i.e. letting the object be initialised by the
global initializers) is not safe (on OSX the call to Printf() in the
CTOR crashes if it is called from a global initializer) so we instead
have a global fuzzer::ExternalFunctions* and initialize it inside
FuzzerDriver().

Multiple unit tests depend also depend on the
fuzzer::ExternalFunctions* global so each unit test that needs
it initializes it for itself.

Diff Detail

Event Timeline

delcypher updated this revision to Diff 59484.Jun 2 2016, 6:42 PM
delcypher retitled this revision from to [LibFuzzer] [WIP] Declare and use sanitizer functions in ``fuzzer::ExternalFunctions``.
delcypher updated this object.
delcypher added reviewers: kcc, aizatsky.
delcypher added subscribers: kcc, aizatsky, kubamracek and 3 others.

@kcc: There are a bunch of problems here that need resolving

  • I've hacked the CHECK_WEAK_API_FUNCTION macro so that the tests pass on Linux but this is really bad. I'm thinking that it would be better to change the last argument `EXT_FUNC to be an enum value (enum { OK, WARN, ERROR};) to indicate how a missing function should be handled and then change the implementations of ExternalFunction::ExternalFunction` to abort as appropriate. That way we could get rid of the CHECK_WEAK_API_FUNCTION macro.
  • I'm not 100% sure which external functions are required and which are just nice to have. I've just made educated guesses about what to warn about but I'm not sure If I have this right.
  • There's still a linking issue on OSX. The LLVMFuzzer-FourIndependentBranchesTest-TracePC test (and probably other tests) fail to link due to having undefined reference to _sanitizer_cov_trace_pc_indir. I'm guessing LibFuzzer ought to provide an implementation but I'm surprised we don't hit this on Linux.

Thoughts?

  • There's still a linking issue on OSX. The LLVMFuzzer-FourIndependentBranchesTest-TracePC test (and probably other tests) fail to link due to having undefined reference to _sanitizer_cov_trace_pc_indir. I'm guessing LibFuzzer ought to provide an implementation but I'm surprised we don't hit this on Linux.

Actually I guess the difference here is the use of libcxx on OSX based on the error message from the linker

[100%] Linking CXX executable ../LLVMFuzzer-FourIndependentBranchesTest-TracePC
Undefined symbols for architecture x86_64:
  "___sanitizer_cov_trace_pc_indir", referenced from:
      std::__1::basic_ostream<char, std::__1::char_traits<char> >& std::__1::__put_character_sequence<char, std::__1::char_traits
<char> >(std::__1::basic_ostream<char, std::__1::char_traits<char> >&, char const*, unsigned long) in FourIndependentBranchesTest
.cpp.o
      std::__1::ostreambuf_iterator<char, std::__1::char_traits<char> > std::__1::__pad_and_output<char, std::__1::char_traits<ch
ar> >(std::__1::ostreambuf_iterator<char, std::__1::char_traits<char> >, char const*, char const*, char const*, std::__1::ios_bas
e&, char) in FourIndependentBranchesTest.cpp.o
ld: symbol(s) not found for architecture x86_64
kcc added inline comments.Jun 3 2016, 5:59 PM
lib/Fuzzer/FuzzerExtFunctions.def
24

This last boolean argument is hard to reason about.
BTW, I don't know why you need it at all.
We must be able to link w./o any of those symbols (see test/UninstrumentedTest.cpp)

lib/Fuzzer/FuzzerIO.cpp
126

Ouch. That's becoming too annoying.
Let's just have a singleton global object "fuzzer::ExternalFunctions *EF" that we initialized at startup
and delete all private instances.

lib/Fuzzer/FuzzerLoop.cpp
49

what do you want to fix here?

delcypher added inline comments.Jun 3 2016, 6:57 PM
lib/Fuzzer/FuzzerExtFunctions.def
24

The meaning of the last boolean argument is whether or not to emit a warning during initialization of ExternalFunctions if the function cannot be found at runtime.

What I was suggesting is that this last argument could be turned into an enum to indicate what to do when function cannot be found at runtime during initialization. This could be one of three things.

  • Do Nothing
  • Emit a warning
  • Emit an error and exit

This does not affect the compile time linking, it affects what happens at run time.

What this would change is that we would immediately exit at runtime if the essential functions are not available rather than waiting until they are called (the currently behaviour). This would then allow the CHECK_WEAK_API_FUNCTION macro to be removed.

How do you feel about this proposal?

lib/Fuzzer/FuzzerLoop.cpp
49
  • The name of the macro is now wrong for OSX (we don't use weak symbols)
  • The suggestion I have made to check for function availability in the ExternalFunctions CTOR means we could get rid of this macro entirely.
  • What I've done is a hack to keep the tests passing because the stringified name #fn gets given to MissingWeakApiFunction(const char *FnName) I had to have users of the macro do

CHECK_WEAK_API_FUNCTION(__sanitizer_reset_coverage)

rather than

CHECK_WEAK_API_FUNCTION(EF.__sanitizer_reset_coverage)

kcc added inline comments.Jun 3 2016, 7:07 PM
lib/Fuzzer/FuzzerExtFunctions.def
24

At the very least that's a change in behavior.
Try not to combine refactoring and changing the behavior it complicates reasoning during the code review.
If you will this functionality is needed let's do it separately.
In this change, please just drop this.

lib/Fuzzer/FuzzerLoop.cpp
49

Remove the FIXME for now.

delcypher added inline comments.Jun 3 2016, 7:14 PM
lib/Fuzzer/FuzzerIO.cpp
126

Do you want this change to be in this patch or a subsequent patch?

kcc added inline comments.Jun 3 2016, 7:17 PM
lib/Fuzzer/FuzzerIO.cpp
126

I don't want you to introduce one more EF here, so yes, in this patch.

delcypher updated this revision to Diff 59799.Jun 6 2016, 4:03 PM
delcypher retitled this revision from [LibFuzzer] [WIP] Declare and use sanitizer functions in ``fuzzer::ExternalFunctions`` to [LibFuzzer] [WIP] Declare and use sanitizer functions in `fuzzer::ExternalFunctions`.
delcypher updated this object.

Rework patch.

@kcc: Sorry for the delay in reworking the patch. I've tested this on Linux and OSX. The tests continue to pass on Linux and on OSX all the tests successfully compile and link although they don't all pass.

kcc added inline comments.Jun 6 2016, 6:09 PM
lib/Fuzzer/FuzzerDriver.cpp
272 ↗(On Diff #59799)

remove the comment, it makes little sense to me

428 ↗(On Diff #59799)

make this ExternalFunctions *EF

lib/Fuzzer/FuzzerExtFunctions.h
23 ↗(On Diff #59799)

Leave the CTOR, initialize with

EF = new ...
delcypher added inline comments.Jun 6 2016, 6:17 PM
lib/Fuzzer/FuzzerDriver.cpp
272 ↗(On Diff #59799)

The comment is stating something fairly important. It's stating that Init should be called after main(). It how is also stated in the ExternalFunctions.h header so I guess this is probably redundant. I'll remove it.

delcypher added inline comments.Jun 6 2016, 6:19 PM
lib/Fuzzer/FuzzerDriver.cpp
428 ↗(On Diff #59799)

We could also use std::unique_ptr<ExternalFunctions> instead or are you fine with an explicit delete?

kcc added inline comments.Jun 6 2016, 6:32 PM
lib/Fuzzer/FuzzerDriver.cpp
428 ↗(On Diff #59799)

I am *not* ok with std::unique_ptr<ExternalFunctions>
And there is no need for an explicit delete.
Just

EF = new  .... ;

in main is enough

In tests you may have new/delete or even

std::unique_ptr<ExternalFunctions> t;
EF = t.get()
delcypher added inline comments.Jun 6 2016, 9:46 PM
lib/Fuzzer/FuzzerDriver.cpp
428 ↗(On Diff #59799)

I am *not* ok with std::unique_ptr<ExternalFunctions>
And there is no need for an explicit delete.
Just

EF = new .... ;
in main is enough

I don't understand. Why do you want a deliberate leak?

In tests you may have new/delete or even

std::unique_ptr<ExternalFunctions> t;
EF = t.get()

Again I don't understand. Why would I want to assign nullptr (that is what t.get() would return here) to EF?

kcc added inline comments.Jun 6 2016, 9:51 PM
lib/Fuzzer/FuzzerDriver.cpp
428 ↗(On Diff #59799)

This won't be a leak. It will be a global variable which is alive throughout the process.

I meant, in test you could either do

FE = new ...
 ....
 delete EF;
 EF = nullptr; // just in case

or, better

       
std::unique_ptr<ExternalFunctions> t(new ...);
EF = t.get()
...
EF = nullptr;  // just tin case
delcypher updated this revision to Diff 59831.EditedJun 6 2016, 11:09 PM
delcypher retitled this revision from [LibFuzzer] [WIP] Declare and use sanitizer functions in `fuzzer::ExternalFunctions` to [LibFuzzer] Declare and use sanitizer functions in `fuzzer::ExternalFunctions`.
delcypher updated this object.
  • Use global pointer for EF
  • Add main() to unit tests to initialize EF.
delcypher marked 13 inline comments as done.Jun 6 2016, 11:14 PM
kcc added inline comments.Jun 7 2016, 11:18 AM
lib/Fuzzer/FuzzerExtFunctions.h
21 ↗(On Diff #59831)

Why you can't call this before main()?
Just curious.

lib/Fuzzer/test/FuzzerUnittest.cpp
430 ↗(On Diff #59831)

Are you sure you need to do that?
Have you seen an lsan report of you don't?

When you do

global_var = new ...

there is no leak from lsan POV because the memory is reachable.

delcypher added inline comments.Jun 7 2016, 3:04 PM
lib/Fuzzer/FuzzerExtFunctions.h
21 ↗(On Diff #59831)

It is probably possible to call somewhere before main() but on OSX when running the uninstrumented test ExternalFunctions::ExternalFunctions tries to use the Printf() function to report that a function is missing. It seems the Printf() call crashes (trying to access an invalid address) when it is invoked from a global initializer. The crash happens when calling vfprintf()

I suspect something wasn't correctly initialized when the Printf() was called leading to the crash. So for safety I thought it best to leave a note saying not to try calling it before entering main().

lib/Fuzzer/test/FuzzerUnittest.cpp
430 ↗(On Diff #59831)

@kcc: Good catch.

Originally the implementation of main() was like this

int main(int argc, char **argv) {
  fuzzer::EF = new ExternalFunctions();
  ::testing::InitGoogleTest(&argc, argv);
  int result =  RUN_ALL_TESTS();
  fuzzer::EF = nullptr;
  return result;
}

In this implementation LSan will fire because I set fuzzer::EF to nullptr. Because of that I started using a std::unique_ptr<ExternalFunctions> but then later I simplified the main() function to not set fuzzer::EF to nullptr. After doing that I forgot to check if the std::unique_ptr<ExternalFunction> was still needed. It isn't really needed anymore.

Would you like me to remove it?

delcypher updated this revision to Diff 59961.Jun 7 2016, 3:09 PM
kcc added inline comments.Jun 7 2016, 3:20 PM
lib/Fuzzer/FuzzerExtFunctions.h
21 ↗(On Diff #59961)

That's quite strange, but ok...

lib/Fuzzer/test/FuzzerUnittest.cpp
430 ↗(On Diff #59961)

Of course, if you do

EF = new ... 
 ...
EF = nullptr

lsan will bark.

Ideally, I want to not add main()
Can you create and destroy EF in every test that needs it?

std::unique_ptr<ExternalFunctions> t(new ExternalFunctions());
EF = t.get();
...

delcypher updated this revision to Diff 59965.Jun 7 2016, 4:18 PM
  • Modify each unittest that needs fuzzer::EF to be initialized to initialize it instead of implementing our main() function.
delcypher updated this object.Jun 7 2016, 4:19 PM
delcypher marked 3 inline comments as done.
kcc accepted this revision.Jun 7 2016, 4:22 PM
kcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 7 2016, 4:22 PM
In D20943#451689, @kcc wrote:

LGTM

Great! With this patch all link failures under OSX are gone :)

This revision was automatically updated to reflect the committed changes.