util: Avoid invalid integer negation in FormatMoney and ValueFromAmount by practicalswift · Pull Request #20406 · bitcoin/bitcoin

practicalswift · 2020-11-16T20:38:19Z

Avoid invalid integer negation in FormatMoney and ValueFromAmount.

Fixes #20402.

Before this patch:

$ CC=clang CXX=clang++ ./configure --with-sanitizers=undefined
$ make -C src/ test/test_bitcoin
$ src/test/test_bitcoin -t rpc_tests/rpc_format_monetary_values -t util_tests/util_FormatMoney
core_write.cpp:21:29: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount'
  (aka 'long'); cast to an unsigned type to negate this value to itself
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior core_write.cpp:21:29 in 
test/rpc_tests.cpp(186): error: in "rpc_tests/rpc_format_monetary_values": 
  check ValueFromAmount(std::numeric_limits<CAmount>::min()).write() == "-92233720368.54775808" has failed 
  [--92233720368.-54775808 != -92233720368.54775808]
util/moneystr.cpp:16:34: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount'
  (aka 'long'); cast to an unsigned type to negate this value to itself
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior util/moneystr.cpp:16:34 in 
test/util_tests.cpp(1188): error: in "util_tests/util_FormatMoney":
  check FormatMoney(std::numeric_limits<CAmount>::min()) == "-92233720368.54775808" has failed 
  [--92233720368.-54775808 != -92233720368.54775808]

After this patch:

$ CC=clang CXX=clang++ ./configure --with-sanitizers=undefined
$ make -C src/ test/test_bitcoin
$ src/test/test_bitcoin -t rpc_tests/rpc_format_monetary_values -t util_tests/util_FormatMoney

laanwj · 2020-11-19T15:28:19Z

I think something is wrong if such a large value is passed in the first place. But the code change looks correct to me anyhow.

practicalswift · 2020-11-19T16:05:51Z

@laanwj I agree it should be rare, but there is at least one real world scenario where such an extreme value can be passed without it being a bug at the call site. In PrioritiseTransaction we call FormatMoney(nFeeDelta) on fee deltas. Fee deltas are allowed to be outside of the normal MoneyRange(…) as pointed out by luke-jr in another PR.

AFAICT we don't put a lower or upper bound on fee deltas (besides being CAmount of course), so I thought it was safest to define FormatMoney and ValueFromAmount for the entire CAmount including the std::numeric_limits<CAmount>::min() corner case :)

Some additional context: #20383 (comment) :)

src/core_write.cpp

RandyMcMillan · 2020-11-20T06:45:00Z

I wasn't able to find a test that covered the negative COIN value scenarios.
Wouldn't It would make sense to test those scenarios since they show up on a basic truth table?

n % COIN

 n %  COIN -->   1  
 n % -COIN -->   1  
-n %  COIN -->  -1  
-n % -COIN -->  -1

src/core_write.cpp

practicalswift · 2020-11-20T22:00:56Z

@RandyMcMillan I'm not sure I can think of a scenario when the constant COIN would be <= 0 but I've added a static_assert to make the assumption of COIN > 0 crystal clear for readers of the code :)

RandyMcMillan · 2020-11-20T22:12:23Z

Thanks - I was just thinking thru different scenarios for testing. Negative moduli aren't something that comes up very often. :)

DrahtBot · 2020-11-21T04:29:53Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: introduce setfeerate (an improved settxfee, in sat/vB) #20391 (wallet: introduce setfeerate (an improved settxfee, in sat/vB) by jonatack)
rpc: deprecate addresses and reqSigs from rpc outputs #20286 (rpc: deprecate addresses and reqSigs from rpc outputs by mjdietzx)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/util/moneystr.cpp

laanwj · 2020-11-23T15:12:45Z

Thanks. Code review ACK 8d9979e04afdb081d923f79b581586330968ff56

luke-jr

utACK, but... is there a reason we don't just leave the sign in quotient?

practicalswift · 2020-11-25T08:44:15Z

@luke-jr Do you mean in FormatMoney? Leaving the sign in quotient would change the output compared to how it works today, no? This PR is meant as a pure refactoring (aside from the avoidance of the invalid integer negation): I don't want to introduce any change to the output format in this PR.

luke-jr · 2020-11-25T15:28:41Z

Right now, we're checking amount < 0 explicitly and prefixing -. If we simply don't negate quotient, strprintf will do the - for us...

practicalswift · 2020-11-26T09:34:38Z

@luke-jr I'll let others chime in, and I'll happily adjust to the consensus opinion regarding the suggested extra refactoring :)

maflcko

review ack

src/core_write.cpp

src/test/rpc_tests.cpp

src/util/moneystr.cpp

src/core_write.cpp

practicalswift · 2020-11-30T16:05:41Z

Thanks for reviewing! Nits addressed (+ rebase: sorry!). Please re-review :)

src/core_io.h

practicalswift · 2020-12-02T14:57:17Z

When fuzzing the RPC interface I stumbled upon this case again: decoderawtransaction ff0000000001000000000000008004ff0400fffe001f00 will trigger the problematic code path :)

If anyone wants to test this PR vs master:

$ git checkout master
$ CC=clang CXX=clang++ ./configure --with-sanitizers=address,undefined
$ make
$ src/bitcoind &
$ src/bitcoin-cli decoderawtransaction ff0000000001000000000000008004ff0400fffe001f00
core_write.cpp:21:29: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount' (aka 'long'); cast to an unsigned type to negate this value to itself
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior core_write.cpp:21:29 in
error: couldn't parse reply from server

