Skip to content

Conversation

@theStack
Copy link
Contributor

@theStack theStack commented Dec 6, 2025

We currently have five public API functions that take an "array of pointers" as input parameter:

  • secp256k1_ec_pubkey_combine (ins: array of pointers to public keys to add)
  • secp256k1_ec_pubkey_sort (pubkeys: array of pointers to public keys to sort)
  • secp256k1_musig_pubkey_agg (pubkeys: array of pointers to public keys to aggregate)
  • secp256k1_musig_nonce_agg (pubnonces: array of pointers to public nonces to aggregate)
  • secp256k1_musig_partial_sig_agg (partial_sigs: array of pointers to partial signatures to aggregate)

Out of these, only _ec_pubkey_combine verifies that the individual pointer elements in the array are non-NULL each:

secp256k1/src/secp256k1.c

Lines 774 to 775 in e7f7083

for (i = 0; i < n; i++) {
ARG_CHECK(pubnonces[i] != NULL);

This PR adds corresponding ARG_CHECKS for the other API functions as well, in order to avoid running into potential UB due to NULL pointer dereference. It seems to me that the tiny run-time overhead is worth it doing this for consistency and to help users in case the arrays are set up incorrectly (I'm thinking e.g. of language binding writers where getting this right might be a bit more involved).

Looking into this was motivated by a review of furszy (thanks!), who pointed out that the non-NULL checks are missing in at least one API function in the silentpayments module PR as well. Happy to add some CHECK_ILLEGAL tests if there is conceptual support for this PR.

@furszy
Copy link
Member

furszy commented Dec 6, 2025

Cool, the review paid off so fast :). Would be good to add test coverage for it.


VERIFY_CHECK(ctx != NULL);
ARG_CHECK(pubkeys != NULL);
for (i = 0; i < n_pubkeys; i++) {

Choose a reason for hiding this comment

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

Does it make sense to check n_pubkeys is greater than 0?

Suggested change
for (i = 0; i < n_pubkeys; i++) {
ARG_CHECK(n_pubkeys > 0);
for (i = 0; i < n_pubkeys; i++) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants