net: Avoid UBSan warning in ProcessMessage(...)#21043
Merged
maflcko merged 2 commits intobitcoin:masterfrom Feb 11, 2021
Merged
net: Avoid UBSan warning in ProcessMessage(...)#21043maflcko merged 2 commits intobitcoin:masterfrom
maflcko merged 2 commits intobitcoin:masterfrom
Conversation
maflcko
approved these changes
Feb 1, 2021
Member
maflcko
left a comment
There was a problem hiding this comment.
review ACK 2a241c31169e849eecb06c3bb5794fdd6ab081bb
This assumes that time is negatable, which holds for system time, but not for mocktime?
I think we should disallow negative mocktime (either here or in a follow up).
Member
patchcommit eea2bf75949fd3b391711b9092e01a1843245db2
Author: MarcoFalke <[email protected]>
Date: Mon Feb 1 08:03:24 2021 +0100
util: Disallow negative mocktime
diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp
index b75a7b8d26..38a0bddddb 100644
--- a/src/rpc/misc.cpp
+++ b/src/rpc/misc.cpp
@@ -365,13 +365,13 @@ static RPCHelpMan signmessagewithprivkey()
static RPCHelpMan setmocktime()
{
return RPCHelpMan{"setmocktime",
- "\nSet the local time to given timestamp (-regtest only)\n",
- {
- {"timestamp", RPCArg::Type::NUM, RPCArg::Optional::NO, UNIX_EPOCH_TIME + "\n"
- " Pass 0 to go back to using the system time."},
- },
- RPCResult{RPCResult::Type::NONE, "", ""},
- RPCExamples{""},
+ "\nSet the local time to given timestamp (-regtest only)\n",
+ {
+ {"timestamp", RPCArg::Type::NUM, RPCArg::Optional::NO, UNIX_EPOCH_TIME + "\n"
+ "Pass 0 to go back to using the system time."},
+ },
+ RPCResult{RPCResult::Type::NONE, "", ""},
+ RPCExamples{""},
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
{
if (!Params().IsMockableChain()) {
@@ -386,7 +386,10 @@ static RPCHelpMan setmocktime()
LOCK(cs_main);
RPCTypeCheck(request.params, {UniValue::VNUM});
- int64_t time = request.params[0].get_int64();
+ const int64_t time{request.params[0].get_int64()};
+ if (time < 0) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Mocktime can not be negative: %s.", time));
+ }
SetMockTime(time);
if (request.context.Has<NodeContext>()) {
for (const auto& chain_client : request.context.Get<NodeContext>().chain_clients) {
diff --git a/src/util/time.cpp b/src/util/time.cpp
index e96972fe12..d130e4e4d4 100644
--- a/src/util/time.cpp
+++ b/src/util/time.cpp
@@ -9,6 +9,8 @@
#include <util/time.h>
+#include <util/check.h>
+
#include <atomic>
#include <boost/date_time/posix_time/posix_time.hpp>
#include <ctime>
@@ -18,7 +20,7 @@
void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); }
-static std::atomic<int64_t> nMockTime(0); //!< For unit testing
+static std::atomic<int64_t> nMockTime(0); //!< For testing
int64_t GetTime()
{
@@ -46,6 +48,7 @@ template std::chrono::microseconds GetTime();
void SetMockTime(int64_t nMockTimeIn)
{
+ Assert(nMockTimeIn >= 0);
nMockTime.store(nMockTimeIn, std::memory_order_relaxed);
}
diff --git a/test/functional/rpc_uptime.py b/test/functional/rpc_uptime.py
index e86f91b1d0..6177970872 100755
--- a/test/functional/rpc_uptime.py
+++ b/test/functional/rpc_uptime.py
@@ -10,6 +10,7 @@ Test corresponds to code in rpc/server.cpp.
import time
from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_raises_rpc_error
class UptimeTest(BitcoinTestFramework):
@@ -18,8 +19,12 @@ class UptimeTest(BitcoinTestFramework):
self.setup_clean_chain = True
def run_test(self):
+ self._test_negative_time()
self._test_uptime()
+ def _test_negative_time(self):
+ assert_raises_rpc_error(-8, "Mocktime can not be negative: -1.", self.nodes[0].setmocktime, -1)
+
def _test_uptime(self):
wait_time = 10
self.nodes[0].setmocktime(int(time.time() + wait_time)) |
Contributor
Author
Good point! I've now added your patch as a second commit. Thanks! |
Signed-off-by: practicalswift <[email protected]>
3a61bd5 to
3ddbf22
Compare
Member
|
re-ACK 3ddbf22 only change is adding patch written by me |
Contributor
|
ACK 3ddbf22 -- code review only |
vasild
reviewed
Feb 5, 2021
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Feb 11, 2021
Github-Pull: bitcoin#21043 Rebased-From: f5f2f97
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Feb 11, 2021
Signed-off-by: practicalswift <[email protected]> Github-Pull: bitcoin#21043 Rebased-From: 3ddbf22
Member
|
Backported in #20901 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Feb 11, 2021
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jul 22, 2022
…time Summary: This is a backport of [[bitcoin/bitcoin#21043 | core#21043]] Most of the work was already done in D6022. The functional test already exists in abc_rpc_mocktime.py, only the error messages need to be updated. Test Plan: ``` ninja all check-all src/bitcoin-cli help setmocktime ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11788
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Avoid UBSan warning in
ProcessMessage(...).Context: #20380 (comment) (thanks Crypt-iQ!)