Skip to content

Commit 6a11d9e

Browse files
committed
Merge #18433: serialization: prevent int overflow for big Coin::nHeight
e980214 serialization: prevent int overflow for big Coin::nHeight (pierrenn) Pull request description: This is an attempt to fix fuzzer issues 1,2,8 reported by practicalswift here : bitcoin/bitcoin#18046 The fuzzer harness doesn't prevent deserialization of unrealistic high values for `Coin::nHeight`. In the [provided examples](bitcoin/bitcoin#18046), we have : - `blockundo_deserialize` : the varint `0x8DD88DD700` is deserialized as `3944983552` in `Coin::nHeight` (`TxInOutFormatter::Unser`) - `coins_deserialize` : the varint `0x8DD5D5EC40` is deserialized as `3939874496` similarly - `txundo_deserialize`: the varint `0x8DCD828F01` is deserialized as `3921725441` in `Coin::nHeight` (`Coin::Unserialize`) Since `Coin::nHeight` is 31 bit long, multiplying a large value by 2 triggers the fuzzer. AFAIK those values are unrealistic (~70k years for the smallest..). I've looked a bit a reducing the range of values the fuzzer can deserialize, but this seems to be too much code change for not much. Hence this PR chooses to static cast `nHeight` when re-serializing; it seems to be the less intrusive/safest way to prevent the fuzzer output. Another more "upstream" approach would be to limit `Coin::nHeight` values to something more realistic, e.g. `0xFFFFFFF` (~5k years) : https://github.com/pierreN/bitcoin/blob/de3a30bab28e2db853a795017c5ec1704a1d0fee/src/undo.h#L39 and https://github.com/pierreN/bitcoin/blob/de3a30bab28e2db853a795017c5ec1704a1d0fee/src/coins.h#L71 Thanks ! NB: i was also not sure about the component/area to prefix the PR/commit with.. ? ACKs for top commit: practicalswift: ACK e980214 -- patch looks correct promag: ACK e980214. sipa: utACK e980214 MarcoFalke: re-ACK e980214 🎑 ryanofsky: Code review ACK e980214. Just removed ternary ? 1 : 0 and replaced / 2 with >> 1 since last review Tree-SHA512: 905fc9e5e52a6857abee4a1c863751767835965804bb8c39474f27a120f65399ff4ba7a49ef1da0ba565379f8c12095bd384b6c492cf06776f01b2db68d522b8
2 parents 5f9cd62 + e980214 commit 6a11d9e

File tree

2 files changed

+4
-4
lines changed

2 files changed

+4
-4
lines changed

src/coins.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ class Coin
5959
template<typename Stream>
6060
void Serialize(Stream &s) const {
6161
assert(!IsSpent());
62-
uint32_t code = nHeight * 2 + fCoinBase;
62+
uint32_t code = nHeight * uint32_t{2} + fCoinBase;
6363
::Serialize(s, VARINT(code));
6464
::Serialize(s, Using<TxOutCompression>(out));
6565
}

src/undo.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ struct TxInUndoFormatter
2424
{
2525
template<typename Stream>
2626
void Ser(Stream &s, const Coin& txout) {
27-
::Serialize(s, VARINT(txout.nHeight * 2 + (txout.fCoinBase ? 1u : 0u)));
27+
::Serialize(s, VARINT(txout.nHeight * uint32_t{2} + txout.fCoinBase ));
2828
if (txout.nHeight > 0) {
2929
// Required to maintain compatibility with older undo format.
3030
::Serialize(s, (unsigned char)0);
@@ -34,9 +34,9 @@ struct TxInUndoFormatter
3434

3535
template<typename Stream>
3636
void Unser(Stream &s, Coin& txout) {
37-
unsigned int nCode = 0;
37+
uint32_t nCode = 0;
3838
::Unserialize(s, VARINT(nCode));
39-
txout.nHeight = nCode / 2;
39+
txout.nHeight = nCode >> 1;
4040
txout.fCoinBase = nCode & 1;
4141
if (txout.nHeight > 0) {
4242
// Old versions stored the version number for the last spend of

0 commit comments

Comments
 (0)