util: Avoid invalid integer negation in FormatMoney and ValueFromAmount #20406

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:avoid-invalid-integer-negation-in-FormatMoney changing 8 files +45 −29
  1. practicalswift commented at 8:38 pm on November 16, 2020: contributor

    Avoid invalid integer negation in FormatMoney and ValueFromAmount.

    Fixes #20402.

    Before this patch:

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

    After this patch:

    0$ CC=clang CXX=clang++ ./configure --with-sanitizers=undefined
1$ make -C src/ test/test_bitcoin
2$ src/test/test_bitcoin -t rpc_tests/rpc_format_monetary_values -t util_tests/util_FormatMoney
  2. DrahtBot added the label Utils/log/libs on Nov 16, 2020
  3. MarcoFalke added the label Refactoring on Nov 17, 2020
  4. laanwj commented at 3:28 pm on November 19, 2020: member
    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.
  5. practicalswift commented at 4:05 pm on November 19, 2020: contributor

    @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 (review) :)

  6. in src/core_write.cpp:22 in 61e6c03370 outdated 
    21-    int64_t n_abs = (sign ? -amount : amount);
22-    int64_t quotient = n_abs / COIN;
23-    int64_t remainder = n_abs % COIN;
24+    int64_t quotient = amount / COIN;
25+    if (quotient < 0) {
26+        quotient = -quotient;
    laanwj commented at 5:42 am on November 20, 2020:

    It kind of feels weird to me how quotient and remainder are treated as positive/negative separately here. It’s only ever possible for either both of them to be positive (defining positive as >=0), or both of them to be negative, right? Depending on amount’s sign.

    (also maybe we need a compile time assertion that COIN is >1 or we’re back in the same place :laughing: )

    practicalswift commented at 9:36 pm on November 20, 2020:
    Good points. Feedback addressed!
  7. RandyMcMillan commented at 6:45 am on November 20, 2020: contributor

    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

    0 n %  COIN -->   1  
1 n % -COIN -->   1  
2-n %  COIN -->  -1  
3-n % -COIN -->  -1
  8. in src/core_write.cpp:29 in 61e6c03370 outdated 
    27+    }
