tests: `secp256k1_ecmult_multi_var` is called with a `NULL` error callback

[niooss-ledger]

Hello,

In the tests, function test_ecmult_accumulate calls secp256k1_ecmult_multi_var with error_callback = NULL (since version 0.2.0, PR #920):

secp256k1/src/tests.c

Lines 5497 to 5498 in 7712a53

	secp256k1_ecmult_multi_var(NULL, scratch, &rj4, x, NULL, NULL, 0);
	secp256k1_ecmult_multi_var(NULL, scratch, &rj5, &secp256k1_scalar_zero, test_ecmult_accumulate_cb, (void*)x, 1);

This function eventually calls secp256k1_scratch_max_allocation:

secp256k1/src/scratch_impl.h

Lines 58 to 60 in 7712a53

	static size_t secp256k1_scratch_max_allocation(const secp256k1_callback* error_callback, const secp256k1_scratch* scratch, size_t objects) {
	if (secp256k1_memcmp_var(scratch->magic, "scratch", 8) != 0) {
	secp256k1_callback_call(error_callback, "invalid scratch space");

... which directly dereferences the callback parameter:

secp256k1/src/util.h

Lines 86 to 87 in 7712a53

	static SECP256K1_INLINE void secp256k1_callback_call(const secp256k1_callback * const cb, const char * const text) {
	cb->fn(text, (void*)cb->data);

In short, it seems secp256k1_ecmult_multi_var does not expect error_callback to be NULL.

The consequence of test_ecmult_accumulate not following this expectation would be a possible crash (by null pointer dereference) if something ever go wrong in the test. While this bug does not directly impact secp256k1 library (it occurs in the test suite), I believe this issue should be fixed because I think tests should follow the calling convention of the library functions (such as not passing NULL where functions expects non-NULL parameters).

Moreover, CHECK() could probably be added to verify the result of secp256k1_ecmult_multi_var. Therefore, I am suggesting this change:

diff --git a/src/tests.c b/src/tests.c
index 2eb3fbfdcea7..dab47608c2e5 100644
--- a/src/tests.c
+++ b/src/tests.c
@@ -5494,8 +5494,8 @@ static void test_ecmult_accumulate(secp256k1_sha256* acc, const secp256k1_scalar
     secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &rj1, x);
     secp256k1_ecmult(&rj2, &gj, x, &secp256k1_scalar_zero);
     secp256k1_ecmult(&rj3, &infj, &secp256k1_scalar_zero, x);
-    secp256k1_ecmult_multi_var(NULL, scratch, &rj4, x, NULL, NULL, 0);
-    secp256k1_ecmult_multi_var(NULL, scratch, &rj5, &secp256k1_scalar_zero, test_ecmult_accumulate_cb, (void*)x, 1);
+    CHECK(secp256k1_ecmult_multi_var(&CTX->error_callback, scratch, &rj4, x, NULL, NULL, 0));
+    CHECK(secp256k1_ecmult_multi_var(&CTX->error_callback, scratch, &rj5, &secp256k1_scalar_zero, test_ecmult_accumulate_cb, (void*)x, 1));
     secp256k1_ecmult_const(&rj6, &secp256k1_ge_const_g, x);
     secp256k1_ge_set_gej_var(&r, &rj1);
     CHECK(secp256k1_gej_eq_ge_var(&rj2, &r));

Would such a change be acceptable? (If yes, I can submit a pull request)

Moreover, should some attributes SECP256K1_ARG_NONNULL be added to functions expecting non-NULL error_callback too?

[real-or-random]

Thanks, I agree with the analysis. Note that secp256k1_ecmult_multi_var (and the entire scratch space machinery) is currently unused. It was added for future performance improvements such as batch verification.

Can I ask you how you found this? Simply by eyeballing or using some (semi-)automated analysis?

Would such a change be acceptable? (If yes, I can submit a pull request)

Yes. (Yes!)

Moreover, should some attributes SECP256K1_ARG_NONNULL be added to functions expecting non-NULL error_callback too?

Hm, I don't think so. I mean, it's not wrong to do this, but our convention so far is that SECP256K1_ARG_NONNULL is only used for arguments of public API functions. You could, however, add VERIFY_CHECK(error_callback != NULL) inside the affected functions instead.

[niooss-ledger]

Thanks for your quick reply :)

Can I ask you how you found this? Simply by eyeballing or using some (semi-)automated analysis?

I was running some static analysis tools against the latest release (0.5.0) and reviewed the findings from clang's static analyzer (scan-build with many options, such as -enable-checker security -enable-checker unix). There were many false positives but it also reported Access to field 'fn' results in a dereference of a null pointer (loaded from variable 'cb'). I tried to understand whether this was an actual issue and analyzed this report more deeply, which led to this issue.

Would such a change be acceptable? (If yes, I can submit a pull request)

Yes. (Yes!)

Ok, I created #1528

Moreover, should some attributes SECP256K1_ARG_NONNULL be added to functions expecting non-NULL error_callback too?

Hm, I don't think so. I mean, it's not wrong to do this, but our convention so far is that SECP256K1_ARG_NONNULL is only used for arguments of public API functions. You could, however, add VERIFY_CHECK(error_callback != NULL) inside the affected functions instead.

All right. This makes sense and I will not add such VERIFY_CHECK invocations.