Skip to content

Conversation

@omjego
Copy link
Contributor

@omjego omjego commented Oct 8, 2020

Fixes #664

This PR has changes for integrating cpp-httplib with oss-fuzz.

PR in oss-fuzz: google/oss-fuzz#4508 (Will be merged post this with some minor changes in Dockerfile)

Have made following changes after referring to this guide:

  1. Add fuzz target to fuzz test server code.
  2. Add seed corpus and dictionary to help libFuzz generate meaningful inputs
  3. Added new targets (server_fuzzer) in Makefile for easier integration with oss-fuzz
  4. Add a standalone server fuzz target runner to reproduce bugs reported by oss-fuzz locally.

Usage:

cd test
make fuzz_test 

or run

make all

TODO:
While generating fuzz target binaries OSS-Fuzz requires you to link all your dependencies statically , I was able to do that with openssl but faced some issues with brotli. zlib was available for dynamic linking. Right now only openssl and zlib support is added for fuzz testing.

* Add fuzz_test option to Makefile
@yhirose
Copy link
Owner

yhirose commented Oct 9, 2020

@omjego, thanks for your fine work! Could you please rebase your pull request with the latest master, so that we can fix the conflicts with the master and the unit test errors on GitHub Actions. Then, I'll review the pull request. Thanks!

@yhirose
Copy link
Owner

yhirose commented Oct 10, 2020

@omjego, thanks for the adjustments. It seems like it is not practical to put the fuzz test in Makefile, becuase the make file is used on GitHub Actions to kick the unit tests on MacOS as well... Do you think it's possible to make a separate Makefile, so that the current Makefile can stay as it is. (We can run the fuzz test with make -f Makefile.fuzz_test on Linux.)
Sorry for causing extra work.

@omjego
Copy link
Contributor Author

omjego commented Oct 10, 2020

@yhirose Have added separate Makefile inside fuzzing folder, which will solely used by oss-fuzz.
I've still kept the fuzz_test target in test/Makefile, which will just run fuzz test binary on the seed corpus just to make sure that fuzz tests are up to date with the httplib.h Although I've removed linker options(added for static linking in fuzz_test) in test/Makefile which were blocking units tests on macOS.
And can you please hold off the review till I fix presubmit checks in oss-fuzz: google/oss-fuzz#4508

@yhirose
Copy link
Owner

yhirose commented Oct 10, 2020

@omjego, no problem. I'll wait until I hear back from you. Thanks for your hard work.

@omjego
Copy link
Contributor Author

omjego commented Oct 12, 2020

@yhirose Hey I've fixed the issues in oss-fuzz PR google/oss-fuzz#4508 (Please follow the discussion for details)
We won't be using memory sanitizer in the first iteration, I'll send out another PR in oss-fuzz repo (and here to if needed to enable it after this one) and tag you for the review.
You can review this one now. Thank you.

@yhirose
Copy link
Owner

yhirose commented Oct 13, 2020

@omjego, thank you for the further modification! I took a look at the current code, but I honestly don't have enough knowledge about it, and I think I can't maintain the code. So I would like to completely separate any oss-fuzz codes from the current Makefile, and put them into a different Makefile like Makefile.fuzz_test or fuss_test/Makefile.

This approach is same as CMakeLists.txt and cmake folder. I basically don't touch them, but @sum01 is currently maintaining the files for CMake users. I would like to take the same approach for this oss-fuzz support as well, so that I can just focus on maintaining httplib.h itself since my time is limited.

Sorry that it will cause further change in the pull request. But I really appreciate your efforts and your understanding!

@omjego
Copy link
Contributor Author

omjego commented Oct 13, 2020

Hey @yhirose thank you. Have made the suggested changes.
Although previously I added the fuzz_test target in test/Makfile to make sure that the fuzz tests written don't break because of the new changes introduced in httplib.h and are always up-to-date. Maybe we can think of adding make -f Makefile.fuzz_test all to Github action flows to tackle this.
Was also wondering if should document this change for users and contributors.

And maybe @sum01 can help with this issue

@yhirose
Copy link
Owner

yhirose commented Oct 15, 2020

@omjego, thanks for the changes.

Maybe we can think of adding make -f Makefile.fuzz_test all to Github action flows to tackle this.

Please go ahead to adding it to the GItHub Action flows, and make sure that it should be applied only to 'ubuntu-latest' in .github/workflows/test.yaml.

Was also wondering if should document this change for users and contributors.

I don't think we need to mention it on README, because most of users don't need to know the existence. Only when a user sends a pull request and the fuzz test on GitHub Action reports something wrong, they need to know what the message means.

Whenever you feel that it's ready to be merged, please let me know. I'll do it right away. Thanks!

@omjego
Copy link
Contributor Author

omjego commented Oct 15, 2020

@yhirose It's ready to be merged.

@yhirose
Copy link
Owner

yhirose commented Oct 15, 2020

@omjego, great! Thanks for your excellent work that I can't do!

@yhirose yhirose merged commit 5292142 into yhirose:master Oct 15, 2020
@yhirose
Copy link
Owner

yhirose commented Oct 18, 2020

@omjego, I have now started receiving problem reports (26439 and 26453) from oss-fuzz. Since I am not familiar to it, could you figure out what the problems are, and make issues if necessary?
Thanks for your help!

@omjego
Copy link
Contributor Author

omjego commented Oct 19, 2020

@yhirose yes sure :)

ExclusiveOrange pushed a commit to ExclusiveOrange/cpp-httplib-exor that referenced this pull request May 2, 2023
* *Add server fuzzer target  and seed corpus
* Add fuzz_test option to Makefile

* Fix yhirose#685

* Try to fix Github actions on Ubuntu

* Added ReadTimeoutSSL test

* Comment out `-fsanitize=address`

* Rebase upstream changes

* remove address sanitizer temporarily

* Add separate Makefile for fuzzing

* 1. Remove special char from dictionary
2. Clean fuzzing/Makefile

* Use specific path to avoid accidently linking openssl version brought in by oss-fuzz

* remove addition of flags

* Refactor Makefile

* Add missing newline

* Add fuzztest to github workflow

* Fix

Co-authored-by: yhirose <yuji.hirose.bug@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add cpp-httplib to oss-fuzz

2 participants