The malformed JSON response can be found here.

practicalswift · 2021-01-15T13:13:14Z

This PR has three stale ACKs (@laanwj, @MarcoFalke, @luke-jr) and I believe all feedback has been addressed.

Anything left to do here? :)

Would be nice to have this addressed to be able to fuzz direct and indirect callers of FormatMoney and ValueFromAmount with UBSan enabled without hitting these invalid integer negations over and over from different call sites :)

practicalswift · 2021-02-28T17:58:33Z

This PR has three stale ACKs and I believe all feedback has been addressed.

Ready for merge after re-review?

…(const CAmount& n) well-defined also when n is std::numeric_limits<CAmount>::min()

…omAmount(const CAmount& n) well-defined also when n is std::numeric_limits<CAmount>::min()

laanwj · 2021-03-03T18:03:57Z

re-ACK 1f05dbd

…ney and… … ValueFromAmount

DrahtBot added the Utils/log/libs label Nov 16, 2020

maflcko added the Refactoring label Nov 17, 2020

practicalswift mentioned this pull request Nov 19, 2020

Avoid signed integer overflow and invalid integer negation when loading malformed mempool.dat files #20383

Closed

laanwj reviewed Nov 20, 2020

View reviewed changes

src/core_write.cpp Outdated Show resolved Hide resolved

maflcko reviewed Nov 20, 2020

View reviewed changes

src/core_write.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the avoid-invalid-integer-negation-in-FormatMoney branch from 61e6c03 to abe78e8 Compare November 20, 2020 21:31

practicalswift force-pushed the avoid-invalid-integer-negation-in-FormatMoney branch 2 times, most recently from f710c50 to 681e92c Compare November 20, 2020 23:05

DrahtBot mentioned this pull request Nov 21, 2020

rpc: calculate fees in getblock using BlockUndo data #18772

Merged

laanwj reviewed Nov 23, 2020

View reviewed changes

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

practicalswift force-pushed the avoid-invalid-integer-negation-in-FormatMoney branch from 681e92c to 8d9979e Compare November 23, 2020 10:48

luke-jr approved these changes Nov 24, 2020

View reviewed changes

maflcko approved these changes Nov 30, 2020

View reviewed changes

src/core_write.cpp Outdated Show resolved Hide resolved

src/core_write.cpp Outdated Show resolved Hide resolved

src/test/rpc_tests.cpp Outdated Show resolved Hide resolved

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

jonatack reviewed Nov 30, 2020

View reviewed changes

src/core_write.cpp Outdated Show resolved Hide resolved

luke-jr reviewed Nov 30, 2020

View reviewed changes

src/core_write.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the avoid-invalid-integer-negation-in-FormatMoney branch from 8d9979e to 6043ae1 Compare November 30, 2020 16:04

jonatack reviewed Nov 30, 2020

View reviewed changes

src/core_io.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Dec 1, 2020

wallet: introduce setfeerate (an improved settxfee, in sat/vB) #20391

Closed

practicalswift mentioned this pull request Dec 1, 2020

refactor: replace manual Satoshis-to-BTC conversions with FormatMoney() #20537

Closed

DrahtBot added the Needs rebase label Dec 24, 2020

practicalswift force-pushed the avoid-invalid-integer-negation-in-FormatMoney branch 2 times, most recently from 2662109 to bd8926d Compare December 27, 2020 19:48

DrahtBot removed the Needs rebase label Dec 27, 2020

DrahtBot mentioned this pull request Dec 29, 2020

rpc: deprecate addresses and reqSigs from rpc outputs #20286

Merged

practicalswift added 2 commits March 2, 2021 16:05

util: Avoid invalid integer negation in FormatMoney: make FormatMoney…

7cc75c9

…(const CAmount& n) well-defined also when n is std::numeric_limits<CAmount>::min()

util: Avoid invalid integer negation in ValueFromAmount: make ValueFr…

1f05dbd

…omAmount(const CAmount& n) well-defined also when n is std::numeric_limits<CAmount>::min()

practicalswift force-pushed the avoid-invalid-integer-negation-in-FormatMoney branch from bd8926d to 1f05dbd Compare March 2, 2021 16:07

laanwj merged commit 47b99ab into bitcoin:master Mar 3, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 3, 2021

Merge bitcoin#20406: util: Avoid invalid integer negation in FormatMo…

d25758e

…ney and… … ValueFromAmount

practicalswift deleted the avoid-invalid-integer-negation-in-FormatMoney branch April 10, 2021 19:44

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

practicalswift commented Nov 16, 2020

Uh oh!

laanwj commented Nov 19, 2020

Uh oh!

practicalswift commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

RandyMcMillan commented Nov 20, 2020

n % COIN

Uh oh!

Uh oh!

practicalswift commented Nov 20, 2020

Uh oh!

RandyMcMillan commented Nov 20, 2020

Uh oh!

DrahtBot commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

laanwj commented Nov 23, 2020

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Nov 25, 2020

Uh oh!

luke-jr commented Nov 25, 2020

Uh oh!

practicalswift commented Nov 26, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jan 15, 2021

Uh oh!

practicalswift commented Feb 28, 2021

Uh oh!

laanwj commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

practicalswift commented Nov 19, 2020 •

edited

Loading

DrahtBot commented Nov 21, 2020 •

edited

Loading

practicalswift commented Nov 30, 2020 •

edited

Loading

practicalswift commented Dec 2, 2020 •

edited

Loading

laanwj commented Mar 3, 2021 •

edited

Loading