cmake: Avoid fuzzer "multiple definition of `main'" errors

ryanofsky commented at 10:51 pm on March 4, 2025: contributor

This change builds libraries with -fsanitize=fuzzer-no-link instead of -fsanitize=fuzzer when the cmake -DSANITIZERS=fuzzer option is specified. This is necessary to make fuzzing and IPC cmake options compatible with each other and avoid CI failures in #30975 which enables IPC in the fuzzer CI build:

https://cirrus-ci.com/task/5366255504326656?logs=ci#L2817 https://cirrus-ci.com/task/5233064575500288?logs=ci#L2384

The failures can also be reproduced by checking out #31741 and building with cmake -B build -DBUILD_FOR_FUZZING=ON -DSANITIZERS=fuzzer -DENABLE_IPC=ON with this fix reverted.

The fix updates the cmake build so when -DSANITIZERS=fuzzer is specified, the fuzz test binary is built with -fsanitize=fuzzer (so it can use libFuzzer’s main function), and libraries are built with -fsanitize=fuzzer-no-link (so they can be linked into other executables with their own main functions).

Previously when -DSANITIZERS=fuzzer was specified, -fsanitize=fuzzer was applied to ALL libraries and executables. This was inappropriate because it made it impossible to build any executables other than the fuzz test executable without triggering link errors:

multiple definition of `main'
"undefined reference to `LLVMFuzzerTestOneInput'

if they depended on any libraries instrumented for fuzzing.

This was especially a problem when the ENABLE_IPC option was set because it made building the mpgen code generator impossible so nothing else that depended on generated sources, including the fuzz test binary, could be built either.

This commit was previously part of #31741 and had some discussion there starting in #31741#pullrequestreview-2619682385

This PR is part of the process separation project.

cmake: Avoid fuzzer "multiple definition of `main'" errors

This change builds libraries with -fsanitize=fuzzer-no-link instead of
-fsanitize=fuzzer when the cmake -DSANITIZERS=fuzzer option is specified. This
is necessary to make fuzzing and IPC cmake options compatible with each other
and avoid CI failures in #30975 which enables IPC in the fuzzer CI build:

https://cirrus-ci.com/task/5366255504326656?logs=ci#L2817
https://cirrus-ci.com/task/5233064575500288?logs=ci#L2384

The failures can also be reproduced by checking out #31741 and building with
`cmake -B build -DBUILD_FOR_FUZZING=ON -DSANITIZERS=fuzzer -DENABLE_IPC=ON`
with this fix reverted.

The fix updates the cmake build so when -DSANITIZERS=fuzzer is specified, the
fuzz test binary is built with -fsanitize=fuzzer (so it can use libFuzzer's
main function), and libraries are built with -fsanitize=fuzzer-no-link (so they
can be linked into other executables with their own main functions).

Previously when -DSANITIZERS=fuzzer was specified, -fsanitize=fuzzer was
applied to ALL libraries and executables. This was inappropriate because it
made it impossible to build any executables other than the fuzz test executable
without triggering link errors:

- "multiple definition of `main'"
- "undefined reference to `LLVMFuzzerTestOneInput'"

if they depended on any libraries instrumented for fuzzing.

This was especially a problem when the ENABLE_IPC option was set because it
made building the mpgen code generator impossible so nothing else that depended
on generated sources, including the fuzz test binary, could be built either.

This commit was previously part of
https://github.com/bitcoin/bitcoin/pull/31741 and had some discussion there
starting in
https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2619682385

57d8b1f1b3

DrahtBot commented at 10:51 pm on March 4, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31992.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto
Concept ACK	TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31802 (Add bitcoin-{node,gui} to release binaries for IPC by Sjors)
#31741 (multiprocess: Add libmultiprocess git subtree by ryanofsky)
#30975 (ci: build multiprocess on most jobs by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Build system on Mar 4, 2025

ryanofsky commented at 10:52 pm on March 4, 2025: contributor

@hebasto suggested separating this change from #31741, so this is now a standalone PR.

