Avoid integer overflow in CheckDiskSpace #27235

dergoegge · 2023-03-09T17:33:22Z

Starting a fresh node with -prune=1 causes an integer overflow to happen in CheckDiskSpace (here) because nPruneTarget is to the max uint64_t value.

 node1 stderr util/system.cpp:138:51: runtime error: unsigned integer overflow: 52428800 + 18446744073709551615 cannot be represented in type 'unsigned long'
    #0 0x564a482b5088 in CheckDiskSpace(fs::path const&, unsigned long) src/./src/util/system.cpp:138:51
    #1 0x564a4728dc59 in AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/./src/init.cpp:1639:14
    #2 0x564a47256e6a in AppInit(node::NodeContext&, int, char**) src/./src/bitcoind.cpp:221:43
    #3 0x564a47256087 in main src/./src/bitcoind.cpp:265:13
    #4 0x7fcb7cbffd8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
    #5 0x7fcb7cbffe3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
    #6 0x564a471957f4 in _start (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0xca07f4) (BuildId: 035cb22302d37317a630900a15a26ecb326d395c)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow util/system.cpp:138:51 in

I think side stepping the overflow for this specific case, is better than adding an exception to the UB suppresions file.

DrahtBot · 2023-03-09T17:33:24Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke, john-moffett

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

martinus · 2023-03-09T17:42:25Z

How about setting nPruneTarget to something smaller, like std::numeric_limits<uint64_t>::max() / 2? That's still large enough

src/util/system.cpp

maflcko · 2023-03-10T10:43:48Z

https://cirrus-ci.com/task/5059018708746240

Not sure about referring to an URL that will auto-delete in 90 days. It would be better to at least copy-paste the relevant lines, or even better, add a test case.

vasild · 2023-03-10T10:59:05Z

Just for reference, like @MarcoFalke pointed above:

 node1 stderr util/system.cpp:138:51: runtime error: unsigned integer overflow: 52428800 + 18446744073709551615 cannot be represented in type 'unsigned long'
    #0 0x564a482b5088 in CheckDiskSpace(fs::path const&, unsigned long) src/./src/util/system.cpp:138:51
    #1 0x564a4728dc59 in AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/./src/init.cpp:1639:14
    #2 0x564a47256e6a in AppInit(node::NodeContext&, int, char**) src/./src/bitcoind.cpp:221:43
    #3 0x564a47256087 in main src/./src/bitcoind.cpp:265:13
    #4 0x7fcb7cbffd8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
    #5 0x7fcb7cbffe3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
    #6 0x564a471957f4 in _start (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0xca07f4) (BuildId: 035cb22302d37317a630900a15a26ecb326d395c)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow util/system.cpp:138:51 in

src/init.cpp

maflcko · 2023-03-10T14:51:04Z

Should be trivial to add a test, no?

maflcko · 2023-03-10T15:27:41Z

Introduced in 6630a1e so I guess no backport needed?

dergoegge · 2023-03-13T09:46:14Z

Should be trivial to add a test, no?

I don't want to add a new functional test just for this.
I tried to add a unit test but all our testing setups don't use the actual init logic.
Maybe it can be added to one of the existing functional tests that do pruning, but I don't know which one. I tried but it always ends up being a bigger change to the test than I think would be justified for this trivial change.

If you know an easy way to add a test let me know, happy to add it.

dergoegge · 2023-03-13T10:21:57Z

How about setting nPruneTarget to something smaller, like std::numeric_limits<uint64_t>::max() / 2? That's still large enough

@martinus That wouldn't quite work because CheckDiskSpace would return false then, given that std::numeric_limits<uint64_t>::max() / 2 + 50MB would likely not be available in free disk space on any system (https://github.com/bitcoin/bitcoin/blob/1884b71b1dc1fe463a2f00e61bcc56e4720cbc69/src/util/system.cpp#LL138). Currently the overflow works out because it just ends up checking that 50MB are available.

maflcko · 2023-03-13T11:10:18Z

All you need to do to add a test is adding a call to start a node with the prune=1 arg. For example:

diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py
index 7a0cedb1f5..8e086cf268 100755
--- a/test/functional/rpc_blockchain.py
+++ b/test/functional/rpc_blockchain.py
@@ -69,6 +69,7 @@ class BlockchainTest(BitcoinTestFramework):
 
     def run_test(self):
         self.wallet = MiniWallet(self.nodes[0])
