hash: Use size_t instead of int for RFC6979 outlen copy #1729

pull john-moffett wants to merge 1 commits into bitcoin-core:master from john-moffett:fix-rfc6979-size_t changing 1 files +1 −1

john-moffett commented at 2:40 PM on September 1, 2025: contributor

If outlen > INT_MAX it results in segfault or hang (when outlen is a multiple of 2^32) on most implementations due to conversion in: int now = outlen producing negative values or zero. Unreachable in current code and highly improbable in future practice, but fits contract better and fixes a couple of compiler warnings.

Use size_t instead of int for RFC6979 outlen copy

If outlen is > INT_MAX, could trigger segfault or hang after copy
int now = outlen.

960ba5f9c6

fanquake commented at 2:42 PM on September 1, 2025: member

fixes a couple of compiler warnings

Which compiler / warnings?
john-moffett commented at 2:53 PM on September 1, 2025: contributor

Not with default flags. I used clang -Wimplicit-int-conversion -Wshorten-64-to-32. It'd probably show up with gcc -Wconversion. There are a lot of benign warnings, but this one stood out to me.
in src/hash_impl.h:1 in 960ba5f9c6
real-or-random commented at 3:28 PM on September 1, 2025:

If you want to improve this further, I'd suggest

renaming now to chunk_len (consistency with the SHA256 implementation above)

moving the entire if block just below the initialization of now (or chunk_len). (The goal is to compute max(outlen, 32). Spreading this over two blocks is a bit silly. It's probably a result of the C89 rule that declarations need to be at the beginning of a block. edit: Indeed, see https://github.com/bitcoin-core/secp256k1/commit/792bcdb015b0272928d4dd602557faa685dcd5a1#diff-e91b67aa33d606267c28ce5da48a89cc789f4081c176eb581a9bff780afd1d6cR260-L262 )
john-moffett commented at 1:53 PM on September 2, 2025:

Thanks! I'll hold off for the moment. Hopefully it'll inspire someone to replace this nonce generation process with something more lightweight and straightforward like BIP340's approach. :)
real-or-random approved
real-or-random commented at 3:30 PM on September 1, 2025: contributor

utACK 960ba5f9c60c3fd454ad4160ce9131eb77d61d7e

Thanks a lot! Consistency of integer types is indeed a bit of a weak spot, at least in the old parts of the library code.
real-or-random added the label assurance on Sep 1, 2025
real-or-random added the label tweak/refactor on Sep 1, 2025
theStack approved
theStack commented at 6:05 PM on September 2, 2025: contributor

Code-review ACK 960ba5f9c60c3fd454ad4160ce9131eb77d61d7e
real-or-random merged this on Sep 2, 2025
real-or-random closed this on Sep 2, 2025

Contributors

Labels

assurance tweak/refactor