This is an archive of the discontinued LLVM Phabricator instance.

[libFuzzer] Use dynamic loading for External Functions on Windows.
ClosedPublic

Authored by mpividori on Feb 9 2017, 12:02 AM.

Details

Summary

I replace the weak aliases with dynamic loading. Weak aliases were generating some problems when linking for MT on Windows. For MT, compiler-rt's libraries are statically linked to the main executable the same than libFuzzer, so if we use weak aliases, we are providing two different default functions for the same weak functions and the linker fails.

So, I decided to re implement ExternalFunctions() using dynamic loading, so it works in both cases (MD and MT).
Also, I think dynamic loading is clearer, since we are not defining any auxiliary external function, and we don't need to deal with weak aliases.

It is equivalent to the implementation using dlsym(RTLD_DEFAULT, FnName) for Posix.

Diff Detail

Event Timeline

mpividori created this revision.Feb 9 2017, 12:02 AM
rnk added a subscriber: rnk.Feb 9 2017, 10:47 AM

Are the .def files missing?

@rnk We don't need .def files, because we use dllexport.

rnk added a comment.Feb 9 2017, 10:58 AM

@rnk We don't need .def files, because we use dllexport.

Ignore me, FuzzerExtFunctions.def is already in the repo. I thought it was a new thing.

@zturner @rnk Ok. This is the final diff necessary to support MT. What do you think?

zturner added inline comments.Feb 9 2017, 5:05 PM
lib/Fuzzer/FuzzerExtFunctionsDlsymWin.cpp
45

Should this be:

this->NAME = (decltype(ExternalFunctions::NAME)*) Fn;

? It looks like you're casting to a function instead of a function pointer. (Not sure if there's a difference)

46

What if it still can't find it? You don't set this->NAME to anything, so it's uninitialized memory, but you don't indicate any kind of error.

Also, what happens if it could be found in more than one module but we choose the wrong one?

amccarth edited edge metadata.Feb 9 2017, 5:09 PM

Looks right to me, but I'll leave it to kcc or zturner to accept.

lib/Fuzzer/FuzzerExtFunctionsDlsymWin.cpp
48

I thought the LLVM pattern was to have the #undef in the .def file itself.

That's just a style question. This code is correct.

mpividori added inline comments.Feb 9 2017, 5:19 PM
lib/Fuzzer/FuzzerExtFunctionsDlsymWin.cpp
45

@zturner thanks for your feedback. ExternalFunctions::NAME are functions pointers, defined in FuzzerExtFunctions.h (members of the struct ExternalFunctions).

46

@zturner Yes, this->NAME is set to NULL if GetProcAddress can't find the function. This is ok, because these functions are optional.
If the functions is not found after considering all the modules, a warning is printed depending on the flag WARN. Is the same than for other platforms. For example, for Darwin, when using dlsym, or for linux, when considering weak symbols, if the function is not present, we set a null pointer.

The functions that we look for are:

  • sanitizer's functions, like: __sanitizer_* , __lsan__*, etc. Which are very unlikely to be defined in another module.
  • fuzzer's functions, like: LLVMFuzzerInitialize, LLVMFuzzerCustomMutator, etc.

Anyway, I could update the code to fail if it finds more than one reference for the same function in different modules.

zturner added inline comments.Feb 9 2017, 5:31 PM
lib/Fuzzer/FuzzerExtFunctionsDlsymWin.cpp
46

No, because that would require searching a potentially huge module list even if you find it on the first one. We should optimize for the normal case, not the exceptional case. I don't know what the best solution is, just wanted to make sure you think about the possibility that the function exists in multiple modules.

mpividori added inline comments.Feb 9 2017, 5:38 PM
lib/Fuzzer/FuzzerExtFunctionsDlsymWin.cpp
46

@zturner yes, exactly. Is the same than for dlsym(RTLD_DEFAULT, FnName),:
``
"... There are two special pseudo-handles, RTLD_DEFAULT and RTLD_NEXT. The former will find the first occurrence of the desired symbol using the default library search order. ..."
``
So, it finds the first occurrence.

zturner accepted this revision.Feb 9 2017, 5:43 PM
This revision is now accepted and ready to land.Feb 9 2017, 5:43 PM
This revision was automatically updated to reflect the committed changes.