-
Notifications
You must be signed in to change notification settings - Fork 38.6k
fuzz: Abort if system time is called without mock time being set #31549
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: Abort if system time is called without mock time being set #31549
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31549. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
f0fc536 to
d482964
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
d482964 to
8cf5c4e
Compare
|
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK .
|
I am unable to crash the fuzz test when clang version: clang++ version: and I've used this command mentioned in docs for macos: Is there anything I am doing wrong?? |
@i-am-yuvi which target was it? It could be that running the test from scratch doesn't quickly reach code where You can find the corpora here: https://github.com/bitcoin-core/qa-assets I think it's also possible that it's machine specifc. The command above for |
8cf5c4e to
28e374f
Compare
Thanks, worked(crashed) on Linux with Corpus. I haven't tested on Mac due to storage issue, it would be interesting to see if that would crash on macos!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK fdc5224d205c69ebcb2d2b7bcd95386ef89181ca on x86_64
- removing
setmocktime ()from any one of the targets would cause a crash. - Not tested on macos
maflcko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you forgot to set it in the init function? Otherwise, the global state may be non-deterministic:
index e4e4723c74..3f3bfbeeaa 100644
--- a/src/test/fuzz/fuzz.cpp
+++ b/src/test/fuzz/fuzz.cpp
@@ -115,6 +115,7 @@ void initialize()
// - GetStrongRandBytes(), which is used for the creation of private key material.
// - Creating a BasicTestingSetup or derived class will switch to a random seed.
SeedRandomStateForTest(SeedRand::ZEROS);
+ //here
// Terminate immediately if a fuzzing harness ever tries to create a socket.
// Individual tests can override this by pointing CreateSock to a mocked alternative.|
Tested that no superfluous ones were added via: diffdiff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp
index e4e4723c74..eda5ff4cc6 100644
--- a/src/test/fuzz/fuzz.cpp
+++ b/src/test/fuzz/fuzz.cpp
@@ -78,11 +78,13 @@ void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target,
static std::string_view g_fuzz_target;
static const TypeTestOneInput* g_test_one_input{nullptr};
+bool g_ever_used_g_mocktime{false};
inline void test_one_input(FuzzBufferType buffer)
{
CheckGlobals check{};
(*Assert(g_test_one_input))(buffer);
+ g_ever_used_g_mocktime|= g_used_mocktime;
}
const std::function<std::string()> G_TEST_GET_FULL_NAME{[]{
@@ -274,6 +276,9 @@ int main(int argc, char** argv)
}
const auto end_time{Now<SteadySeconds>()};
std::cout << g_fuzz_target << ": succeeded against " << tested << " files in " << count_seconds(end_time - start_time) << "s." << std::endl;
+ if (tested && !g_ever_used_g_mocktime && g_set_mocktime) {
+ Assert(false); // remove unused SetMockTime()?
+ }
#endif
return 0;
}
diff --git a/src/test/fuzz/util/check_globals.cpp b/src/test/fuzz/util/check_globals.cpp
index f91a965afc..76d62fa3ca 100644
--- a/src/test/fuzz/util/check_globals.cpp
+++ b/src/test/fuzz/util/check_globals.cpp
@@ -17,6 +17,8 @@ struct CheckGlobalsImpl {
{
g_used_g_prng = false;
g_seeded_g_prng_zero = false;
+ g_set_mocktime= false;
+ g_used_mocktime=false;
g_used_system_time = false;
SetMockTime(0s);
}
diff --git a/src/test/fuzz/util/check_globals.h b/src/test/fuzz/util/check_globals.h
index 12d39c2daf..798a9ba506 100644
--- a/src/test/fuzz/util/check_globals.h
+++ b/src/test/fuzz/util/check_globals.h
@@ -10,6 +10,8 @@
#include <optional>
#include <string>
+extern std::atomic<bool> g_set_mocktime;
+extern std::atomic<bool> g_used_mocktime;
extern std::atomic<bool> g_used_system_time;
struct CheckGlobalsImpl;
diff --git a/src/util/time.cpp b/src/util/time.cpp
index 00f0f47392..f8f1eb5504 100644
--- a/src/util/time.cpp
+++ b/src/util/time.cpp
@@ -20,6 +20,8 @@
void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); }
static std::atomic<std::chrono::seconds> g_mock_time{}; //!< For testing
+std::atomic<bool> g_set_mocktime{false};
+std::atomic<bool> g_used_mocktime{false};
std::atomic<bool> g_used_system_time{false};
NodeClock::time_point NodeClock::now() noexcept
@@ -27,6 +29,8 @@ NodeClock::time_point NodeClock::now() noexcept
const auto mocktime{g_mock_time.load(std::memory_order_relaxed)};
if (!mocktime.count()) {
g_used_system_time = true;
+ }else{
+ g_used_mocktime=true;
}
const auto ret{
mocktime.count() ?
@@ -40,6 +44,7 @@ void SetMockTime(int64_t nMockTimeIn) { SetMockTime(std::chrono::seconds{nMockTi
void SetMockTime(std::chrono::seconds mock_time_in)
{
Assert(mock_time_in >= 0s);
+ g_set_mocktime=true;
g_mock_time.store(mock_time_in, std::memory_order_relaxed);
}
|
|
ACK 28e374f0a7dc1bda1c71e14eb896b2d9d2f23b48 🍭 Show signatureSignature: |
28e374f to
a96b84c
Compare
|
Thanks for the review @i-am-yuvi!
So I learned that it's likely this bitcoin/src/test/fuzz/util.cpp Lines 278 to 289 in 41a2ce9
that is preventing the load_external_block_file target from crashing on a mac.
|
Good catch, thank you. Added in ff21870 |
dergoegge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK a96b84c
brunoerg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crACK a96b84c
|
ACK a96b84c |
|
post-merge reACK a96b84c (only change is adding missing mocktime in init) |
This PR expands the
CheckGlobalsutility that was introduced in #31486 and should help with fuzz stability (#29018).System time shouldn't be used when running a fuzz test, as it is likely to introduce instability (non-determinism). This PR identifies and fixes the targets that were calling system time without setting mock time at the start of an iteration.
Removing
SetMockTime()from any one of these targets should result in a crash and a message describing the issue.