Skip to content

Conversation

john-moffett
Copy link
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.

If outlen is > INT_MAX, could trigger segfault or hang after copy
int now = outlen.
@fanquake
Copy link
Member

fanquake commented Sep 1, 2025

fixes a couple of compiler warnings

Which compiler / warnings?

@john-moffett
Copy link
Contributor Author

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.

Copy link
Contributor

@real-or-random real-or-random Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 792bcdb#diff-e91b67aa33d606267c28ce5da48a89cc789f4081c176eb581a9bff780afd1d6cR260-L262 )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 960ba5f

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.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-review ACK 960ba5f

@real-or-random real-or-random merged commit 8113671 into bitcoin-core:master Sep 2, 2025
116 checks passed
vmta added a commit to umkoin/umkoin that referenced this pull request Sep 21, 2025
36e76952c Merge bitcoin-core/secp256k1#1738: check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so)
4985ac0f8 Merge bitcoin-core/secp256k1#1737: doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static)
7ebaa134a check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so)
806de38bf doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static)
03fb60ad2 Merge bitcoin-core/secp256k1#1681: doc: Recommend clang-cl when building on Windows
d93380fb3 Merge bitcoin-core/secp256k1#1731: schnorrsig: Securely clear buf containing k or its negation
8113671f8 Merge bitcoin-core/secp256k1#1729: hash: Use size_t instead of int for RFC6979 outlen copy
325d65a8c Rename and clear var containing k or -k
960ba5f9c Use size_t instead of int for RFC6979 outlen copy
737912430 ci: Add more tests for clang-cl
7379a5bed doc: Recommend clang-cl when building on Windows
f36afb8b3 Merge bitcoin-core/secp256k1#1725: tests: refactor tagged hash verification
5153cf1c9 tests: refactor tagged hash tests
d2dcf5209 Merge bitcoin-core/secp256k1#1726: docs: fix broken link to Tromer's cache.pdf paper
489a43d1b docs: fix broken link to eprint cache.pdf paper
d59971414 Merge bitcoin-core/secp256k1#1722: docs: Exclude modules' `bench_impl.h` headers from coverage report
0458def51 doc: Add `--gcov-ignore-parse-errors=all` option to `gcovr` invocations
1aecce593 doc: Add `--merge-mode-functions=separate` option to `gcovr` invocations
106a7cbf4 doc: Exclude modules' `bench_impl.h` headers from coverage report
a9e955d3e autotools, docs: Adjust help string for `--enable-coverage` option
e523e4f90 Merge bitcoin-core/secp256k1#1720: chore(ci): Fix typo in Dockerfile comment
24ba8ff16 chore(ci): Fix typo in Dockerfile comment
74b8068c5 Merge bitcoin-core/secp256k1#1717: test: update wycheproof test vectors
c25c3c8a8 test: update wycheproof test vectors
20e3b4474 Merge bitcoin-core/secp256k1#1688: cmake: Avoid contaminating parent project's cache with `BUILD_SHARED_LIBS`
2c076d907 Merge bitcoin-core/secp256k1#1711: tests: update Wycheproof
7b07b2295 cmake: Avoid contaminating parent project's cache with BUILD_SHARED_LIBS
5433648ca Fix typos and spellings
9ea54c69b tests: update Wycheproof files
b9313c6e1 Merge bitcoin-core/secp256k1#1708: release cleanup: bump version after 0.7.0
a660a4976 Merge bitcoin-core/secp256k1#1707: release: Prepare for 0.7.0
7ab8b0cc0 release cleanup: bump version after 0.7.0
a3e742d94 release: Prepare for 0.7.0
f67b0ac1a ci: Don't hardcode ABI version
020ee6049 Merge bitcoin-core/secp256k1#1706: musig/tests: initialize keypair
cde413089 musig/tests: initialize keypair
6037833c9 Merge bitcoin-core/secp256k1#1702: changelog: update
40b4a0652 changelog: update
5e74086dc Merge bitcoin-core/secp256k1#1705: musig/test: Remove dead code
7c3380423 Merge bitcoin-core/secp256k1#1696: build: Refactor visibility logic and add override
8d967a602 musig/test: Remove dead code
983711cd6 musig/tests: Refactor vectors_signverify
73a695958 Merge bitcoin-core/secp256k1#1704: cmake: Make `secp256k1_objs` inherit interface defines from `secp256k1`
bf082221f cmake: Make `secp256k1_objs` inherit interface defines from `secp256k1`
c82d84bb8 build: add CMake option for disabling symbol visibility attributes
ce7923874 build: Add SECP256K1_NO_API_VISIBILITY_ATTRIBUTES
e5297f6d7 build: Refactor visibility logic
cbbbf3bd6 Merge bitcoin-core/secp256k1#1699: ci: enable musig module for native macOS arm64 job
943479a7a Merge bitcoin-core/secp256k1#1694: Revert "cmake: configure libsecp256k1.pc during install"
3352f9d66 ci: enable musig module for native macOS arm64 job
ad60ef7ea Merge bitcoin-core/secp256k1#1689: ci: Convert `arm64` Cirrus tasks to GHA jobs
c49877909 Merge bitcoin-core/secp256k1#1687: cmake: support the use of launchers in ctest -S scripts
44b205e9e Revert "cmake: configure libsecp256k1.pc during install"
0dfe387db cmake: support the use of launchers in ctest -S scripts
89096c234 Merge bitcoin-core/secp256k1#1692: cmake: configure libsecp256k1.pc during install
7106dce6f cmake: configure libsecp256k1.pc during install
29e73f4ba Merge bitcoin-core/secp256k1#1685: cmake: Emulate Libtool's behavior on FreeBSD
746e36b14 Merge bitcoin-core/secp256k1#1678: cmake: add a helper for linking into static libs
a28c2ffa5 Merge bitcoin-core/secp256k1#1683: README: add link to musig example
2a9d37473 Merge bitcoin-core/secp256k1#1690: ci: Bump GCC snapshot major version to 16
add146e10 ci: Bump GCC snapshot major version to 16
004f57fcd ci: Move Valgrind build for `arm64` from Cirrus to GHA
5fafdfc30 ci: Move `gcc-snapshot` build for `arm64` from Cirrus to GHA
e814b79a8 ci: Switch `arm64_debian` from QEMU to native `arm64` Docker image
bcf77346b ci: Add `arm64` architecture to `docker_cache` job
b77aae922 ci: Rename Docker image tag to reflect architecture
145ae3e28 cmake: add a helper for linking into static libs
819210974 README: add link to musig example, generalize module enabling hint
95db29b14 Merge bitcoin-core/secp256k1#1679: cmake: Use `PUBLIC_HEADER` target property in installation logic
37dd422b5 cmake: Emulate Libtool's behavior on FreeBSD
f24b838be Merge bitcoin-core/secp256k1#1680: doc: Promote "Building with CMake" to standard procedure
3f31ac43e doc: Promote "Building with CMake" to standard procedure
6f67151ee cmake: Use `PUBLIC_HEADER` target property
c32715b2a cmake, move-only: Move module option processing to `src/CMakeLists.txt`
201b2b8f0 Merge bitcoin-core/secp256k1#1675: cmake: Bump minimum required CMake version to 3.22
3af71987a cmake: Bump minimum required CMake version to 3.22
92394476e Merge bitcoin-core/secp256k1#1673: Assert field magnitude at control-flow join
3a4f448cb Assert field magnitude at control-flow join

git-subtree-dir: src/secp256k1
git-subtree-split: 36e76952cbf1cf54ddd2d8756cc31a486e2ba1d9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants