bitcoin-tx: Reject non-integral and out of range int strings #23253

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:2110-utilTxSeqId changing 3 files +61 −7

MarcoFalke commented at 12:03 PM on October 11, 2021: member

Seems odd to silently accept arbitrary strings that don't even represent integral values.

Fix that.
MarcoFalke renamed this:
~~bitcoin-tx: Reject non-integral and out of range sequence ids~~
bitcoin-tx: Reject non-integral and out of range int strings
on Oct 11, 2021
MarcoFalke force-pushed on Oct 11, 2021
DrahtBot added the label Utils/log/libs on Oct 11, 2021
practicalswift commented at 1:33 PM on October 11, 2021: contributor

Concept ACK

promag commented at 8:32 AM on October 12, 2021: member

Code review ACK fa334a8765a88f18e7a63d908b672b27343d8219.

My only suggestion is to create a helper to avoid duplicate code, like:

template <typename T>
T ParseIntegral(const std::string& str, const std::string& error_msg)
{
    const auto parsed{ToIntegral<T>(TrimString(str))};
    if (!parsed.has_value()) {
        throw std::runtime_error(error_msg + " '" + str + "'");
    }
    return parsed.value();
}

test: Check that bitcoin-tx accepts whitespace around sequence id and multisig numbers fa53d3d826
bitcoin-tx: Reject non-integral and out of range sequence ids fafab8ea5e
bitcoin-tx: Reject non-integral and out of range multisig numbers fa6f29de51
MarcoFalke force-pushed on Oct 12, 2021
MarcoFalke commented at 10:47 AM on October 12, 2021: member

Thanks, done
promag commented at 11:16 AM on October 12, 2021: member

Code review ACK fa6f29de516c7af5206b91b59ada466032329250.
practicalswift commented at 8:14 AM on October 13, 2021: contributor

cr ACK fa6f29de516c7af5206b91b59ada466032329250

in src/bitcoin-tx.cpp:243 in fa6f29de51

 234 | @@ -235,6 +235,16 @@ static void MutateTxRBFOptIn(CMutableTransaction& tx, const std::string& strInId
 235 |      }
 236 |  }
 237 |  
 238 | +template <typename T>
 239 | +static T TrimAndParse(const std::string& int_str, const std::string& err)
 240 | +{
 241 | +    const auto parsed{ToIntegral<T>(TrimString(int_str))};
 242 | +    if (!parsed.has_value()) {
 243 | +        throw std::runtime_error(err + " '" + int_str + "'");

laanwj commented at 11:36 AM on October 13, 2021:

It is very un-us to throw an exception for a parse error here. But as this is local to bitcoin-tx where this is explicitly caught I think it's acceptable.

laanwj commented at 11:37 AM on October 13, 2021: member

Code review ACK fa6f29de516c7af5206b91b59ada466032329250
Empact commented at 11:39 AM on October 13, 2021: member

Code review ACK https://github.com/bitcoin/bitcoin/pull/23253/commits/fa6f29de516c7af5206b91b59ada466032329250
laanwj merged this on Oct 13, 2021
laanwj closed this on Oct 13, 2021
sidhujag referenced this in commit e72db9bd46 on Oct 13, 2021
MarcoFalke deleted the branch on Oct 22, 2021
DrahtBot locked this on Oct 30, 2022

Contributors

Labels