28+    int64_t remainder = amount % COIN;
29+    if (remainder < 0) {
30+        remainder = -remainder;
31+    }
32     return UniValue(UniValue::VNUM,
    MarcoFalke commented at 10:47 am on November 20, 2020:

    Couldn’t this just call FormatMoney?

    0    return UniValue(UniValue::VNUM, FormatMoney(amount));
    practicalswift commented at 9:39 pm on November 20, 2020:
    Good point. The only difference between them is that FormatMoney chops off some trailing zeroes ("0.01" instead of "0.01000000") to make the output more human-friendly. That should be OK also for JSON consumption. Now calling FormatMoney.
    practicalswift commented at 10:55 pm on November 20, 2020:
    Oh, turns out we have quite a few assumptions about the exact number of trailing zeroes in the tests that read test/util/data/*.json. Reverting to the previous version!
    laanwj commented at 9:33 am on November 23, 2020:
    Yes, many clients assume the number of zeros is equal to the subdivision. This is why we don’t use the human-readable function for the JSON formatting.
  9. practicalswift force-pushed on Nov 20, 2020
  10. practicalswift commented at 10:00 pm on November 20, 2020: contributor
    @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 :)
  11. RandyMcMillan commented at 10:12 pm on November 20, 2020: contributor
    Thanks - I was just thinking thru different scenarios for testing. Negative moduli aren’t something that comes up very often. :)
  12. practicalswift force-pushed on Nov 20, 2020
  13. practicalswift force-pushed on Nov 20, 2020
  14. DrahtBot commented at 4:29 am on November 21, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #20391 (wallet: introduce setfeerate (an improved settxfee, in sat/vB) by jonatack)
    • #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.

  15. in src/util/moneystr.cpp:16 in 681e92ccef outdated 
    12@@ -13,9 +13,13 @@ std::string FormatMoney(const CAmount& n)
13 {
14     // Note: not using straight sprintf here because we do NOT want
15     // localized number formatting.
16-    int64_t n_abs = (n > 0 ? n : -n);
17-    int64_t quotient = n_abs/COIN;
18-    int64_t remainder = n_abs%COIN;
19+    static_assert(COIN > 0);
    laanwj commented at 9:34 am on November 23, 2020:
    Shouldn’t this be > 1 not > 0? If COIN=1, the same undefined behavior is back.
    practicalswift commented at 10:48 am on November 23, 2020:
    Now asserting COIN > 1 :)
  16. practicalswift force-pushed on Nov 23, 2020
  17. laanwj commented at 3:12 pm on November 23, 2020: member
    Thanks. Code review ACK 8d9979e04afdb081d923f79b581586330968ff56
  18. luke-jr approved
  19. luke-jr commented at 11:34 pm on November 24, 2020: member
    utACK, but… is there a reason we don’t just leave the sign in quotient?
  20. practicalswift commented at 8:44 am on November 25, 2020: contributor
    @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.
  21. luke-jr commented at 3:28 pm on November 25, 2020: member
    Right now, we’re checking amount < 0 explicitly and prefixing -. If we simply don’t negate quotient, strprintf will do the - for us…
  22. practicalswift commented at 9:34 am on November 26, 2020: contributor
    @luke-jr I’ll let others chime in, and I’ll happily adjust to the consensus opinion regarding the suggested extra refactoring :)
  23. in src/core_write.cpp:22 in 8d9979e04a outdated 
    20 {
21-    bool sign = amount < 0;
22-    int64_t n_abs = (sign ? -amount : amount);
23-    int64_t quotient = n_abs / COIN;
24-    int64_t remainder = n_abs % COIN;
25+    static_assert(COIN > 1);
    MarcoFalke commented at 2:59 pm on November 30, 2020:
    Seems redundant. I can’t imagine why anyone would want to set COIN to 0 or negative. Literally every test will fail if it was changed anyay.
    practicalswift commented at 3:10 pm on November 30, 2020:
    The compile-time assertion was suggested by @laanwj in #20406 (review). As noted in #20406 (comment) I don’t have any strong opinion. I’ll let others chime in, but leaving as-is for now :)
  24. in src/core_write.cpp:23 in 8d9979e04a outdated 
    21-    bool sign = amount < 0;
22-    int64_t n_abs = (sign ? -amount : amount);
23-    int64_t quotient = n_abs / COIN;
24-    int64_t remainder = n_abs % COIN;
25+    static_assert(COIN > 1);
26+    int64_t quotient = amount / COIN;
    MarcoFalke commented at 3:00 pm on November 30, 2020:

    style-nit: Could avoid the if below

    0    int64_t quotient{std::abs(amount / COIN)};
  25. in src/test/rpc_tests.cpp:186 in 8d9979e04a outdated 
    181+    BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::max() - 3).write(), "92233720368.54775804");   //  9223372036854775804
182+    // ...
183+    BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::min() + 3).write(), "-92233720368.54775805");  // -9223372036854775805
184+    BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::min() + 2).write(), "-92233720368.54775806");  // -9223372036854775806
185+    BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::min() + 1).write(), "-92233720368.54775807");  // -9223372036854775807
186+    BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::min()).write(), "-92233720368.54775808");      // -9223372036854775808
    MarcoFalke commented at 3:01 pm on November 30, 2020: 
    0    BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::min()).write(), "-92233720368.54775808");

    what is the point of the comment? I think the code is self-explanatory.

    practicalswift commented at 3:12 pm on November 30, 2020:
    I see your point, but leaving as-is in order to not invalidate ACK:s. Will fix if I touch the code for other reasons :)
    practicalswift commented at 4:04 pm on November 30, 2020:
    Now fixed :)
  26. in src/util/moneystr.cpp:18 in 8d9979e04a outdated 
    12@@ -13,9 +13,13 @@ std::string FormatMoney(const CAmount& n)
13 {
14     // Note: not using straight sprintf here because we do NOT want
15     // localized number formatting.
16-    int64_t n_abs = (n > 0 ? n : -n);
17-    int64_t quotient = n_abs/COIN;
18-    int64_t remainder = n_abs%COIN;
19+    static_assert(COIN > 1);
20+    int64_t quotient = n / COIN;
21+    int64_t remainder = n % COIN;
    MarcoFalke commented at 3:01 pm on November 30, 2020:

    same here

    0    int64_t remainder{std::abs(n % COIN)};
  27. MarcoFalke approved
  28. MarcoFalke commented at 3:01 pm on November 30, 2020: member
    review ack
  29. in src/core_write.cpp:18 in 8d9979e04a outdated 
    11@@ -12,17 +12,20 @@
12 #include <serialize.h>
13 #include <streams.h>
14 #include <univalue.h>
15-#include <util/system.h>
16 #include <util/strencodings.h>
17+#include <util/system.h>
18 
19 UniValue ValueFromAmount(const CAmount& amount)
    jonatack commented at 3:24 pm on November 30, 2020:

    CAmount is a cheaply-copied type; while refactoring ValueFromAmount could pass by value; same for FormatMoney

    0UniValue ValueFromAmount(CAmount amount)
    practicalswift commented at 4:04 pm on November 30, 2020:
    Feedback addressed :)
  30. in src/core_write.cpp:31 in 8d9979e04a outdated 
    30+        remainder = -remainder;
31+    }
32     return UniValue(UniValue::VNUM,
33-            strprintf("%s%d.%08d", sign ? "-" : "", quotient, remainder));
34+            strprintf("%s%d.%08d", amount < 0 ? "-" : "", quotient, remainder));
35 }
    luke-jr commented at 3:24 pm on November 30, 2020: 
    0UniValue ValueFromAmount(const CAmount& amount)
1{
2    int64_t quotient = amount / COIN;
3    int64_t remainder = std::abs(amount % COIN);
4    static_assert(COIN == 100000000);  // required for format-string below
5    return UniValue(UniValue::VNUM, strprintf("%d.%08d", quotient, remainder));
6}
    MarcoFalke commented at 3:29 pm on November 30, 2020:
    That doesn’t work because the sign is lost whenever quotient is 0 (there is no negative zero)
    luke-jr commented at 3:49 pm on November 30, 2020:
    Aww, it was so close to being a simple/elegant solution, but you’re right…
  31. practicalswift force-pushed on Nov 30, 2020
  32. practicalswift commented at 4:05 pm on November 30, 2020: contributor
    Thanks for reviewing! Nits addressed (+ rebase: sorry!). Please re-review :)
  33. in src/core_io.h:43 in 6043ae14ee outdated 
    38@@ -39,7 +39,7 @@ std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strN
39 int ParseSighashString(const UniValue& sighash);
40 
41 // core_write.cpp
42-UniValue ValueFromAmount(const CAmount& amount);
43+UniValue ValueFromAmount(const CAmount amount);
    jonatack commented at 4:18 pm on November 30, 2020:
    Thanks! Can you please drop the const here and in FormatMoney (discussion why: #19845 (review))
    practicalswift commented at 4:46 pm on November 30, 2020:
    void f(const int i) vs void f(int i) is a matter of taste :) Leaving as-is for now. As always with taste issues: if someone feels strongly about such a choice the right forum is probably a PR updating the developer notes :)
    luke-jr commented at 5:28 pm on November 30, 2020:
    IMO const should be used for the definition, but not declaration, when appropriate. In the definition, it instructs the compiler to error if you attempt to change it (eg, a bug). In the declaration, it has no effect at all.
    laanwj commented at 6:40 pm on December 17, 2020:

    FWIW the original idea with passing a reference here is that if CAmount is a more complex object, a copy is not needed. At the time, this helped e.g. sidechains and altcoins with different concept of amount (say, a variable length integer) easier use the code. I don’t care much about this, but just thought I’d mention.

    I agree with @luke-jr that const in by-value definitions doesn’t do anything and so is just clutter.

    MarcoFalke commented at 6:50 pm on December 17, 2020:
    Seems unrelated to a bugfix in any case
  34. practicalswift commented at 2:57 pm on December 2, 2020: contributor

    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:

    0$ git checkout master
1$ CC=clang CXX=clang++ ./configure --with-sanitizers=address,undefined
2$ make
3$ src/bitcoind &
4$ src/bitcoin-cli decoderawtransaction ff0000000001000000000000008004ff0400fffe001f00
5core_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
6SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior core_write.cpp:21:29 in
7error: couldn't parse reply from server

    The malformed JSON response can be found here.

  35. DrahtBot added the label Needs rebase on Dec 24, 2020
  36. practicalswift force-pushed on Dec 27, 2020
  37. practicalswift force-pushed on Dec 27, 2020
  38. DrahtBot removed the label Needs rebase on Dec 27, 2020
  39. practicalswift commented at 1:13 pm on January 15, 2021: contributor

    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 :)

  40. practicalswift commented at 5:58 pm on February 28, 2021: contributor

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

    Ready for merge after re-review?

  41. util: Avoid invalid integer negation in FormatMoney: make FormatMoney(const CAmount& n) well-defined also when n is std::numeric_limits<CAmount>::min() 7cc75c9ba3
  42. util: Avoid invalid integer negation in ValueFromAmount: make ValueFromAmount(const CAmount& n) well-defined also when n is std::numeric_limits<CAmount>::min() 1f05dbd06d
  43. practicalswift force-pushed on Mar 2, 2021
  44. laanwj commented at 6:03 pm on March 3, 2021: member
    re-ACK 1f05dbd06d896849d16b026bfc3315ee8b73a89f
  45. laanwj merged this on Mar 3, 2021
  46. laanwj closed this on Mar 3, 2021
  47. sidhujag referenced this in commit d25758e560 on Mar 3, 2021
  48. practicalswift deleted the branch on Apr 10, 2021
  49. DrahtBot locked this on Aug 16, 2022


practicalswift laanwj RandyMcMillan MarcoFalke DrahtBot luke-jr jonatack

Labels
Refactoring Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me