Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

attempt at oss-fuzz integration #1476

Closed
wants to merge 2 commits into from

Conversation

freddyb
Copy link
Contributor

@freddyb freddyb commented May 9, 2017

This is a first attempt to integrate the oss-fuzz target into the curl repository.
oss-fuzz wants to be as build-system agnostic as possible, so the desired outcome is that oss-fuzz can do something along the lines of

  • check out curl source
  • configure
  • make fuzz-target
  • link the fuzz-target(s) to their libFuzzer.o
  • fuzz

This initial pull request takes the targets from the existing oss-fuzz code, and rewrites it into C89 that is up to curl's coding style guidelines (at least that's what I hope!).
That being said, I'd happy to expose some other functionality that is considered more worthwhile to test.

What this pull request is lacking, is proper build-system integration as I am quite inexperienced with the various "makes" (cmake, automake, etc.). Please review and advise how to proceed :)

@bagder
Copy link
Member

bagder commented May 13, 2017

0001-fuzz-makefile-adjustments.patch

Here's my initial take at adding support for configure / make to build the test code in the tests/fuzz directory, built on top of your work. You need to apply the patch, rerun ./buildconf and configure and then make should work in the test/fuzz dir.

The building of the test apps doesn't quite work but it wasn't clear to me exactly how they should get built so I couldn't fix that completely. What compile and linking should get done for each of the fuzz programs?

@freddyb
Copy link
Contributor Author

freddyb commented May 14, 2017

The building of the test apps doesn't quite work but it wasn't clear to me exactly how they should get built so I couldn't fix that completely. What compile and linking should get done for each of the fuzz programs?

The fuzz apps, aren't strictly apps. They are static libraries. LibFuzzer will provide the main file and loop though the LLVMFuzzerTestOneInput function exposed by the library.

With your patch, I could build the individual test apps with make httpupload-curl_fuzzer.o, and pop3-curl_fuzzer.o and such. These aren't really defined in the makefiles but through automake, so I'm not sure how I'd define a list of all available targets to create a build target that easily extensible.

I also realize that while I tried to explain what oss-fuzz expects, I should have linked to the original sources. I am currently looking at their integration guide and the existing integration

@bagder
Copy link
Member

bagder commented May 14, 2017

The fuzz apps, aren't strictly apps. They are static libraries. LibFuzzer will provide the main file and loop though the LLVMFuzzerTestOneInput function exposed by the library.

I looked at the existing integration just now. Shouldn't we just make the makefile link each fuzz app with libcurl and use -lFuzzingEngine to create the outputs?

With your patch, I could build the individual test apps with make httpupload-curl_fuzzer.o, and pop3-curl_fuzzer.o and such. These aren't really defined in the makefiles but through automake, so I'm not sure how I'd define a list of all available targets to create a build target that easily extensible

It builds, or tries to build, each app that is listed in the FUZZPROGS variable at the top of tests/fuzz/Makefile.inc. The rest of the Makefile.inc then lists the specific requirements for each of those applications.

@bagder
Copy link
Member

bagder commented May 14, 2017

I played around a little more with the addition below, but I get funny link errors so it's not quite there yet:

diff --git a/tests/fuzz/Makefile.am b/tests/fuzz/Makefile.am
index e77dcb708..44906ce4b 100644
--- a/tests/fuzz/Makefile.am
+++ b/tests/fuzz/Makefile.am
@@ -40,14 +40,14 @@ AM_CPPFLAGS = -I$(top_builddir)/include/curl \
               -I$(top_srcdir)/tests/fuzz
 
 EXTRA_DIST = Makefile.inc CMakeLists.txt
 
 # Prevent LIBS from being used for all link targets
-LIBS = $(BLANK_AT_MAKETIME)
+LIBS = -lpthread -lFuzzer
+LDFLAGS = -L/usr/lib/llvm-5.0/lib
 
-LDADD = $(top_builddir)/src/libcurltool.la   \
-        $(top_builddir)/lib/libcurlu.la      \
+LDADD = $(top_builddir)/lib/libcurl.la      \
         @LDFLAGS@ @LIBCURL_LIBS@
 
 # Makefile.inc provides neat definitions
 include Makefile.inc
 

@freddyb
Copy link
Contributor Author

freddyb commented May 14, 2017

-LIBS = $(BLANK_AT_MAKETIME)
+LIBS = -lpthread -lFuzzer

It seems that other oss-fuzz build scripts want to link against the fuzzer (in contrast to doing it in the target). I'm not sure about the pros & cons, just a thing I observed.

@bagder
Copy link
Member

bagder commented May 19, 2017

I'm trying to make this link using the already built libcurl and using the libFuzzer I have installed as part of clang, I don't have any particular opinions exactly how that is done as long as the test applications end up working! They don't right now and I'm not sure how it is supposed to work really...

Currently this setup causes this set of errors:

