Skip to content
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: Limit fuzzed time to years 2000-2100 #23992

Merged
merged 1 commit into from Jan 17, 2022

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jan 6, 2022

It doesn't make sense to fuzz times in the past, as Bitcoin Core will refuse to start in the past.

Fix that and also remove a sanitizer suppression, which would be hit in net_processing in ProcessMessage:

             if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60)
                 addr.nTime = nNow - 5 * 24 * 60 * 60; // <-- Here 

This changes the format of fuzz inputs. Previously a time value was (de)serialized as 40 bytes, now it is 32 bytes.

@DrahtBot DrahtBot added the Tests label Jan 6, 2022
@mzumsande
Copy link
Contributor

Apperently this is not enough to remove the sanitizer suppression, see fuzzer fails for large values - the problem is that nTime is of type uint32_t, which can't go beyond the year 2106, whereas the fuzzed nNow is int64_t.

@maflcko maflcko changed the title fuzz: Set min fuzzed time to y2k fuzz: Limit fuzzed time to years 2000-2100 Jan 10, 2022
@maflcko
Copy link
Member Author

maflcko commented Jan 10, 2022

Thanks, done!

@mzumsande
Copy link
Contributor

Code Review ACK fa72383

I think we don't lose a lot of generality by restricting ConsumeTime() like this, since it basically used for setting the mock time, where times outside the range 2000-2100 aren't realistic.
Being able to remove the sanitizer suppression for all of net_processing seems more important.

@maflcko
Copy link
Member Author

maflcko commented Jan 11, 2022

2000-2100 aren't realistic

Indeed. For now. If someone fixes the 2106 bug, then at the same time the fuzzed range can be extended.

@maflcko maflcko merged commit 7de2cf9 into bitcoin:master Jan 17, 2022
@maflcko maflcko deleted the 2201-fuzzTimeMin branch January 17, 2022 07:46
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 18, 2022
fa72383 fuzz: Limit fuzzed time to years 2000-2100 (MarcoFalke)

Pull request description:

  It doesn't make sense to fuzz times in the past, as Bitcoin Core will refuse to start in the past.

  Fix that and also remove a sanitizer suppression, which would be hit in net_processing in `ProcessMessage`:

  ```cpp

               if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60)
                   addr.nTime = nNow - 5 * 24 * 60 * 60; // <-- Here
  ```

  This changes the format of fuzz inputs. Previously a time value was (de)serialized as 40 bytes, now it is 32 bytes.

ACKs for top commit:
  mzumsande:
    Code Review ACK fa72383

Tree-SHA512: ca6e7233beec2d9ef9fd481d8f1331942a4d2c8fe518b857629bebcc53a4f42ae123b994cf5d359384a0a8022098ff5a9c146600bc2593c6d88734e25bc240ad
@bitcoin bitcoin locked and limited conversation to collaborators Jan 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants