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.

Sign up for GitHub

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

Jump to bottom

rpc, bugfix: Enforce maximum value for setmocktime #29869

Merged
merged 1 commit into from
Apr 15, 2024
Merged

rpc, bugfix: Enforce maximum value for setmocktime #29869

merged 1 commit into from
Apr 15, 2024

Conversation

dergoegge
Copy link
Member

The maximum value for our mocktime must be representable in nanoseconds, otherwise we end up with negative values returned from NodeClock::now().

Found through fuzzing:

$ echo "c2V0bW9ja3RpbWVcZTptYf9w/3NldG3///////////////9p////ZP///ymL//////89////Nv9L////////LXkBAABpAA==" | base64 --decode > rpc-8cab9148ab4418ebd1923c213e9d3fe9c9b49b39.crash
$ FUZZ=rpc ./src/test/fuzz/fuzz rpc-8cab9148ab4418ebd1923c213e9d3fe9c9b49b39.crash
fuzz_libfuzzer: util/time.cpp:28: static NodeClock::time_point NodeClock::now(): Assertion `ret > 0s' failed.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 15, 2024 •
edited

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK maflcko, brunoerg, glozow

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

maflcko
maflcko approved these changes Apr 15, 2024
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could combine the two error messages into one, as both are related to out-of-range?

src/rpc/node.cpp Outdated Show resolved Hide resolved
maflcko
maflcko approved these changes Apr 15, 2024
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, lgtm

src/rpc/node.cpp Outdated Show resolved Hide resolved
@maflcko
Copy link
Member

maflcko commented Apr 15, 2024

lgtm ACK 61df72e

@maflcko maflcko added Tests and removed Tests labels Apr 15, 2024
@dergoegge
Copy link
Member Author

Fixed rpc_uptime functional test

@maflcko
Copy link
Member

maflcko commented Apr 15, 2024

re-ACK c2e0489

brunoerg
brunoerg approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK c2e0489

glozow
glozow reviewed Apr 15, 2024
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK c2e0489

test/functional/rpc_uptime.py
@@ -23,7 +23,7 @@ def run_test(self):
self._test_uptime()

def _test_negative_time(self):
assert_raises_rpc_error(-8, "Mocktime cannot be negative: -1.", self.nodes[0].setmocktime, -1)
assert_raises_rpc_error(-8, "Mocktime must be in the range [0, 9223372036], not -1.", self.nodes[0].setmocktime, -1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a

assert_raises_rpc_error(-8, "Mocktime must be in the range [0, 9223372036], not 9223372037.", self.nodes[0].setmocktime, 9223372037)

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are changing this, it could also make sense to test that a full round trip of the value 9223372036 works.

@glozow glozow merged commit 07720b1 into bitcoin:master Apr 15, 2024
16 checks passed
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Apr 16, 2024
@dergoegge @fanquake
[rpc, bugfix] Enforce maximum value for setmocktime
0c34f12 
Github-Pull: bitcoin#29869
Rebased-From: c2e0489
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Apr 16, 2024
@dergoegge @fanquake
[rpc, bugfix] Enforce maximum value for setmocktime
897e5af 
Github-Pull: bitcoin#29869
Rebased-From: c2e0489
@fanquake fanquake mentioned this pull request Apr 16, 2024
Merged
@fanquake
Copy link
Member

Backported to 27.x in #29888.

glozow pushed a commit to glozow/bitcoin that referenced this pull request Apr 17, 2024
@dergoegge @glozow
[rpc, bugfix] Enforce maximum value for setmocktime
a81a922 
Github-Pull: bitcoin#29869
Rebased-From: c2e0489
@glozow glozow mentioned this pull request Apr 17, 2024
Merged
fanquake added a commit that referenced this pull request May 13, 2024
@fanquake
Merge #29888: [27.x] Backports
c7885ec 
bd5860b [WIP] doc: release notes for 27.x (fanquake)
475aac4 doc: add LLVM instruction for macOS < 13 (Sjors Provoost)
a995902 depends: Fix build of Qt for 32-bit platforms (laanwj)
0fcceef Fix #29767, set m_synced = true after Commit() (nanlour)
ae9a2ed sign: don't assume we are parsing a sane Miniscript (Antoine Poinsot)
a6a59cf rpc: Reword SighashFromStr error message (MarcoFalke)
364bf01 build: Fix false positive `CHECK_ATOMIC` test for clang-15 (Hennadii Stepanov)
9277793 test: Fix failing univalue float test (MarcoFalke)
5c09791 doc: archive 27.0 release notes (fanquake)
897e5af [rpc, bugfix] Enforce maximum value for setmocktime (dergoegge)
602cfd5 ci: Bump s390x to ubuntu:24.04 (MarcoFalke)
20e6e8d Change Luke Dashjr seed to dashjr-list-of-p2p-nodes.us (Luke Dashjr)
a6862c5 depends: fix mingw-w64 Qt DEBUG=1 build (fanquake)

Pull request description:

  Backports:
  * #29691
  * #29747
  * #29776
  * #29853
  * #29856
  * #29859
  * #29869
  * #29870
  * #29886
  * #29892
  * #29934
  * #29985

ACKs for top commit:
  willcl-ark:
    reACK bd5860b
  stickies-v:
    re-ACK bd5860b
  TheCharlatan:
    ACK bd5860b

Tree-SHA512: a1a40de70cf52b5fc01d9dcc772421751a18c6a48a726c4c05c0371c585a53a27902e17daed9e0d721ab7763c94bb32de05c146bd6bc73fd558edd08b31e8547
@glozow
Copy link
Member

glozow commented May 13, 2024

Backport for 26.x is in #29899

glozow added a commit that referenced this pull request May 24, 2024
@glozow
Merge #29899: [26.x] archive 26.1 release notes + backports
6d7a1e3 
aa7e876 [doc] add draft release notes for 26.2rc1 (glozow)
21d9aaa p2p, bugfix: detect addnode cjdns peers in GetAddedNodeInfo() (Jon Atack)
ec5ce2f windeploy: Renew certificate (Ava Chow)
96d0e81 rpc: Reword SighashFromStr error message (MarcoFalke)
6685aff rpc: move UniValue in blockToJSON (willcl-ark)
7f45e00 depends: Fix build of Qt for 32-bit platforms (laanwj)
f9b76ba ci: Pull in qtbase5-dev instead of seperate low-level libraries (laanwj)
c587753 doc: Suggest installing dev packages for debian/ubuntu qt5 build (laanwj)
7ecdb08 ci: Bump s390x to ubuntu:24.04 (MarcoFalke)
d9ef6cf sign: don't assume we are parsing a sane Miniscript (Antoine Poinsot)
e4859c8 depends: fix mingw-w64 Qt DEBUG=1 build (fanquake)
bb46b90 Fix #29767, set m_synced = true after Commit() (nanlour)
bf5b6fc Throw error if invalid parameters passed to getnetworkhashps RPC endpoint (Jameson Lopp)
a81a922 [rpc, bugfix] Enforce maximum value for setmocktime (dergoegge)
d39ea51 Change Luke Dashjr seed to dashjr-list-of-p2p-nodes.us (Luke Dashjr)
c21bbcc [doc] archive 26.1 release notes (glozow)

Pull request description:

  Archives 26.1 release notes and adds draft release notes for 26.2rc1

  Also backports:
  - #29691
  - #29869
  - #28554
  - #29747
  - #29853
  - #29856
  - #29764
  - #29776
  - #29985
  - #30094
  - #29870
  - #30149
  - #30085

ACKs for top commit:
  stickies-v:
    re-ACK aa7e876, only changes are fixing commit msg and transifex reference
  willcl-ark:
    ACK aa7e876

Tree-SHA512: b81ba6092640de696d782114cdf43e7ed1d63ea0a3231cade30653c2743d87700e0f852a1b1fcc42ae313b2d4f004e6026ddbad87d58c2fde0a660e90026ed98
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maflcko maflcko maflcko approved these changes

@glozow glozow glozow left review comments

@brunoerg brunoerg brunoerg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dergoegge @DrahtBot @maflcko @fanquake @glozow @brunoerg