Skip to content

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Jan 27, 2020

This fixes fuzzing using libFuzzer on macOS, which caused a few issues during the recent review club. macOS users could only fuzz using afl, or inside a VM.

It seems that the __attribute__((weak)) marking is not quite enough to properly mark main() as weak on macOS. See Apples docs on Frameworks and Weak Linking.

Have tested fuzzing using libFuzzer and AFL with this patch.

@practicalswift
Copy link
Contributor

ACK 6b04aff -- diff looks correct

Thanks for fixing this!

@eriknylund
Copy link
Contributor

ACK 6b04aff

Tested both AFL and libFuzzer using documentation from #17942

Both AFL and libFuzzer build and run without issues.

@fjahr
Copy link
Contributor

fjahr commented Jan 27, 2020

ACK 6b04aff

Tested compiling fuzzer tests with libFuzzer which had not been working previously.

@Sjors
Copy link
Member

Sjors commented Jan 28, 2020

What error am I supposed to be seeing on master (I run into plenty of problems, but don't know if that's because not following instructions correctly)?

On macOS 10.15.2 with llvm/9.0.1 (homebrew) I followed the instructions in #17942, first on master then on this PR. On master I ran into subprocess timed out: Currently only libFuzzer is supported. With this branch it just exits after Fuzz targets selected: ['bech32']. And then I'm able to run src/test/fuzz/bech32 and watch a bunch of cryptic info scroll by.

@fjahr
Copy link
Contributor

fjahr commented Jan 28, 2020

What error am I supposed to be seeing on master (I run into plenty of problems, but don't know if that's because not following instructions correctly)?

For me the fuzzers compile when following the instructions from #17942 but when I start any of the fuzzers, they just hang. There is not output and they don't stop until interrupted manually. Running them through the python harness gives an error message about only AFL being supported but that's because the fuzzer hangs and the harness has a timeout built in (error message could be improved though).

@fanquake
Copy link
Member Author

With this branch it just exits after Fuzz targets selected: ['bech32']

@Sjors It doesn't just exit, the bech32 fuzzer is run. However the log level defaults to INFO, which doesn't generate any output. Pass -l DEBUG and you'll see:

Fuzz targets selected: ['bech32']
Run bech32 with args ['/Users/michael/github/fanquake-bitcoin/src/test/fuzz/bech32', '-runs=1', '-detect_leaks=0', '../qa-assets/fuzz_seed_corpus/bech32']
Output: INFO: Seed: 254137248
INFO: Loaded 1 modules   (4751 inline 8-bit counters): 4751 [0x1010c4b58, 0x1010c5de7), 
INFO: Loaded 1 PC tables (4751 PCs): 4751 [0x1010c5de8,0x1010d86d8), 
INFO:      254 files found in ../qa-assets/fuzz_seed_corpus/bech32
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: seed corpus: files: 254 min: 1b max: 713b total: 50505b rss: 27Mb
#255	INITED cov: 1076 ft: 4020 corp: 151/19Kb exec/s: 0 rss: 33Mb
#255	DONE   cov: 1076 ft: 4020 corp: 151/19Kb lim: 713 exec/s: 0 rss: 33Mb
Done 255 runs in 0 second(s)

@maflcko
Copy link
Member

maflcko commented Jan 29, 2020

On master I ran into subprocess timed out: Currently only libFuzzer is supported.

This is exactly the error you are supposed to see without this patch on macos. The fuzzer will run the main function, but not the one provided by libFuzzer.

libFuzzer will provide a main(). This also fixes a weak linking
issue when fuzzing with libFuzzer on macOS.
@fanquake fanquake force-pushed the macos_libfuzzer_weak_main branch from 6b04aff to b35567f Compare January 29, 2020 00:31
@fanquake fanquake changed the title test: fix fuzzing using libFuzzer on macOS test: only declare a main() when fuzzing with AFL Jan 29, 2020
@fanquake
Copy link
Member Author

Updated to drop main() deceleration entirely except for when using AFL.

@maflcko
Copy link
Member

maflcko commented Jan 29, 2020

ACK b35567f

Can't test on mac, sorry 😬

@Sjors
Copy link
Member

Sjors commented Jan 29, 2020

Tested b35567f on macOS 10.15.2 (also checked that AFL still works)

@fjahr
Copy link
Contributor

fjahr commented Jan 29, 2020

ACK b35567f

Tested libFuzzer on macOS.

fanquake added a commit that referenced this pull request Jan 29, 2020
b35567f test: only declare a main() when fuzzing with AFL (fanquake)

Pull request description:

  This fixes fuzzing using [libFuzzer](https://llvm.org/docs/LibFuzzer.html) on macOS, which caused a few issues during the recent review club. macOS users could only fuzz using afl, or inside a VM.

  It seems that the `__attribute__((weak))` marking is not quite enough to properly mark `main()` as weak on macOS. See Apples docs on [Frameworks and Weak Linking](https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/WeakLinking.html#//apple_ref/doc/uid/20002378-107262-CJBJAEID).

  Have tested fuzzing using libFuzzer and AFL with this patch.

ACKs for top commit:
  MarcoFalke:
    ACK b35567f
  fjahr:
    ACK b35567f

Tree-SHA512: b881fdd98c7e1587fcf44debd31f5e7a52df938059ab91c41d0785077b3329b793e051a2bf2eee64488b9f6029d9288c911052ec23ab3ab8c0561a2be1682dae
@fanquake fanquake merged commit b35567f into bitcoin:master Jan 29, 2020
@fanquake fanquake deleted the macos_libfuzzer_weak_main branch January 29, 2020 12:49
maflcko pushed a commit that referenced this pull request Jun 26, 2020
…ding uninstrumented harnesses with --enable-fuzz.

1087807 tests: Provide main(...) function in fuzzer (practicalswift)

Pull request description:

  Provide `main(...)` function in fuzzer. Allow building uninstrumented harnesses with only `--enable-fuzz`.

  This PR restores the behaviour to how things worked prior to #18008. #18008 worked around an macOS specific issue but did it in a way which unnecessarily affected platforms not in need of the workaround :)

  Before this patch:

  ```
  # Build uninstrumented fuzzing harness (no libFuzzer/AFL/other-fuzzer-instrumentation)
  $ ./configure --enable-fuzz
  $ make
    CXXLD    test/fuzz/span
  /usr/lib/gcc/x86_64-linux-gnu/7/../../../x86_64-linux-gnu/Scrt1.o: In function `_start':
  (.text+0x20): undefined reference to `main'
  collect2: error: ld returned 1 exit status
  Makefile:7244: recipe for target 'test/fuzz/span' failed
  make[2]: *** [test/fuzz/span] Error 1
  make[2]: *** Waiting for unfinished jobs....
  $
  ```

  After this patch:

  ```
  # Build uninstrumented fuzzing harness (no libFuzzer/AFL/other-fuzzer-instrumentation)
  $ ./configure --enable-fuzz
  $ make
  $ echo foo | src/test/fuzz/span
  $
  ```

  The examples above show the change in non-macOS functionality. macOS functionality is unaffected by this patch.

ACKs for top commit:
  MarcoFalke:
    ACK 1087807

Tree-SHA512: 9c16ea32ffd378057c4fae9d9124636d11e3769374d340f68a1b761b9e3e3b8a33579e60425293c96b8911405d8b96ac3ed378e669ea4c47836af06892aca73d
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 19, 2021
Summary:
This makes it possible to build with libfuzzer on macOS ([[bitcoin/bitcoin#18008 | PR18008]]), fix
the issue that this PR introduced for other platforms ([[bitcoin/bitcoin#19366 | PR19366]]) and add
the corresponding documentation ([[bitcoin/bitcoin#17942 | PR17942]]).

This is a backport of core [[bitcoin/bitcoin#17942 | PR17942]], [[bitcoin/bitcoin#18008 | PR18008]] and [[bitcoin/bitcoin#19366 | PR19366]].

Test Plan:
On macOS, read the documentation and follow the instructions.
I only ran the instructions for libfuzzer.

  ninja bitcoin-fuzzers
  ./test/fuzz/test_runner.py <path_to_corpus>

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D8956
kwvg added a commit to kwvg/dash that referenced this pull request Aug 2, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 2, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants