-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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
fuzz: Link all targets once #20560
fuzz: Link all targets once #20560
Conversation
Concept ACK, under the condition that we can do some basic testing that this doesn't meaningfully affect the speed at which fuzzers find issues. |
The cost is dereferencing one pointer (to a function). I highly doubt that this affects performance, but I am happy to test. |
That seems entirely reasonable, and is my expectation too. But given the pervasive "one binary per test" recommendation, I'd rather make sure. |
fa537b5
to
faec1c1
Compare
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
@RandyMcMillan Good catch on macOS. Should be fixed now (hopefully) |
🕵️ @achow101 @sipa @practicalswift @harding have been requested to review this pull request as specified in the REVIEWERS file. |
I'm very much in favor of this with regard to organization. The large number of binaries was bothering me. That said, I know nothing about fuzzing, so only ACK if it doesn't make fuzzing less useful. |
faec1c1
to
fa2185d
Compare
BenchmarksCompilationMeasured was wall clock time and disk usage of the full build pipeline with a warm ccache:
Running CIMeasured was the time to iterate over all seeds, typically done by CI: ./test/fuzz/test_runner.py -j 9
Coverage
Coverage decreased slightly because the tinyformat fuzzer had to be renamed, should recover once the seed dir is renamed as well. Generating seedsMeasured was the number of iterations required to find a synthetic bug, typically done on a fuzzing farm (dedicated hardware). The bug used: diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index ec5400c3d8..f15a7a990a 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2696,6 +2696,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
return;
} else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
AddTxAnnouncement(pfrom, gtxid, current_time);
+ Assert(false);
}
} else {
LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());
|
fa2185d
to
fac1885
Compare
benchmarks updated with images |
Doing a benchmark as well. Error introducing patch (similar to a bug I had during development of #19988): diff --git a/src/txrequest.cpp b/src/txrequest.cpp
index e54c073328..8a68e4fd8a 100644
--- a/src/txrequest.cpp
+++ b/src/txrequest.cpp
@@ -553,16 +553,17 @@ public:
// In other words, the situation where std::next(it) is deleted can only occur if std::next(it)
// belongs to a different peer but the same txhash as 'it'. This is covered by the first bulletpoint
// already, and we'll have set it_next to end().
- auto it_next = (std::next(it) == index.end() || std::next(it)->m_peer != peer) ? index.end() :
- std::next(it);
// If the announcement isn't already COMPLETED, first make it COMPLETED (which will mark other
// CANDIDATEs as CANDIDATE_BEST, or delete all of a txhash's announcements if no non-COMPLETED ones are
// left).
if (MakeCompleted(m_index.project<ByTxHash>(it))) {
// Then actually delete the announcement (unless it was already deleted by MakeCompleted).
- Erase<ByPeer>(it);
+ it = Erase<ByPeer>(it);
+ } else {
+ it = std::next(it);
}
- it = it_next;
}
} Number of iterations until crash is detected (avg +- stddev measured over 250000+ crashes for each): |
Updated my numbers. It appears that there is a (statistically) significant difference in iteration count, but for -use_value_profile=0 it's in the other direction than -use_value_profile=1. So I'm going to assume it's just due to arbitrary alignment changes in the binary or so. Concept ACK. I'm not concerned anymore about the impact on fuzzing speed. Will review the code changes soon. |
Began looking over the code. Concept ACK. |
Thanks for adding Concept ACK Will review. |
Tested ACK fa13e1b Great work! Thanks! ❤️
|
@sipa Mind to re-ACK? |
ACK fa13e1b. Reviewed the code changes, and tested the 3 different test_runner.py modes (run once, merge, generate). I also tested building with the new --enable-danger-fuzz-link-all As a potential follow-up, if there is interesting in building the separate-binary strategy faster it may be useful to instead of modifying the source code in place, make the script create copies of fuzz.cpp, plus an alternative Makefile to build them all. That'd avoid messing with the source tree, and permit building all the target binaries in parallel. |
Just for reference (not sure if you noticed during review), one of the targets had to be renamed, which is why I also pushed bitcoin-core/qa-assets@70083a8 |
Is it expected behaviour that
no longer works, and now requires the The error I'm seeing during
|
It's late and I could be misremembering but I think that flag was always needed. |
@dongcarl I think this is a bug (typo). Functions with a body in a header file need to be So you might be able to fix it by adding |
d8b9cec inline non-member functions with body in fuzzing headers (Patrick Strateman) Pull request description: Resolves the issue noted [here](bitcoin/bitcoin#20560 (comment)) ACKs for top commit: MarcoFalke: ACK d8b9cec Tree-SHA512: fb34707e2d2c5b664d4160e0e4b56e3df9fb2c9045da6ddea7139e0b4982262c4e085812a8543a6221febc9cd0815423b8287fec66baae3236e5f3339cc9df8c
… headers d8b9cec inline non-member functions with body in fuzzing headers (Patrick Strateman) Pull request description: Resolves the issue noted [here](bitcoin#20560 (comment)) ACKs for top commit: MarcoFalke: ACK d8b9cec Tree-SHA512: fb34707e2d2c5b664d4160e0e4b56e3df9fb2c9045da6ddea7139e0b4982262c4e085812a8543a6221febc9cd0815423b8287fec66baae3236e5f3339cc9df8c
… config 0dade91 fuzz: remove no-longer-necessary packages from fuzzbuzz config (fanquake) Pull request description: I take it this is actively being used, given [comments in #20560](bitcoin/bitcoin#20560 (comment)); so remove old dependencies from setup. ACKs for top commit: practicalswift: ACK 0dade91 Tree-SHA512: 781466776575e6051d0dddf4101bd057e484648f63e8e967240fefbf4b5832cacda6f6543708a0c368214a1efe0d60d371da78d7a920646cb93f1a4752aaf639
…uzzbuzz config 0dade91 fuzz: remove no-longer-necessary packages from fuzzbuzz config (fanquake) Pull request description: I take it this is actively being used, given [comments in bitcoin#20560](bitcoin#20560 (comment)); so remove old dependencies from setup. ACKs for top commit: practicalswift: ACK 0dade91 Tree-SHA512: 781466776575e6051d0dddf4101bd057e484648f63e8e967240fefbf4b5832cacda6f6543708a0c368214a1efe0d60d371da78d7a920646cb93f1a4752aaf639
…uzzbuzz config 0dade91 fuzz: remove no-longer-necessary packages from fuzzbuzz config (fanquake) Pull request description: I take it this is actively being used, given [comments in bitcoin#20560](bitcoin#20560 (comment)); so remove old dependencies from setup. ACKs for top commit: practicalswift: ACK 0dade91 Tree-SHA512: 781466776575e6051d0dddf4101bd057e484648f63e8e967240fefbf4b5832cacda6f6543708a0c368214a1efe0d60d371da78d7a920646cb93f1a4752aaf639
…uzzbuzz config 0dade91 fuzz: remove no-longer-necessary packages from fuzzbuzz config (fanquake) Pull request description: I take it this is actively being used, given [comments in bitcoin#20560](bitcoin#20560 (comment)); so remove old dependencies from setup. ACKs for top commit: practicalswift: ACK 0dade91 Tree-SHA512: 781466776575e6051d0dddf4101bd057e484648f63e8e967240fefbf4b5832cacda6f6543708a0c368214a1efe0d60d371da78d7a920646cb93f1a4752aaf639
…uzzbuzz config 0dade91 fuzz: remove no-longer-necessary packages from fuzzbuzz config (fanquake) Pull request description: I take it this is actively being used, given [comments in bitcoin#20560](bitcoin#20560 (comment)); so remove old dependencies from setup. ACKs for top commit: practicalswift: ACK 0dade91 Tree-SHA512: 781466776575e6051d0dddf4101bd057e484648f63e8e967240fefbf4b5832cacda6f6543708a0c368214a1efe0d60d371da78d7a920646cb93f1a4752aaf639
…uzzbuzz config 0dade91 fuzz: remove no-longer-necessary packages from fuzzbuzz config (fanquake) Pull request description: I take it this is actively being used, given [comments in bitcoin#20560](bitcoin#20560 (comment)); so remove old dependencies from setup. ACKs for top commit: practicalswift: ACK 0dade91 Tree-SHA512: 781466776575e6051d0dddf4101bd057e484648f63e8e967240fefbf4b5832cacda6f6543708a0c368214a1efe0d60d371da78d7a920646cb93f1a4752aaf639
…uzzbuzz config 0dade91 fuzz: remove no-longer-necessary packages from fuzzbuzz config (fanquake) Pull request description: I take it this is actively being used, given [comments in bitcoin#20560](bitcoin#20560 (comment)); so remove old dependencies from setup. ACKs for top commit: practicalswift: ACK 0dade91 Tree-SHA512: 781466776575e6051d0dddf4101bd057e484648f63e8e967240fefbf4b5832cacda6f6543708a0c368214a1efe0d60d371da78d7a920646cb93f1a4752aaf639
…uzzbuzz config 0dade91 fuzz: remove no-longer-necessary packages from fuzzbuzz config (fanquake) Pull request description: I take it this is actively being used, given [comments in bitcoin#20560](bitcoin#20560 (comment)); so remove old dependencies from setup. ACKs for top commit: practicalswift: ACK 0dade91 Tree-SHA512: 781466776575e6051d0dddf4101bd057e484648f63e8e967240fefbf4b5832cacda6f6543708a0c368214a1efe0d60d371da78d7a920646cb93f1a4752aaf639
…uzzbuzz config 0dade91 fuzz: remove no-longer-necessary packages from fuzzbuzz config (fanquake) Pull request description: I take it this is actively being used, given [comments in bitcoin#20560](bitcoin#20560 (comment)); so remove old dependencies from setup. ACKs for top commit: practicalswift: ACK 0dade91 Tree-SHA512: 781466776575e6051d0dddf4101bd057e484648f63e8e967240fefbf4b5832cacda6f6543708a0c368214a1efe0d60d371da78d7a920646cb93f1a4752aaf639
Currently the linker is invoked more than 150 times when compiling with
--enable-fuzz
. This is problematic for several reasons:buffer
or assume thebuffer
to be concatenations of seeds, which increases complexity of seeds and complexity for the fuzz engine to explore; Thus reducing the effectiveness of the affected fuzz targetsFixes #20088