dergoegge commented at 8:59 am on March 12, 2025: member

Why do we want IPC enabled for the fuzzing builds? It’s not used/tested by our fuzzing harnesses.

ryanofsky commented at 1:39 pm on March 12, 2025: contributor

Why do we want IPC enabled for the fuzzing builds? It’s not used/tested by our fuzzing harnesses.

To be clear, this PR isn’t enabling IPC in the fuzzing CI configuration, just fixing a more general build system bug which causes that configuration to be broken. But a followup PR #30975 does enable IPC in the fuzzing CI configuration for a few reasons:

To test ENABLE_IPC option in different scenarios and uncover incompatibilities like this
To make the fuzz configuration more similar to an eventual default configuration, since a goal after #31741 is to enable ipc by default.
To make it possible to write fuzz tests covering IPC serialization code. This could involve writing new tests or just changing setup of existing tests. For example with #29409, FuzzedWallet could be passed an IPC chain interface instead of a local one.

ryanofsky commented at 1:53 pm on March 12, 2025: contributor

Also while the PR description mentions the ENABLE_IPC / SANITIZERS=fuzzerincompatibility to motivate this change, I think the change is a more general bugfix and makes sense for a variety of reasons listed previously in #31741 (comment):

(1) It fixes the CI bug (2) It documents sanitize_interface and removes an attribute(weak) hack. (3) It doesn’t compile libraries with a flag intended to bring in a main() function. (4) It provides immediate and clear feedback if -DSANITIZERS=fuzzer is used with without -DBUILD_FOR_FUZZING=ON which is is a perfectly logical thing to try and easy thing to forget about, instead of providing an opaque error message and failing at the very end of a slow build. (5) It lets us write useful fuzz tests that cover IPC code.

ryanofsky commented at 11:30 am on March 19, 2025: contributor

@hebasto since you asked me to create this PR and separate it from #31741, do you have any particular concerns or thoughts here?

I think this change is an improvement by itself, not just a fix for IPC issues because it adds a clear error message if -DSANITIZERS=fuzzer is specified without -DBUILD_FOR_FUZZING=ON instead of causing confusing link errors. I also think it is a conceptual improvement to only add flags appropriate for building libraries to core_interface and not make it impossible to build other binaries depending on core_interface when fuzzing is enabled.

dergoegge commented at 9:33 am on March 24, 2025: member

Re: #31992 (comment)

Thank you for explaining.

I haven’t tested this but looking at the diff here I don’t think this will work for all fuzzing engines, as it only works around the error for libFuzzer. Oss-fuzz for example uses FUZZ_LIBS to link against the afl++ libfuzzer driver (i.e. FUZZ_LIBS=/usr/lib/libFuzzingEngine.a). This is just one example and it is quite useful to allow linking against pretty much anything here.

Could we instead opt to not build mpgen with any instrumentation? Iiuc we’re not interested in fuzzing mpgen itself.

ryanofsky force-pushed on Mar 24, 2025

ryanofsky commented at 3:32 pm on March 24, 2025: contributor

Updated 34aeb70748ef8ee186fe53f0db2580a445452dc2 -> 60d2afe65484f755d99191bb650b9f9a784ee2c2 (pr/subtree-fuzz.1 -> pr/subtree-fuzz.2, compare) moving FUZZ_LIBS to the fuzzer_interface target

re: #31992 (comment)

I haven’t tested this but looking at the diff here I don’t think this will work for all fuzzing engines, as it only works around the error for libFuzzer. Oss-fuzz for example uses FUZZ_LIBS to link against the afl++ libfuzzer driver (i.e. FUZZ_LIBS=/usr/lib/libFuzzingEngine.a). This is just one example and it is quite useful to allow linking against pretty much anything here.

Good catch, and thanks for checking this. I didn’t notice the FUZZ_LIBS variable, but logically it also makes sense for that to be part of the fuzzer_interface target not core_interface target. Latest push should move it, though even without this change I think the oss-fuzz build would be ok since it is not enabling ipc

