-
Notifications
You must be signed in to change notification settings - Fork 974
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
Make CMake sanitizer agnostic #82
Conversation
The CI errors might be due to problems on the master branch. These problems were fixed on the latest master, so rebasing might help. |
To make sure we're on the same page: when producing a build for OSS-Fuzz, we should use For CI,
|
332bd43
to
254bdbb
Compare
Update: Rebased and incorporated @Dor1s suggestion of separate compile/link flags. |
5947a54
to
6c7dc2b
Compare
Once this PR is merged,I will create an ossfuzz integration |
Dear all, I created a branch locally to add snappy to ossfuzz and verified that all sanitizer builds (asan, ubsan, msan) are successful. Looking forward to the review of this PR. Thank you! |
Dear all, A gentle reminder that this PR is awaiting a review :-) Thank you! |
Dear all, I'm waiting on a review to get this merged :-) Thank you |
6c7dc2b
to
dd70a3c
Compare
Rebased on current master |
@pwnall - can you please help to review this latest rebased change. Thanks! |
Sorry for the radio silence here. I won't be able to tackle this for another week or so. |
dd70a3c
to
1a77adf
Compare
Oops, spotted another mistake in this PR in the meantime :-) Now, snappy builds should build transparently against choice of |
1a77adf
to
bb89c2c
Compare
@bshastry @inferno-chromium @Dor1s I don't like the branching in this PR. I'd prefer that both the CMake config and the Travis config stay as simple as possible, and that it's easy to get a reasonable fuzzing build up in development. I've prototyped an alternative at pwnall@a4b8446 in https://github.com/pwnall/snappy/tree/fix_fuzz -- TL;DR: the fuzzing flags get extracted to |
@pwnall I had a quick look at your prototype. I was mistaken. This can work provided we don't hard code the linking option to "-fsanitize=fuzzer" and write the build script correctly.
|
bb89c2c
to
0a4f692
Compare
@pwnall Rebased and tried to incorporate your feedback. Hope I got it right. |
76a560b
to
f270149
Compare
f270149
to
999ab17
Compare
Dear @pwnall I readied the PR for a re-review. Looking forward. Thank you, |
Dear all, I was wondering if you had the chance to review this PR. Would be nice to see snappy fuzzed continuously :-) Best |
@pwnall - can you please take another look. |
Ping :-) |
999ab17
to
c808648
Compare
Rebased. |
c808648
to
64eaea0
Compare
@pwnall could you please take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annotated PR with my notes.
option(SNAPPY_OSSFUZZ_BUILD "Build Snappy for oss-fuzz." OFF) | ||
|
||
set(SNAPPY_FUZZING_FLAGS "-fsanitize=address" CACHE STRING | ||
"Compiler flags for fuzzing (e.g., -fsanitize=address).") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SNAPPY_FUZZING_FLAGS
are used for the CI fuzzer build.
@@ -45,6 +45,11 @@ option(SNAPPY_BUILD_TESTS "Build Snappy's own tests." ON) | |||
|
|||
option(SNAPPY_FUZZING_BUILD "Build Snappy for fuzzing." OFF) | |||
|
|||
option(SNAPPY_OSSFUZZ_BUILD "Build Snappy for oss-fuzz." OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SNAPPY_OSSFUZZ_BUILD
is set by the oss-fuzz builder. See https://github.com/google/oss-fuzz/pull/2874/files#diff-92672b1afbd2ebe31528dff0bd201f41R22-R23
message(WARNING "Fuzzing builds are only supported with Clang") | ||
endif (NOT "${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang") | ||
endif(NOT "${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warn if either the CI fuzzing build or the ossfuzz build is not being compiled via clang
if(NOT DEFINED ENV{LIB_FUZZING_ENGINE}) | ||
message(SEND_ERROR "OSS-Fuzz builds require LIB_FUZZING_ENGINE to be set") | ||
endif(NOT DEFINED ENV{LIB_FUZZING_ENGINE}) | ||
endif(SNAPPY_OSSFUZZ_BUILD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error if the ossfuzz build has not defined LIB_FUZZING_ENGINE
environment variable.
if(NOT CMAKE_CXX_FLAGS MATCHES "${SNAPPY_FUZZING_FLAGS}") | ||
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${SNAPPY_FUZZING_FLAGS}") | ||
endif(NOT CMAKE_CXX_FLAGS MATCHES "${SNAPPY_FUZZING_FLAGS}") | ||
endif(SNAPPY_FUZZING_BUILD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Append fuzzing related compiler flags for the CI fuzzing build
if(DEFINED ENV{LIB_FUZZING_ENGINE}) | ||
set_target_properties(snappy_compress_fuzzer | ||
PROPERTIES LINK_FLAGS $ENV{LIB_FUZZING_ENGINE} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link against LIB_FUZZING_ENGINE
for the ossfuzz build
else() | ||
set_target_properties(snappy_compress_fuzzer | ||
PROPERTIES LINK_FLAGS "-fsanitize=fuzzer" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link against libfuzzer for the CI fuzzing build
if(DEFINED ENV{LIB_FUZZING_ENGINE}) | ||
set_target_properties(snappy_uncompress_fuzzer | ||
PROPERTIES LINK_FLAGS $ENV{LIB_FUZZING_ENGINE} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here and below
64eaea0
to
8d6d17a
Compare
Rebased. |
Friendly reminder that this PR is pending another round of reviews :-) |
Friendly reminder that this PR is pending review CC @inferno-chromium :-) This PR blocks google/oss-fuzz#2874 |
I think this PR may be closed now since integration is complete? |
Closing due to complete integration |
Fixes a minor issue in #78 (see #78 (comment)) and another one (see google/oss-fuzz#2834 (comment))