-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
net: Avoid UBSan warning in ProcessMessage(...) #21043
Conversation
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.
review ACK 2a241c3
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).
patchcommit eea2bf75949fd3b391711b9092e01a1843245db2
Author: MarcoFalke <falke.marco@gmail.com>
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)) |
Good point! I've now added your patch as a second commit. Thanks! |
Signed-off-by: practicalswift <practicalswift@users.noreply.github.com>
3a61bd5
to
3ddbf22
Compare
re-ACK 3ddbf22 only change is adding patch written by me |
ACK 3ddbf22 -- code review only |
Github-Pull: bitcoin#21043 Rebased-From: f5f2f97
Signed-off-by: practicalswift <practicalswift@users.noreply.github.com> Github-Pull: bitcoin#21043 Rebased-From: 3ddbf22
Backported in #20901 |
…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
Avoid UBSan warning in
ProcessMessage(...)
.Context: #20380 (comment) (thanks Crypt-iQ!)