Could we instead opt to not build mpgen with any instrumentation? Iiuc we’re not interested in fuzzing mpgen itself.

It would be possible, but there would be complications because mpgen depends on a the libmultiprocess library which should be built with fuzzing options and should possible to fuzz. A workaround like building two copies of the library with and without fuzzing options would would be technically possible, but I don’t see a reason to implement a workaround like this. I think an approach where the build provides a fuzzer_interface target to contain fuzz linker flags is strictly better than adding fuzz linker flags to the core_interface target where they will be applied to all binaries including ones that are not fuzz tests.

hebasto commented at 4:27 pm on March 24, 2025: member

Concept ACK.

in CMakeLists.txt:368 in 60d2afe654 outdated

363+  # Transform list of sanitizers into -fsanitize flags, replacing "fuzzer" with
364+  # "fuzzer-no-link" in sanitize_interface flags, and moving "fuzzer" to
365+  # fuzzer_interface flags.
366+  string(REGEX REPLACE "(^|,)fuzzer($|,)" "\\1fuzzer-no-link\\2" SANITIZE_FLAG "${SANITIZERS}")
367+  set(FUZZ_FLAG "")
368+  if (NOT "${SANITIZE_FLAG}" STREQUAL "${SANITIZERS}")

hebasto commented at 5:15 pm on March 24, 2025:

I found this code hard to read. Wouldn’t it better:

0  if(SANITIZERS MATCHES "(^|,)fuzzer($|,)")

?

