net: Fix an uninitialized read in ProcessMessage(…, "tx", …) when receiving a transaction we already have #17624

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:uninitialized-read-in-ProcessMessage changing 1 files +2 −2
  1. practicalswift commented at 10:14 PM on November 27, 2019: contributor

    Fix an uninitialized read in ProcessMessage(…, "tx", …) when receiving a transaction we already have.

    The uninitialized value is read and used on L2526 in the case of AlreadyHave(inv) == true.

    Proof of concept being run against a bitcoind built with MemorySanitizer (-fsanitize=memory):

    $ ./p2p-uninit-read-in-conditional-poc.py
    Usage: ./p2p-uninit-read-in-conditional-poc.py <dstaddr> <dstport> <net>
    $ bitcoind -regtest &
    $ ./p2p-uninit-read-in-conditional-poc.py 127.0.0.1 18444 regtest
    SUMMARY: MemorySanitizer: use-of-uninitialized-value
    [1]+  Exit 77                 bitcoind -regtest
    $
    

    Proof of concept being run against a bitcoind running under Valgrind (valgrind --exit-on-first-error):

    $ valgrind -q --exit-on-first-error=yes --error-exitcode=1 bitcoind -regtest &
    $ ./p2p-uninit-read-in-conditional-poc.py 127.0.0.1 18444 regtest
    ==27351== Conditional jump or move depends on uninitialised value(s)
    [1]+  Exit 1                  valgrind -q --exit-on-first-error=yes --error-exitcode=1 bitcoind -regtest
    $ 
    

    Proof of concept script:

    #!/usr/bin/env python3
    
    import sys
    
    from test_framework.mininode import NetworkThread
    from test_framework.mininode import P2PDataStore
    from test_framework.messages import CTransaction, CTxIn, CTxOut, msg_tx
    
    
    def send_duplicate_tx(dstaddr="127.0.0.1", dstport=18444, net="regtest"):
        network_thread = NetworkThread()
        network_thread.start()
    
        node = P2PDataStore()
        node.peer_connect(dstaddr=dstaddr, dstport=dstport, net=net)()
        node.wait_for_verack()
    
        tx = CTransaction()
        tx.vin.append(CTxIn())
        tx.vout.append(CTxOut())
        node.send_message(msg_tx(tx))
        node.send_message(msg_tx(tx))
        node.peer_disconnect()
        network_thread.close()
    
    
    if __name__ == "__main__":
        if len(sys.argv) != 4:
            print("Usage: {} <dstaddr> <dstport> <net>".format(sys.argv[0]))
            sys.exit(0)
        send_duplicate_tx(sys.argv[1], int(sys.argv[2]), sys.argv[3])
    

    Note that the transaction in the proof of concept is the simplest possible, but really any transaction can be used. It does not have to be a valid transaction.

    This bug was introduced in #15921 ("validation: Tidy up ValidationState interface") which was merged in to master 28 days ago.

    Luckily this bug was caught before being part of any Bitcoin Core release :)

  2. net: Fix uninitialized read in ProcessMessage(...) 73b96c94cb
  3. DrahtBot added the label P2P on Nov 27, 2019
  4. DrahtBot commented at 10:33 PM on November 27, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #17557 (util: Refactor message hashing into a utility function by jkczyz)
    • #17399 (validation: Templatize ValidationState instead of subclassing by jkczyz)

    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.

  5. jnewbery commented at 3:40 AM on November 28, 2019: member

    utACK 73b96c94cb6c2afdee7f151768a96944ecaf9d9b

    Great catch @practicalswift . Are the else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) and else branches really not covered by any of our tests? If so, that seems like a big hole in our testing. We should add tests for duplicate TX messages received.

  6. laanwj commented at 8:56 AM on November 28, 2019: member

    ACK 73b96c94cb6c2afdee7f151768a96944ecaf9d9b, thanks for discovering and reporting this before it ended up in a release.

    (travis fail is unrelated, restarted …)

  7. practicalswift commented at 9:39 AM on November 28, 2019: contributor

    @jnewbery

    Both branches are covered by our functional tests (specifically test/functional/p2p_segwit.py), but unfortunately we're not running the functional tests under MemorySanitizer or Valgrind :(

    I found this issue after observing some weirdness on a mainnet when running a live node with MemorySanitizer instrumentation enabled. Receiving duplicate transactions on mainnet happens quite regularly. I then reproduced the issue on regtest.

    Since discovering the issue on mainnet I've "rediscovered" it 1.) using static analysis tooling (Coverity is one example), 2.) by running the ordinary functional tests under MemorySanitizer, and 3.) by running the ordinary functional tests under Valgrind.

    My personal view is that we really really underuse the excellent modern tooling that is typically used in security critical C++ projects to guard against introduction of bugs like this. I find that a bit surprising and I promise to do my best to help improve that situation going forward :)


    Rediscovery 1. Finding the issue using static analysis (Coverity in this example)

    ________________________________________________________________________________________________________
    *** CID 350378:  Uninitialized members  (UNINIT_CTOR)
    /src/consensus/validation.h: 117 in TxValidationState::TxValidationState()()
    111     };
    112     
    113     inline ValidationState::~ValidationState() {};
    114     
    115     class TxValidationState : public ValidationState {
    116     private:
    >>>     CID 350378:  Uninitialized members  (UNINIT_CTOR)
    >>>     The compiler-generated constructor for this class does not initialize "m_result".
    117         TxValidationResult m_result;
    118     public:
    119         bool Invalid(TxValidationResult result,
    120                      const std::string &reject_reason="",
    121                      const std::string &debug_message="")
    122         {
    ** CID 350377:  Uninitialized variables  (UNINIT)
    ________________________________________________________________________________________________________
    *** CID 350377:  Uninitialized variables  (UNINIT)
    /src/net_processing.cpp: 2526 in ProcessMessage(CNode *, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, CDataStream &, long, const CChainParams &, CConnman *, BanMan *, const std::atomic<bool> &)()
    2520                     tx.GetHash().ToString(),
    2521                     mempool.size(), mempool.DynamicMemoryUsage() / 1000);
    2522     
    2523                 // Recursively process any orphan transactions that depended on this one
    2524                 ProcessOrphanTx(connman, pfrom->orphan_work_set, lRemovedTxn);
    2525             }
    >>>     CID 350377:  Uninitialized variables  (UNINIT)
    >>>     Using uninitialized value "state.m_result" when calling "GetResult".
    2526             else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS)
    2527             {
    2528                 bool fRejectedParents = false; // It may be the case that the orphans parents have all been rejected
    2529                 for (const CTxIn& txin : tx.vin) {
    2530                     if (recentRejects->contains(txin.prevout.hash)) {
    2531                         fRejectedParents = true;
    

    Rediscovery 2. Finding the issue using dynamic analysis (MemorySanitizer)

    Running test/functional/test_runner.py with bitcoind compiled with MSAN instrumentation:

    $ test/functional/test_runner.py
    …
    2020-01-02T04:05:06.123456Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
    2020-01-02T04:05:06.123456Z TestFramework (ERROR): Assertion failed
    …
    SUMMARY: MemorySanitizer: use-of-uninitialized-value
    

    Rediscovery 3. Finding the issue using dynamic analysis (Valgrind)

    Running BITCOIND=bitcoind_valgrind test/functional/test_runner.py where bitcoind_valgrind is a shell script wrapper doing valgrind -q --exit-on-first-error=yes --error-exitcode=1 --gen-suppressions=all --leak-check=full --show-leak-kinds=all --suppressions=contrib/valgrind.supp src/bitcoind "$@":

    $ BITCOIND=bitcoind_valgrind test/functional/test_runner.py
    …
    2020-01-02T04:05:06.123456Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
    2020-01-02T04:05:06.123456Z TestFramework (ERROR): Assertion failed
    …
    ==47281== Conditional jump or move depends on uninitialised value(s)
    
  8. laanwj referenced this in commit 114e89e596 on Nov 28, 2019
  9. laanwj merged this on Nov 28, 2019
  10. laanwj closed this on Nov 28, 2019

  11. MarcoFalke referenced this in commit 3d6752779f on Dec 10, 2019
  12. sidhujag referenced this in commit 6e0bed9cbe on Dec 10, 2019
  13. jasonbcox referenced this in commit 0704a943d9 on Jul 14, 2020
  14. sidhujag referenced this in commit bcf410ff00 on Nov 10, 2020
  15. practicalswift deleted the branch on Apr 10, 2021
  16. PastaPastaPasta referenced this in commit c5ad6c618d on Jun 4, 2021
  17. vijaydasmp referenced this in commit 82e8a6b2fd on Apr 1, 2022
  18. vijaydasmp referenced this in commit 53fee2a01c on Apr 2, 2022
  19. vijaydasmp referenced this in commit 77f106ebf5 on Apr 3, 2022
  20. kittywhiskers referenced this in commit 6d1ac9b459 on Apr 5, 2022
  21. kittywhiskers referenced this in commit 09b0205903 on Apr 5, 2022
  22. DrahtBot locked this on Aug 16, 2022

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: 2026-05-02 18:14 UTC

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