What the developer wrote
Authored by Tim Ruffing
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.
What changed, and why it matters
This is a test-only cleanup. The commit removes a unit test pattern that passed a stack-allocated (non-malloc) scratch-space object to a destroy function that could call free(), which is undefined behavior. The test logic is reimplemented using a heap-allocated object instead. It does not change the production library code that Bitcoin or other applications use.
No production action needed. The change is a quality improvement to the test suite and can be merged as part of normal maintenance.
Security signals we found
Undefined behavior in test code: passing a stack pointer to a function that may call free()
Test relied on the tested function not reaching the free() path
No change to library implementation or public API behavior
Evidence from the diff
In src/tests.c, the run_scratch_tests function previously created a local (stack-allocated) secp256k1_scratch_space, zeroed it, and passed it to secp256k1_scratch_space_destroy(). If that destroy function ever reached its free() call, it would free a non-heap pointer, producing undefined behavior. The commit moves this invalid-scratch test into a new run_invalid_scratch_space_tests helper, allocates the scratch object with checked_malloc, and then calls free() on it explicitly. This silences a GCC 17 warning and removes reliance on the correctness of the tested destroy function. No production code is modified.
Changed components
src/tests.crun_scratch_testsnew run_invalid_scratch_space_testsInspect captured patch +16 / −10
diff --git a/src/tests.c b/src/tests.c
index 6c3cd39..b0d94d6 100644
--- a/src/tests.c
+++ b/src/tests.c
@@ -372,7 +372,6 @@ static void run_scratch_tests(void) {
size_t checkpoint;
size_t checkpoint_2;
secp256k1_scratch_space *scratch;
- secp256k1_scratch_space local_scratch;
/* Test public API */
scratch = secp256k1_scratch_space_create(CTX, 1000);
@@ -412,16 +411,7 @@ static void run_scratch_tests(void) {
CHECK_ERROR_VOID(CTX, secp256k1_scratch_apply_checkpoint(&CTX->error_callback, scratch, checkpoint_2)); /* checkpoint_2 is after checkpoint */
CHECK_ERROR_VOID(CTX, secp256k1_scratch_apply_checkpoint(&CTX->error_callback, scratch, (size_t) -1)); /* this is just wildly invalid */
- /* try to use badly initialized scratch space */
- secp256k1_scratch_space_destroy(CTX, scratch);
- memset(&local_scratch, 0, sizeof(local_scratch));
- scratch = &local_scratch;
- CHECK_ERROR(CTX, secp256k1_scratch_max_allocation(&CTX->error_callback, scratch, 0));
- CHECK_ERROR(CTX, secp256k1_scratch_alloc(&CTX->error_callback, scratch, 500));
- CHECK_ERROR_VOID(CTX, secp256k1_scratch_space_destroy(CTX, scratch));
-
/* Test that large integers do not wrap around in a bad way */
- scratch = secp256k1_scratch_space_create(CTX, 1000);
/* Try max allocation with a large number of objects. Only makes sense if
* ALIGNMENT is greater than 1 because otherwise the objects take no extra
* space. */
@@ -436,6 +426,21 @@ static void run_scratch_tests(void) {
secp256k1_scratch_space_destroy(CTX, NULL); /* no-op */
}
+/* try to use badly initialized scratch space */
+static void run_invalid_scratch_space_tests(void) {
+ secp256k1_scratch_space* scratch = checked_malloc(&CTX->error_callback, sizeof(*scratch));
+ size_t magic_size = sizeof(scratch->magic);
+ memset(scratch, 0, sizeof(*scratch));
+ /* catch accesses beyond the magic */
+ SECP256K1_CHECKMEM_UNDEFINE((unsigned char*)scratch + magic_size, sizeof(*scratch) - magic_size);
+
+ CHECK_ERROR(CTX, secp256k1_scratch_max_allocation(&CTX->error_callback, scratch, 0));
+ CHECK_ERROR(CTX, secp256k1_scratch_alloc(&CTX->error_callback, scratch, 500));
+ CHECK_ERROR_VOID(CTX, secp256k1_scratch_space_destroy(CTX, scratch));
+
+ free(scratch);
+}
+
/* A compression function that does nothing */
static void invalid_sha256_compression(uint32_t *s, const unsigned char *msg, size_t rounds) {
(void)s; (void)msg; (void)rounds;
@@ -7935,6 +7940,7 @@ static const struct tf_test_entry tests_general[] = {
CASE(all_static_context_tests),
CASE(deprecated_context_flags_test),
CASE(scratch_tests),
+ CASE(invalid_scratch_space_tests),
CASE(plug_sha256_compression_tests),
CASE(sha256_multi_block_compression_tests),
};
Why this scored 17/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.