style nit: The surrounding code has no space between if and (.

ryanofsky commented at 4:56 pm on March 25, 2025:

re: #31992 (review)

Thanks, fixed spacing. The other change seems like it would duplicate the regex, so I it’s not a change I would like to spend time implementing myself. But if you want to post a more complete change that I can apply, I’d happy to apply it, since I don’t have a strong preference.

in CMakeLists.txt:374 in 60d2afe654 outdated

369+    set(FUZZ_FLAG "-fsanitize=fuzzer")
370+    if(NOT BUILD_FOR_FUZZING)
371+      message(FATAL_ERROR "Error: Enabling -DSANITIZERS=fuzzer without -DBUILD_FOR_FUZZING=ON is not supported.")
372+    endif()
373+  endif()
374+  set(SANITIZE_FLAG "-fsanitize=${SANITIZE_FLAG}")

hebasto commented at 5:17 pm on March 24, 2025:

Maybe avoid reusing the SANITIZE_FLAG variable?

style-nit: The surrounding code mostly uses ALL_CAPS naming for user-faced variables, not for internal ones.

ryanofsky commented at 5:06 pm on March 25, 2025:

re: #31992 (review)

Thanks made both changes and renamed SANITIZE_FLAG and FUZZ_FLAGS variables. I wasn’t aware of the caps convention but it makes sense and seems helpful.

ryanofsky commented at 5:37 pm on March 24, 2025: contributor

re: #31992 (comment) @hebasto thanks for the review! I can try to implement your suggested changes but since they are all about code style, maybe it would be faster if you could just post a diff with your preferred change and I can apply it? I could try to figure out how to integrate your suggestions, but since they go against my own preferences for having fewer variables and not repeating code I think it would be harder for me to figure out what you want than for you to just implement what you want.

hebasto commented at 1:38 pm on March 25, 2025: member

Approach ACK 60d2afe65484f755d99191bb650b9f9a784ee2c2, I have reviewed the code and it looks OK.

Tested on Ubuntu 24.10 and in the OSS-Fuzz environment.

As ‘a separate “fuzzer_interface” with flags specifically intended to apply to the fuzz test binary’, it seems reasonable to place all <COMMAND>(fuzzer_interface ...) invocations under the existing if(BUILD_FUZZ_BINARY) condition.

nit: No need to quote variables here:

 0--- a/CMakeLists.txt
 1+++ b/CMakeLists.txt
 2@@ -365,7 +365,7 @@ if(SANITIZERS)
 3   # fuzzer_interface flags.
 4   string(REGEX REPLACE "(^|,)fuzzer($|,)" "\\1fuzzer-no-link\\2" SANITIZE_FLAG "${SANITIZERS}")
 5   set(FUZZ_FLAG "")
 6-  if (NOT "${SANITIZE_FLAG}" STREQUAL "${SANITIZERS}")
 7+  if(NOT SANITIZE_FLAG STREQUAL SANITIZERS)
 8     set(FUZZ_FLAG "-fsanitize=fuzzer")
 9     if(NOT BUILD_FOR_FUZZING)
10       message(FATAL_ERROR "Error: Enabling -DSANITIZERS=fuzzer without -DBUILD_FOR_FUZZING=ON is not supported.")

… they go against my own preferences…

Understood.

The only question I have before final ACK: I failed to figure out the documented behaviour where both -fsanitize=fuzzer and -fsanitize=fuzzer-no-link are passed simultaneously to the compiler during linking. Any insights on that?

ryanofsky force-pushed on Mar 25, 2025

ryanofsky commented at 6:19 pm on March 25, 2025: contributor

Updated 60d2afe65484f755d99191bb650b9f9a784ee2c2 -> 57d8b1f1b3375452213c48ce80e8207e2fe53ebc (pr/subtree-fuzz.2 -> pr/subtree-fuzz.3, compare) making suggested code changes and also dropping the check that forbid specifying -DSANITIZERS=fuzzer without -DBUILD_FOR_FUZZING=ON.

(Before this PR specifying -DSANITIZERS=fuzzer without -DBUILD_FOR_FUZZING=ON would result in “multiple definition of main” errors, and now it won’t, but this is harmless. I originally thought triggering an error in this case was important to prevent binaries from being built that could skip proof of work checks, but just this was a misconception that came from confusing the BUILD_FUZZ_BINARY and BUILD_FOR_FUZZING variables.)

re: #31992#pullrequestreview-2713808140

Thanks for the review!

As ‘a separate “fuzzer_interface” with flags specifically intended to apply to the fuzz test binary’, it seems reasonable to place all <COMMAND>(fuzzer_interface ...) invocations under the existing if(BUILD_FUZZ_BINARY) condition.

Thanks, moved fuzzer_interface definition there.

nit: No need to quote variables here:

Thanks, applied suggested change.

The only question I have before final ACK: I failed to figure out the documented behaviour where both -fsanitize=fuzzer and -fsanitize=fuzzer-no-link are passed simultaneously to the compiler during linking. Any insights on that?

I don’t think you will be able to find explicit documentation on how they are combined but the options are documented at https://llvm.org/docs/LibFuzzer.html, where it says -fsanitize=fuzzer-no-link will “request just the instrumentation without linking” and -fsanitize=fuzzer “links in the libFuzzer’s main() symbol”. I think the only sensible way to combine the options is to provide both features, and this is what happens in practice. If this were not the case there should be pretty obvious linker errors, so I didn’t feel the need to separate the flags when there’s no problem, but if there was a problem they could be separated.

hebasto approved

hebasto commented at 11:03 am on March 26, 2025: member

ACK 57d8b1f1b3375452213c48ce80e8207e2fe53ebc, tested on Ubuntu 24.04.

TheCharlatan commented at 11:08 am on March 26, 2025: contributor

Concept ACK, but I’ll defer to the opinions of people actually dealing with the fuzzing infrastructure on a regular basis.

hebasto commented at 9:06 am on March 27, 2025: member

Friendly ping @dergoegge and @maflcko ;)

maflcko commented at 9:09 am on March 27, 2025: member

seems fine to do this, if all prior use cases and configurations continue to work fine without any changes or breakages

dergoegge commented at 3:22 pm on March 28, 2025: member

lgtm, afaiu this shouldn’t have any downsides to existing infra

hebasto merged this on Mar 29, 2025

hebasto closed this on Mar 29, 2025

cmake: Avoid fuzzer “multiple definition of `main’” errors #31992

Code Coverage & Benchmarks

Reviews

Conflicts