aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorWladimir J. van der Laan <laanwj@protonmail.com>2019-11-02 11:40:40 +0100
committerWladimir J. van der Laan <laanwj@protonmail.com>2019-11-02 11:40:56 +0100
commit9641366950276da88af626d0898676195df8d83a (patch)
tree6c3e6cdad6f13b20d344fb2c989efabb0a673dc2
parenta5224be6454140ad601347bd536c172b7b6f148b (diff)
parenta35b6824f3a0bdb68c5aef599c0f17562689970e (diff)
downloadbitcoin-9641366950276da88af626d0898676195df8d83a.tar.xz
Merge #17293: Add assertion to randrange that input is not 0
a35b6824f3a0bdb68c5aef599c0f17562689970e Add assertion to randrange that input is not 0 (Jeremy Rubin) Pull request description: From the comment in randrange, their is an implicit argument that randrange cannot accept an argument of 0. If the argument is 0, then we have to return {}, which is not possible in a uint64_t. The current code takes a very interesting approach, which is to return [0..std::numeric_limits<uint64_t>]. This can cause all sorts of fun problems, like allocating a lot of memory, accessing random memory (maybe with your private keys), and crashing the computer entirely. This gives us three choices of how to make it "safe": 1) return Optional<uint64_t> 2) Change the return type to [0..range] 3) Return 0 if 0 4) Assert(range) So which solution is best? 1) seems a bit overkill, as it makes any code using randrange worse. 2) Changing the return type as in 2 could be acceptable, but it imposes the potential overflow checking on the caller (which is what we want). 3) An interesting option -- effective makes the return type in {0} U [0..range]. But this is a bad choice, because it leads to code like `vec[randrange(vec.size())]`, which is incorrect for an empty vector. Null set should mean null set. 4) Assert(range) stands out as the best mitigation for now, with perhaps a future change to solution 2. It prevents the error from propagating at the earliest possible time, so the program crashes cleanly rather than by freezing the computer or accessing random memory. ACKs for top commit: instagibbs: Seems reasonable for now, ACK https://github.com/bitcoin/bitcoin/pull/17293/commits/a35b6824f3a0bdb68c5aef599c0f17562689970e laanwj: ACK a35b6824f3a0bdb68c5aef599c0f17562689970e promag: ACK a35b6824f3a0bdb68c5aef599c0f17562689970e. Tree-SHA512: 8fc626cde4b04b918100cb7af28753f25ec697bd077ce0e0c640be0357626322aeea233e3c8fd964ba1564b0fda830b7f5188310ebbb119c113513a4b89952dc
-rw-r--r--src/random.h1
1 files changed, 1 insertions, 0 deletions
diff --git a/src/random.h b/src/random.h
index 22801ec155..9d1f751773 100644
--- a/src/random.h
+++ b/src/random.h
@@ -166,6 +166,7 @@ public:
/** Generate a random integer in the range [0..range). */
uint64_t randrange(uint64_t range) noexcept
{
+ assert(range);
--range;
int bits = CountBits(range);
while (true) {