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

[bitcoin-core] Support AFL builds #5716

Merged
merged 1 commit into from
May 6, 2021

Conversation

guidovranken
Copy link
Contributor

Copy link
Collaborator

@DavidKorczynski DavidKorczynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks Guido! Since this is a fairly new project and an issue related to it was opened here #5713 I will just wait for @MarcoFalke approval before merging.

Copy link
Contributor

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Looks good. Some minor cleanup ideas that can be done the next time the file is touched.

@@ -44,8 +44,8 @@ fi
if [ "$FUZZING_ENGINE" = "libfuzzer" ]; then
CONFIG_SITE="$PWD/depends/$BUILD_TRIPLET/share/config.site" ./configure --enable-fuzz --with-sanitizers=fuzzer
else
# See https://google.github.io/oss-fuzz/getting-started/new-project-guide/#Requirements
CONFIG_SITE="$PWD/depends/$BUILD_TRIPLET/share/config.site" ./configure --enable-fuzz LDFLAGS="$LIB_FUZZING_ENGINE"
sed -i "s|PROVIDE_FUZZ_MAIN_FUNCTION|NEVER_PROVIDE_MAIN_FOR_OSS_FUZZ|g" "./src/test/fuzz/fuzz.cpp"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot that the fuzz.cpp is hacked later for another reason. For a smaller diff, maybe:

Suggested change
sed -i "s|PROVIDE_FUZZ_MAIN_FUNCTION|NEVER_PROVIDE_MAIN_FOR_OSS_FUZZ|g" "./src/test/fuzz/fuzz.cpp"
sed -i "s|PROVIDE_FUZZ_MAIN_FUNCTION|NEVER_PROVIDE_MAIN_FOR_OSS_FUZZ|g" "./configure.ac"

@@ -44,8 +44,8 @@ fi
if [ "$FUZZING_ENGINE" = "libfuzzer" ]; then
CONFIG_SITE="$PWD/depends/$BUILD_TRIPLET/share/config.site" ./configure --enable-fuzz --with-sanitizers=fuzzer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to get rid of this special case and replace this with

Suggested change
CONFIG_SITE="$PWD/depends/$BUILD_TRIPLET/share/config.site" ./configure --enable-fuzz --with-sanitizers=fuzzer
CONFIG_SITE="$PWD/depends/$BUILD_TRIPLET/share/config.site" ./configure --enable-fuzz SANITIZER_LDFLAGS="$LIB_FUZZING_ENGINE"

as well

@maflcko
Copy link
Contributor

maflcko commented May 6, 2021

Suggested cleanup diff:

diff --git a/projects/bitcoin-core/build.sh b/projects/bitcoin-core/build.sh
index 92cd4f94..30fb71d1 100755
--- a/projects/bitcoin-core/build.sh
+++ b/projects/bitcoin-core/build.sh
@@ -36,17 +36,13 @@ fi
 
 # Build the fuzz targets
 
+sed -i "s|PROVIDE_FUZZ_MAIN_FUNCTION|NEVER_PROVIDE_MAIN_FOR_OSS_FUZZ|g" "./configure.ac"
 ./autogen.sh
 
 # OSS-Fuzz will provide CC, CXX, etc. So only set:
 # * --enable-fuzz, see https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md
 # * CONFIG_SITE, see https://github.com/bitcoin/bitcoin/blob/master/depends/README.md
-if [ "$FUZZING_ENGINE" = "libfuzzer" ]; then
-  CONFIG_SITE="$PWD/depends/$BUILD_TRIPLET/share/config.site" ./configure --enable-fuzz --with-sanitizers=fuzzer
-else
-  sed -i "s|PROVIDE_FUZZ_MAIN_FUNCTION|NEVER_PROVIDE_MAIN_FOR_OSS_FUZZ|g" "./src/test/fuzz/fuzz.cpp"
-  CONFIG_SITE="$PWD/depends/$BUILD_TRIPLET/share/config.site" ./configure --enable-fuzz SANITIZER_LDFLAGS="$LIB_FUZZING_ENGINE"
-fi
+CONFIG_SITE="$PWD/depends/$BUILD_TRIPLET/share/config.site" ./configure --enable-fuzz SANITIZER_LDFLAGS="$LIB_FUZZING_ENGINE"
 
 make -j$(nproc)
 
@@ -55,7 +51,6 @@ FUZZ_TARGETS=( 'process_messages' 'asmap' )
 for fuzz_target in ${FUZZ_TARGETS[@]}; do
   git checkout -- "./src/test/fuzz/fuzz.cpp"
   sed -i "s|std::getenv(\"FUZZ\")|\"$fuzz_target\"|g" "./src/test/fuzz/fuzz.cpp"
-  sed -i "s|PROVIDE_FUZZ_MAIN_FUNCTION|NEVER_PROVIDE_MAIN_FOR_OSS_FUZZ|g" "./src/test/fuzz/fuzz.cpp"
   make -j$(nproc)
   mv ./src/test/fuzz/fuzz $OUT/$fuzz_target
   (

@maflcko
Copy link
Contributor

maflcko commented May 6, 2021

@jonathanmetzman You can merge this one

@DavidKorczynski DavidKorczynski merged commit 9a492d4 into google:master May 6, 2021
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.

3 participants