Skip to content

Commit 81f64f7

Browse files
fanquakePastaPastaPasta
authored andcommitted
Merge bitcoin#18413: script: prevent UB when computing abs value for num opcode serialize
2748e87 script: prevent UB when computing abs value for num opcode serialize (pierrenn) Pull request description: This was reported by practicalswift here bitcoin#18046 It seems that the original author of the line used a reference to glibc `abs`: https://github.com/lattera/glibc/blob/master/stdlib/abs.c However depending on some implementation details this can be undefined behavior for unusual values. A detailed explanation of the UB is provided here : https://stackoverflow.com/questions/17313579/is-there-a-safe-way-to-get-the-unsigned-absolute-value-of-a-signed-integer-with (by [Billy O'Neal](https://twitter.com/malwareminigun)) Simple relevant godbolt example : https://godbolt.org/z/yRwtCG Thanks! ACKs for top commit: sipa: ACK 2748e87 MarcoFalke: ACK 2748e87, only checked that the bitcoind binary does not change with clang -O2 🎓 practicalswift: ACK 2748e87 Tree-SHA512: 539a34c636c2674c66cb6e707d9d0dfdce63f59b5525610ed88da10c9a8d59d81466b111ad63b850660cef3750d732fc7755530c81a2d61f396be0707cd86dec
1 parent fa9e34c commit 81f64f7

File tree

3 files changed

+3
-11
lines changed

3 files changed

+3
-11
lines changed

src/script/script.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ class CScriptNum
331331

332332
std::vector<unsigned char> result;
333333
const bool neg = value < 0;
334-
uint64_t absvalue = neg ? -value : value;
334+
uint64_t absvalue = neg ? ~static_cast<uint64_t>(value) + 1 : static_cast<uint64_t>(value);
335335

336336
while(absvalue)
337337
{

src/test/fuzz/integer.cpp

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -141,11 +141,7 @@ FUZZ_TARGET_INIT(integer, initialize_integer)
141141

142142
const CScriptNum script_num{i64};
143143
(void)script_num.getint();
144-
// Avoid negation failure:
145-
// script/script.h:332:35: runtime error: negation of -9223372036854775808 cannot be represented in type 'int64_t' (aka 'long'); cast to an unsigned type to negate this value to itself
146-
if (script_num != CScriptNum{std::numeric_limits<int64_t>::min()}) {
147-
(void)script_num.getvch();
148-
}
144+
(void)script_num.getvch();
149145

150146
const arith_uint256 au256 = UintToArith256(u256);
151147
assert(ArithToUint256(au256) == u256);

src/test/fuzz/scriptnum_ops.cpp

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -123,10 +123,6 @@ FUZZ_TARGET(scriptnum_ops)
123123
script_num &= fuzzed_data_provider.ConsumeIntegral<int64_t>();
124124
});
125125
(void)script_num.getint();
126-
// Avoid negation failure:
127-
// script/script.h:332:35: runtime error: negation of -9223372036854775808 cannot be represented in type 'int64_t' (aka 'long'); cast to an unsigned type to negate this value to itself
128-
if (script_num != CScriptNum{std::numeric_limits<int64_t>::min()}) {
129-
(void)script_num.getvch();
130-
}
126+
(void)script_num.getvch();
131127
}
132128
}

0 commit comments

Comments
 (0)