Signed integer overflow in CCoinsViewCache::GetValueIn(…) #18858

issue practicalswift openend this issue on May 3, 2020
  1. practicalswift commented at 5:43 pm on May 3, 2020: contributor

    CCoinsViewCache::GetValueIn(…) performs money summation like this:

     0CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const
     1{
     2    if (tx.IsCoinBase())
     3        return 0;
     4
     5    CAmount nResult = 0;
     6    for (unsigned int i = 0; i < tx.vin.size(); i++)
     7        nResult += AccessCoin(tx.vin[i].prevout).out.nValue;
     8
     9    return nResult;
    10}
    

    Note that no check is done to make sure that the resulting nResult is such that it stays within the money bounds (MoneyRange(nResult)), or that the summation does not trigger a signed integer overflow.

    Proof of concept output:

    0coins.cpp:243:17: runtime error: signed integer overflow: 9223200000000000000 + 2100000000000000 cannot be represented in type 'long'
    1GetValueIn = -9221444073709551616
    

    Proof of concept code:

     0CMutableTransaction mutable_transaction;
     1mutable_transaction.vin.resize(4393);
     2
     3Coin coin;
     4coin.out.nValue = MAX_MONEY;
     5assert(MoneyRange(coin.out.nValue));
     6
     7CCoinsCacheEntry coins_cache_entry;
     8coins_cache_entry.coin = coin;
     9coins_cache_entry.flags = CCoinsCacheEntry::DIRTY;
    10
    11CCoinsView backend_coins_view;
    12CCoinsViewCache coins_view_cache{&backend_coins_view};
    13CCoinsMap coins_map;
    14coins_map.emplace(COutPoint{}, std::move(coins_cache_entry));
    15coins_view_cache.BatchWrite(coins_map, {});
    16
    17const CAmount total_value_in = coins_view_cache.GetValueIn(CTransaction{mutable_transaction});
    18std::cout << "GetValueIn = " << total_value_in << std::endl;
    

    As far as I can tell CCoinsViewCache::GetValueIn is unused outside of tests:

    0$ git grep GetValueIn ":(exclude)src/test/" ":(exclude)src/bench/"
    1src/coins.cpp:CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const
    2src/coins.h:    CAmount GetValueIn(const CTransaction& tx) const;
    3src/primitives/transaction.h:    // GetValueIn() is a method on CCoinsViewCache, because
    

    I suggest we drop the unused CCoinsViewCache::GetValueIn(…).

    Any objections? :)

  2. practicalswift commented at 5:56 pm on May 3, 2020: contributor
    Another unrelated oddity I noticed is that coins_view_cache.GetValueIn(tx) > 0 does not imply coins_view_cache.HaveInputs(tx) == true for all tx.
  3. sipa commented at 6:02 pm on May 3, 2020: member
    If a function is only used in tests it should be removed.
  4. MarcoFalke added the label Tests on May 3, 2020
  5. practicalswift commented at 6:15 pm on May 3, 2020: contributor

    @sipa

    I’ll remove.

    It seems like GetValueIn was added in #748 (“Pay-to-script-hash (OP_EVAL replacement)”, merged in 2012) and the last use in validation code was removed in #8498 (“Near-Bugfix: Optimization: Minimize the number of times it is checked that no money…”, merged in 2017).

  6. MarcoFalke closed this on May 4, 2020

  7. ComputerCraftr referenced this in commit 04dcc6ec3e on Jun 10, 2020
  8. ComputerCraftr referenced this in commit 514b330e83 on Jun 10, 2020
  9. DrahtBot locked this on Feb 15, 2022
  10. vijaydasmp referenced this in commit 443a5d0d5d on Jan 11, 2023
  11. vijaydasmp referenced this in commit 9df0f801fd on Jan 11, 2023
  12. vijaydasmp referenced this in commit a2d87de55d on Jan 19, 2023
  13. vijaydasmp referenced this in commit 9390cba303 on Jan 20, 2023
  14. vijaydasmp referenced this in commit 65da6ab198 on Jan 20, 2023
  15. vijaydasmp referenced this in commit ec5a06e5d1 on Feb 17, 2023
  16. vijaydasmp referenced this in commit eaf1b72d66 on Feb 18, 2023
  17. vijaydasmp referenced this in commit 00f177b53c on Feb 18, 2023
  18. vijaydasmp referenced this in commit 78a6e4fe33 on Feb 20, 2023
  19. vijaydasmp referenced this in commit f7708e87f5 on Feb 20, 2023
  20. vijaydasmp referenced this in commit 59f83c5590 on Feb 22, 2023
  21. vijaydasmp referenced this in commit 6d8ddd1ab9 on Feb 22, 2023
  22. vijaydasmp referenced this in commit 74240be1d1 on Feb 23, 2023
  23. vijaydasmp referenced this in commit 3d511b7fb5 on Feb 24, 2023
  24. vijaydasmp referenced this in commit 320678efb8 on Feb 28, 2023
  25. vijaydasmp referenced this in commit cb3be0fd3b on Mar 3, 2023
  26. PastaPastaPasta referenced this in commit f1c84cabf7 on Mar 27, 2023

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-10-05 01:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me