Mark `CheckTxInputs` `[[nodiscard]]`. Avoid UUM in fuzzing harness `coins_view`. #22065

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:nodiscard-CheckTxInputs changing 2 files +4 −3

practicalswift commented at 9:18 PM on May 25, 2021: contributor

Mark CheckTxInputs [[nodiscard]] (out-param txfee only set if call is successful).

Avoid use of uninitialised memory (UUM) in fuzzing harness coins_view.
Mark `CheckTxInputs` `[[nodiscard]]` (out-param `txfee` only set if call is successful). Avoid UUM in fuzzing harness `coins_view`. 37371268d1
DrahtBot added the label Tests on May 25, 2021
MarcoFalke commented at 5:52 AM on May 26, 2021: member
Quite fascinating that this was introduced when the fuzz test was initially written exactly one year ago on the day in commit https://github.com/bitcoin/bitcoin/commit/f9b22e3bdb54acb2f830b3ebbad47ff17dfb5781#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R245

OSS-Fuzz report: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34611

Apparently uninitialized reads are hard to reproduce and track down. See also:
- https://github.com/google/oss-fuzz/issues/5836
- https://github.com/bitcoin/bitcoin/issues/22064
MarcoFalke commented at 5:55 AM on May 26, 2021: member

review ACK 37371268d14ed6d5739af5b65d8bdb38b0e8dda2

in src/consensus/tx_verify.h:27 in 37371268d1

  23 | @@ -24,7 +24,7 @@ namespace Consensus {
  24 |   * @param[out] txfee Set to the transaction fee if successful.
  25 |   * Preconditions: tx.IsCoinBase() is false.
  26 |   */
  27 | -bool CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee);
  28 | +[[nodiscard]] bool CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee);

MarcoFalke commented at 5:56 AM on May 26, 2021:

might be nice to change this to a struct similar to struct MempoolAcceptResult, so that std::optional can be used

practicalswift commented at 6:02 AM on May 26, 2021:

Agreed! That would make a nice follow-up PR. Keeping this PR as-is to limit scope.

ryanofsky commented at 2:51 PM on May 26, 2021: member

What's UUM?
practicalswift commented at 3:14 PM on May 26, 2021: contributor

@ryanofsky

Use of uninitialised memory (UUM) :)

The abbreviation is sometimes used in the academic literature (see the MemorySanitizer paper as an example), but after Googling it I now understand it is not used outside of academia AFAICT. Sorry about the unusual abbreviation: I've now updated the OP to make it more clear :)
MarcoFalke commented at 6:52 AM on June 3, 2021: member

Also fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34843
MarcoFalke merged this on Jun 3, 2021
MarcoFalke closed this on Jun 3, 2021
sidhujag referenced this in commit 1afcbbde11 on Jun 3, 2021
gwillen referenced this in commit 08bd31102a on Jun 1, 2022
DrahtBot locked this on Aug 16, 2022

Contributors

Labels