The randomness in the path element is required to allow a single fuzz test to run in parallel. Previous releases used a uint256 random value, but 10 random bytes should be sufficient as well, while avoiding a MAX_PATH violation on Windows.
The issue was introduced by myself, by suggesting to use the current time in #31000 (review).
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31317.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator, tdb3, kevkevinpal, dergoegge |
| Stale ACK | furszy |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
This should fix https://issues.oss-fuzz.com/issues/379418970, which seems to be running on servers that do not have nanosecond precision:
libc++abi: terminating due to uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in remove_all: No such file or directory ["/tmp/test_common bitcoin/1731742732339168000/regtest/fuzzed_i2p_private_key"]
To test locally, one can reduce to microsecond precision (or less) via:
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 46a6daa88c..08aabbd026 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -138,7 +138,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
const std::string test_name{G_TEST_GET_FULL_NAME ? G_TEST_GET_FULL_NAME() : ""};
if (!m_node.args->IsArgSet("-testdatadir")) {
- const auto now{TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now())};
+ const auto now{TicksSinceEpoch<std::chrono::microseconds>(SystemClock::now())};
m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / util::ToString(now);
TryCreateDirectories(m_path_root);
} else {
Then running something like FUZZ=utxo_total_supply ./bld-cmake/src/test/fuzz/fuzz -runs=10 -jobs=2 -workers=2 should eventually crash.
146 | + // tests, such as the fuzz tests to run in several processes at the 147 | + // same time, add a random element to the path. Keep it small enough to 148 | + // avoid a MAX_PATH violation on Windows. 149 | + const auto rand{HexStr(g_rng_temp_path.randbytes(10))}; 150 | + m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / rand; 151 | TryCreateDirectories(m_path_root);
What about checking TryCreateDirectories too? And doing this in a loop until it creates a new dir?
Probably we might also need to lock the directory like we do for the custom datadir path.
loop
I am not sure that running tests (or the setup of them) in a loop until they pass is a good approach. Generally, when there is an issue, it should be addressed, instead of being silently hidden.
Probably we might also need to lock the directory like we do for the custom datadir path.
Maybe a follow-up can do this? The goal of this pull request is to unbreak OSS-Fuzz (not sure if any other deployment is affected) by simply restoring what has been done for all previous releases in the unit tests.
Probably we might also need to lock the directory like we do for the custom datadir path.
Maybe a follow-up can do this? The goal of this pull request is to unbreak OSS-Fuzz (not sure if any other deployment is affected) by simply restoring what has been done for all previous releases in the unit tests
Created this PR, let me know if you would prefer any changes or if you think my approach is incorrect! https://github.com/bitcoin/bitcoin/pull/31328
Code ACK fa80b08fef0eaa600339caa678fdf80a8aec3ce3
code review and light test ACK fa80b08fef0eaa600339caa678fdf80a8aec3ce3
Ran unit tests and benchmarks, and saw that the dirs generated were random.
tACK fa80b08
was able to reproduce the crash from #31317 (comment) locally on my machine. Reviewed and ran the code aswell and it looks good to me
74 | @@ -75,6 +75,8 @@ using node::VerifyLoadedChainstate; 75 | const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr; 76 | 77 | constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues. 78 | +/** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
Tested cherrypicking 8e5f8a4f775b93d30b86d28405886eea232cf875 from #30737 on top of this PR to forward RANDOM_CTX_SEED on inside test/fuzz/test_runner.py.
Ran this ~10 times:
RANDOM_CTX_SEED=21 build_fuzz/test/fuzz/test_runner.py ../qa-assets/
Many of the failures come down to:
Done 2 runs in 0 second(s)
terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
what(): filesystem error: cannot remove all: No such file or directory [/tmp/test_common bitcoin/02ccd0019e3e40fb6001] [/tmp/test_common bitcoin/02ccd0019e3e40fb6001/regtest]
==41419== ERROR: libFuzzer: deadly signal
This proves to me that the latter part of this comment is not true. I would therefore suggest one of:
m_rng directly instead of reintroducing g_rng_temp_path.g_rng_temp_path truly non-deterministic (maybe through reverting part of 97e16f57042cab07e5e73f6bed19feec2006e4f7, moving the seeding, haven't tested).g_rng_temp_path, so the other two alternatives would be preferable)./** Random context to get unique temp data dirs. */
Yes, this issue is re-introduced.
It can also be tested on top of https://github.com/bitcoin/bitcoin/commit/fa80b08fef0eaa600339caa678fdf80a8aec3ce3 via something like: RANDOM_CTX_SEED=21 FUZZ=utxo_total_supply ./bld-cmake/src/test/fuzz/fuzz -runs=10 -jobs=2 -workers=2
I didn't want to fix it, as it was pre-existing and no one complained (at least OSS-Fuzz did not), whereas OSS-Fuzz did complain about the fixed issue.
However, as this is a trivial fix, I've added another commit to do so.
Let me know if this is acceptable. If not, my recommendation would be to move it to a follow-up to not delay this fix further.
Having g_rng_temp_path_init be called as soon as this compilation unit is included in a binary isn't ideal but still acceptable (BasicTestingSetup may not be used in a given run).
Good call on removing the defunct comment before SeedRandomForTest() as well.
Having
g_rng_temp_path_initbe called as soon as this compilation unit is included in a binary isn't ideal but still acceptable (BasicTestingSetupmay not be used in a given run).
Yeah, I guess it should be fine to move the init inside the BasicTestingSetup constructor, as long as it is before the SeedRandomForTest. I am happy to review a follow-up doing that, but I'll leave this as-is for now, due to the three acks.
I like the robustness of the current solution. Never know where SeedRandomForTest might creep in inside unit/bench/fuzz. Please resolve this.
Reviewed fa80b08fef0eaa600339caa678fdf80a8aec3ce3
ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2
Tested cherrypicking 8e5f8a4f775b93d30b86d28405886eea232cf875 on top and re-running test from https://github.com/bitcoin/bitcoin/pull/31317/files#r1848404959, confirming that the second commit does in fact make paths random.
re ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2
re-ran unit tests and benchmarks, and saw that the dirs generated were random.
reACK https://github.com/bitcoin/bitcoin/commit/faaaf59f71ede057b2c1d369ef8db973c2f2dbc2
Ran fuzz tests with 8 jobs and 8 workers FUZZ=utxo_total_supply ./build_fuzz/src/test/fuzz/fuzz -jobs=8 -workers=8
and validated that the temp directorys at /tmp/test_common bitcoin/<random value> got created and deleted for each fuzz test.
ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2