/usr/bin/ld: /usr/lib/llvm-5.0/lib/libFuzzer.a(FuzzerMain.o): relocation R_X86_64_32 against symbol 'LLVMFuzzerTestOneInput' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: /usr/lib/llvm-5.0/lib/libFuzzer.a(FuzzerDriver.o): relocation R_X86_64_32 against '.rodata.str1.1' can not be used when making a shared object; recompile with -fPIC

Alternatively, I use this:

LIBS = -lpthread -Wl,-Bstatic -lFuzzer

which instead leads to other complaints:

/usr/bin/ld: attempted static link of dynamic object `/home/daniel/build-nghttp2/lib/libnghttp2.so'
collect2: error: ld returned 1 exit status

@jay
Copy link
Member

jay commented May 20, 2017

/usr/bin/ld: /usr/lib/llvm-5.0/lib/libFuzzer.a(FuzzerMain.o): relocation R_X86_64_32 against symbol 'LLVMFuzzerTestOneInput' can not be used when making a shared object; recompile with -fPIC

Unfortunately I'm pretty sure this is what it says. If the library is static (ie you want a static foo in dynamic libcurl) then rebuild static foo with -fPIC or use static libcurl.

/usr/bin/ld: attempted static link of dynamic object `/home/daniel/build-nghttp2/lib/libnghttp2.so'
collect2: error: ld returned 1 exit status

This is likely because you didn't turn off static afterwards, so basically LIBS becomes something like -lpthread -Wl,-Bstatic -lFuzzer -lnghttp2 then both are static, when what I think you wanted to do is toggle it again like -lpthread -Wl,-Bstatic -lFuzzer -Wl,-Bdynamic -lnghttp2. But I really don't understand why you'd want to force static Fuzzer if that's giving you the PIC problem.

@bagder
Copy link
Member

bagder commented May 21, 2017

rebuild static foo with -fPIC or use static libcurl

That was with a static libcurl, but not all dependencies were static.

I really don't understand why you'd want to force static Fuzzer if that's giving you the PIC problem.

I don't care which. I want a working build and I don't have that.

@jay
Copy link
Member

jay commented May 22, 2017

That was with a static libcurl, but not all dependencies were static.

Ok. Well it says "can not be used when making a shared object" so I think a good place to start is examine the verbose output and find out what object it is. Also you might want to try -Wl,--verbose to see where all the dependencies are coming from.

@bagder
Copy link
Member

bagder commented May 22, 2017

Here's another take.

  1. Apply the patch v2-0001-fuzz-makefile-adjustments.patch and run
    ./buildconf
  2. Build curl with clang:
$ CC=clang ./configure --disable-shared --enable-debug --enable-maintainer-mode ...
$ make -sj
  1. Build the fuzzers:
$ cd tests/fuzz
$ make

The patch currently points out the libFuzzer directory with a hard-coded path, which is a bit unfortunate but on my Debian system it isn't put in a path that the linker searches by default.

The difference from before is that this works if we build with clang and we add libstdc++ and libm as explicit libs on the linker command line.

@bagder
Copy link
Member

bagder commented May 22, 2017

I can't get the produced tools to actually do anything though so I guess there's still something missing:

$ ./http11 ./corpora/http1_1
INFO: Seed: 618696309
INFO: Loaded 0 modules (0 guards): 
Loading corpus dir: ./corpora/http1_1
INFO: -max_len is not provided, using 327
#0      READ units: 2
#2      INITED exec/s: 0 rss: 38Mb
ERROR: no interesting inputs were found. Is the code instrumented for coverage? Exiting.

@bagder
Copy link
Member

bagder commented Jun 8, 2017

@freddyb do you know if that is supposed to work or perhaps what we need to do to actually make some fuzzing happen?

@Dor1s
Copy link

Dor1s commented Jun 15, 2017

Re #1476 (comment) and #1476 (comment), I guess that -fsanitize and -fsanitize-coverage flags are missing. Here are exact combinations of those flags used by OSS-Fuzz: https://github.com/google/oss-fuzz/blob/master/infra/base-images/base-builder/Dockerfile

e.g. -fsanitize=address -fsanitize-address-use-after-scope -fsanitize-coverage=trace-pc-guard,trace-cmp to get ASan build.

@bagder
Copy link
Member

bagder commented Jun 15, 2017

Ah yes, with those flags it seems to run. Thanks a lot. I figure the next step is then to make sure it actually runs something useful...

@inferno-chromium
Copy link

inferno-chromium commented Jun 15, 2017

curl fuzzer hasn't found anything interesting, so i think the fuzz target might need improvements as well.

@bagder
Copy link
Member

bagder commented Jun 15, 2017

I think the fuzz target might need improvements as well.

Yes clearly! I suppose I should merge what we have so far to make it easier for others to help out here.

@freddyb
Copy link
Contributor Author

freddyb commented Jun 15, 2017 via email

@inferno-chromium
Copy link

Thanks a ton, we are very excited to see this integration finally happening.

@bagder bagder closed this in 7e8247c Jun 15, 2017
@bagder
Copy link
Member

bagder commented Jun 15, 2017

Thanks @freddyb, this first take is now merged.

@lock lock bot locked as resolved and limited conversation to collaborators May 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants