[Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue #8164
Conversation
af94dca to
86efa30
Compare
|
ACK 86efa30 |
|
Hmm, doesn't this call for generalization? Ie. to not need to name the files in this directory? |
| @@ -206,7 +206,7 @@ static void MutateTxAddInput(CMutableTransaction& tx, const string& strInput) | |||
| // extract the optional sequence number | |||
| uint32_t nSequenceIn=std::numeric_limits<unsigned int>::max(); | |||
| if (vStrInputParts.size() > 2) | |||
| nSequenceIn = atoi(vStrInputParts[2]); | |||
| nSequenceIn = std::stoul(vStrInputParts[2]); | |||
There was a problem hiding this comment.
This should be stored to an unsigned long and checked against std::numeric_limits<uint32_t>::max() just to be safe, otherwise invalid values will wrap and be quietly accepted. To be doubly safe, it'd need to use strtoll and check errno and negatives :.
I'm unsure what amount of paranoia is called for here.
There was a problem hiding this comment.
IMO we only support platforms/archs where int is always 32bit and I think std::numeric_limits<uint32_t>::max() won't change much.
There was a problem hiding this comment.
@jonasschnelli These are the cases I'm worried about (running on current master):
./bitcoin-tx 01000000000000000000 in=838af7eea675044a35a646f937ba26c412e4db51dc2b155c45f5c2675acd0cae:1:-1
0100000001ae0ccd5a67c2f5455c152bdc51dbe412c426ba37f946a6354a0475a6eef78a830100000000ffffffff0000000000
./bitcoin-tx 01000000000000000000 in=838af7eea675044a35a646f937ba26c412e4db51dc2b155c45f5c2675acd0cae:1:4294967296
0100000001ae0ccd5a67c2f5455c152bdc51dbe412c426ba37f946a6354a0475a6eef78a830100000000000000000000000000
In order to be able to test for those cases on the worst-case platform (win64), you need to store the input as a signed long long, since a signed long isn't big enough there.
There was a problem hiding this comment.
why not use ParseInt32, a function I especially wrote for this purpose and does the necessary checking?
Edit: as discused on IRC we really need a ParseUInt32, going to write one
There was a problem hiding this comment.
Right. ParseInt32 would be better. I guess it would require a ParseIntU32. This can be done in a later PR and also changes the rest of bitcoin-tx (there are servals atoi() and atoi64).
|
For the sake of unborking master, ACK 86efa30. I'll address my complaints in a follow-up. |
…ssue 86efa30 [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue (Jonas Schnelli)
Add error and range-checking parsers for unsigned 32 and 64 bit numbers. The 32-bit variant is required for parsing sequence numbers from the command line in `bitcoin-tx` (see bitcoin#8164 for discussion). I've thrown in the 64-bit variant as a bonus, as I'm sure it will be needed at some point. Also adds tests, and updates `developer-notes.md`.
… atoi issue 86efa30 [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue (Jonas Schnelli)
…quence number bitcoin/bitcoin#7957 - [RPC][Bitcoin-TX] Add support for sequence number bitcoin/bitcoin#8164 - [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue bitcoin/bitcoin#8171 - [RPC] Fix createrawtx sequence number unsigned int parsing [RPC][Bitcoin-TX] Add support for sequence number
…quence number bitcoin/bitcoin#7957 - [RPC][Bitcoin-TX] Add support for sequence number bitcoin/bitcoin#8164 - [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue bitcoin/bitcoin#8171 - [RPC] Fix createrawtx sequence number unsigned int parsing [RPC][Bitcoin-TX] Add support for sequence number
Add error and range-checking parsers for unsigned 32 and 64 bit numbers. The 32-bit variant is required for parsing sequence numbers from the command line in `bitcoin-tx` (see bitcoin#8164 for discussion). I've thrown in the 64-bit variant as a bonus, as I'm sure it will be needed at some point. Also adds tests, and updates `developer-notes.md`.
… atoi issue 86efa30 [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue (Jonas Schnelli)
Travis is currently failing because of missing test fixtures from #7957.
This PR adds the two missing fixture files to the
Makefile.test.include.