tests: Fix GCC 17 snapshot warning #1881

pull real-or-random wants to merge 1 commits into bitcoin-core:master from real-or-random:202606-stack-pointer-free changing 1 files +16 −10

real-or-random commented at 9:22 AM on June 25, 2026: contributor
Passing a non-malloc pointer to free() would be UB. In this case, the free() line is never actually reached (and GCC 17 fails to prove this) in a correct implementation of secp256k1_scratch_space_destroy(), but the test shouldn't rely on the correctness of the tested function.

Alternative to #1880. I think this is cleaner
- it doesn't recreate the scratch space in the middle of some other tests
- it has an obvious matching (malloc, free) pair
- it additionally checks that only magic is accessed
real-or-random added the label bug on Jun 25, 2026
real-or-random added the label assurance on Jun 25, 2026
theStack commented at 9:58 AM on June 25, 2026: contributor

ACK 715e954abe77ece9f16955f5ae83191896582aef

(I wonder if strictly speaking the current code is really UB though? The problematic free call is never reached, as _scratch_destroy with the non-malloc pointer returns early due to invalid magic)

in src/tests.c:424 in 715e954abe outdated

 418 | -    CHECK_ERROR(CTX, secp256k1_scratch_max_allocation(&CTX->error_callback, scratch, 0));
 419 | -    CHECK_ERROR(CTX, secp256k1_scratch_alloc(&CTX->error_callback, scratch, 500));
 420 | -    CHECK_ERROR_VOID(CTX, secp256k1_scratch_space_destroy(CTX, scratch));
 421 | -
 422 |      /* Test that large integers do not wrap around in a bad way */
 423 | -    scratch = secp256k1_scratch_space_create(CTX, 1000);

theStack commented at 11:03 AM on June 25, 2026:

was this line removed intentionally?

real-or-random commented at 12:46 PM on June 25, 2026:

Yes, we don't need to recreate the scratch because the call secp256k1_scratch_space_destroy() in line 416 is also removed.

real-or-random commented at 12:51 PM on June 25, 2026: contributor

(I wonder if strictly speaking the current code is really UB though? The problematic free call is never reached, as _scratch_destroy with the non-malloc pointer returns early due to invalid magic)

Ok, sure, you're right. I missed this.

But this argument also only holds if the tested code is correct, and the test shouldn't assume this. (Of course, the test may assume that the tested code doesn't invoke UB, because if it does, there's nothing that the can meaningfully do. But at least the test itself shouldn't introduce additional UB.)
real-or-random force-pushed on Jun 25, 2026
real-or-random renamed this:
~~tests: Don't pass non-malloc pointer to free()~~
tests: Fix GCC 17 snapshot warning
on Jun 25, 2026
real-or-random commented at 12:57 PM on June 25, 2026: contributor

(I wonder if strictly speaking the current code is really UB though? The problematic free call is never reached, as _scratch_destroy with the non-malloc pointer returns early due to invalid magic)

Ok, sure, you're right. I missed this.

I force-pushed to update the commit message to reflect this. I also changed the PR description accordingly.

tests: Fix GCC 17 snapshot warning

Passing a non-malloc pointer to free() would be UB. In this case, the
free() line is never actually reached (and GCC 17 fails to prove this)
in a correct implementation of secp256k1_scratch_space_destroy(), but
the test shouldn't rely on the correctness of the tested function.

9d75769dec

real-or-random force-pushed on Jun 25, 2026
real-or-random commented at 12:58 PM on June 25, 2026: contributor

And force-pushed again to rebase on master (to avoid the macOS failure in CI).
hebasto approved
hebasto commented at 1:45 PM on June 25, 2026: member

ACK 9d75769dec4fcb40ee7e41f674f22f3f5e5cfc7e, I have reviewed the code and it looks OK.
theStack approved
theStack commented at 2:29 PM on June 25, 2026: contributor

re-ACK 9d75769dec4fcb40ee7e41f674f22f3f5e5cfc7e
real-or-random merged this on Jun 25, 2026
real-or-random closed this on Jun 25, 2026

Contributors

Labels

Linked (view graph)

#1880 test: avoid scratch free warning