Closed niooss-ledger closed 1 month ago
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.
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 https://github.com/bitcoin-core/secp256k1/pull/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, addVERIFY_CHECK(error_callback != NULL)
inside the affected functions instead.
All right. This makes sense and I will not add such VERIFY_CHECK
invocations.
Hello,
In the tests, function
test_ecmult_accumulate
callssecp256k1_ecmult_multi_var
witherror_callback = NULL
(since version 0.2.0, PR https://github.com/bitcoin-core/secp256k1/pull/920): https://github.com/bitcoin-core/secp256k1/blob/7712a53061b1e36ecf47a3a46ea1e67ef31904d9/src/tests.c#L5497-L5498This function eventually calls
secp256k1_scratch_max_allocation
: https://github.com/bitcoin-core/secp256k1/blob/7712a53061b1e36ecf47a3a46ea1e67ef31904d9/src/scratch_impl.h#L58-L60... which directly dereferences the callback parameter: https://github.com/bitcoin-core/secp256k1/blob/7712a53061b1e36ecf47a3a46ea1e67ef31904d9/src/util.h#L86-L87
In short, it seems
secp256k1_ecmult_multi_var
does not expecterror_callback
to beNULL
.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 impactsecp256k1
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 passingNULL
where functions expects non-NULL
parameters).Moreover,
CHECK()
could probably be added to verify the result ofsecp256k1_ecmult_multi_var
. Therefore, I am suggesting this change: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?