+        self._test_prune_disk_space()
         self.mine_chain()
         self._test_max_future_block_time()
         self.restart_node(
@@ -100,6 +101,20 @@ class BlockchainTest(BitcoinTestFramework):
             self.generate(self.wallet, 1)
         assert_equal(self.nodes[0].getblockchaininfo()['blocks'], HEIGHT)
 
+    def _test_prune_disk_space(self):
+        self.stop_node(0)
+        self.log.info("Avoid integer overflow")  
+        from test_framework.test_node import ErrorMatch 
+        self.nodes[0].assert_start_raises_init_error(
+            extra_args=["-prune=1"],
+            match=ErrorMatch.PARTIAL_REGEX,
+            expected_msg="runtime error: unsigned integer overflow: 52428800 \+ 18446744073709551615 cannot be represented in type 'unsigned long'",
+        )
+        self.log.info("Avoid warning when assumed chain size is enough")  
+        self.start_node(0, extra_args=["-prune=123456789"])
+        self.stop_node(0, expected_stderr="may not accommodate the block files. Approximately 0 GB of data will be stored in this directory.")
+        assert False
+
     def _test_max_future_block_time(self):
         self.stop_node(0)
         self.log.info("A block tip of more than MAX_FUTURE_BLOCK_TIME in the future raises an error")

maflcko

lgtm

test/functional/rpc_blockchain.py

maflcko · 2023-03-13T12:27:02Z

test/functional/rpc_blockchain.py

+        self.log.info("Test that a manually pruned node does not run into "
+                      "integer overflow on first start up")
+        self.restart_node(0, extra_args=["-prune=1"])
+        self.log.info("Avoid warning when assumed chain size is enough")


Suggested change

self.log.info("Avoid warning when assumed chain size is enough")

self.log.info("Test that no warning appears when assumed chain size fits on disk, but the prune target would not")

nit (if you retouch)

maflcko

ACK 05eeba2 🥝

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 05eeba2c5fb312e0e6a730b01eb7d1b422d75dbb 🥝
ldqnydo8QCYst0rz5DHKllo5bSa1wtQwO0yxrHDUxZFuBdkdTde53ru7wlcgYhRrGK2WIJyKmNEgbQMF7paMCg==

john-moffett

ACK 05eeba2

05eeba2 [test] Add manual prune startup test case (dergoegge) 4517419 [util] Avoid integer overflow in CheckDiskSpace (dergoegge) Pull request description: Starting a fresh node with `-prune=1` causes an integer overflow to happen in `CheckDiskSpace` ([here](https://github.com/bitcoin/bitcoin/blob/f7bdcfc83f5753349018be3b5a663c8923d1a5eb/src/init.cpp#L1633-L1648)) because `nPruneTarget` is to the max `uint64_t` value. ``` node1 stderr util/system.cpp:138:51: runtime error: unsigned integer overflow: 52428800 + 18446744073709551615 cannot be represented in type 'unsigned long' #0 0x564a482b5088 in CheckDiskSpace(fs::path const&, unsigned long) src/./src/util/system.cpp:138:51 #1 0x564a4728dc59 in AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/./src/init.cpp:1639:14 #2 0x564a47256e6a in AppInit(node::NodeContext&, int, char**) src/./src/bitcoind.cpp:221:43 #3 0x564a47256087 in main src/./src/bitcoind.cpp:265:13 #4 0x7fcb7cbffd8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d) #5 0x7fcb7cbffe3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d) #6 0x564a471957f4 in _start (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0xca07f4) (BuildId: 035cb22302d37317a630900a15a26ecb326d395c) SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow util/system.cpp:138:51 in ``` I think side stepping the overflow for this specific case, is better than adding an exception to the UB suppresions file. ACKs for top commit: MarcoFalke: ACK 05eeba2 🥝 john-moffett: ACK 05eeba2 Tree-SHA512: 1d8e6bcb49818139f04b5ab2cbef7f9b422bf0c38a804cd532b6bd0ba4c4fd07f959ba977e59896343f213086c8ecc48180f50d006638dc84649c66ec379d58a

vasild · 2023-03-16T13:27:38Z

ACK 05eeba2

vasild reviewed Mar 9, 2023

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

dergoegge marked this pull request as draft March 9, 2023 18:24

maflcko reviewed Mar 10, 2023

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

dergoegge force-pushed the 2023-03-system-ub branch from e3ec6d7 to 4164b1f Compare March 10, 2023 14:44

dergoegge marked this pull request as ready for review March 10, 2023 14:44

maflcko reviewed Mar 10, 2023

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

[util] Avoid integer overflow in CheckDiskSpace

4517419

dergoegge force-pushed the 2023-03-system-ub branch from 4164b1f to 4517419 Compare March 10, 2023 14:51

maflcko approved these changes Mar 13, 2023

View reviewed changes

test/functional/rpc_blockchain.py Outdated Show resolved Hide resolved

test/functional/rpc_blockchain.py Show resolved Hide resolved

[test] Add manual prune startup test case

05eeba2

dergoegge force-pushed the 2023-03-system-ub branch from 7225dcb to 05eeba2 Compare March 13, 2023 12:10

maflcko reviewed Mar 13, 2023

View reviewed changes

maflcko approved these changes Mar 13, 2023

View reviewed changes

john-moffett approved these changes Mar 13, 2023

View reviewed changes

glozow merged commit f50fb17 into bitcoin:master Mar 13, 2023

bitcoin locked and limited conversation to collaborators Mar 15, 2024

	self.log.info("Avoid warning when assumed chain size is enough")
	self.log.info("Test that no warning appears when assumed chain size fits on disk, but the prune target would not")

Avoid integer overflow in CheckDiskSpace #27235

Avoid integer overflow in CheckDiskSpace #27235

Uh oh!

Conversation

dergoegge commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

martinus commented Mar 9, 2023

Uh oh!

Uh oh!

Uh oh!

maflcko commented Mar 10, 2023

Uh oh!

vasild commented Mar 10, 2023

Uh oh!

Uh oh!

maflcko commented Mar 10, 2023

Uh oh!

maflcko commented Mar 10, 2023

Uh oh!

dergoegge commented Mar 13, 2023

Uh oh!

dergoegge commented Mar 13, 2023

Uh oh!

maflcko commented Mar 13, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Mar 16, 2023

Uh oh!

Uh oh!

dergoegge commented Mar 9, 2023 •

edited

Loading

DrahtBot commented Mar 9, 2023 •

edited

Loading