Add ARG_CHECKs to ensure “array of pointers” elements are non-NULL #1779

1. 
   theStack commented at 1:05 am on December 6, 2025: contributor

   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: https://github.com/bitcoin-core/secp256k1/blob/e7f7083b530a55c83ce9089a7244d2d9d67ac8b2/src/secp256k1.c#L774-L775

   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.

2. Add ARG_CHECKs to ensure "array of pointers" elements are non-NULL 5a08c1bcdc
3. 
   furszy commented at 4:50 pm on December 6, 2025: member

   Cool, the review paid off so fast :). Would be good to add test coverage for it.
4. in src/secp256k1.c:332 in 5a08c1bcdc outdated

   324@@ -325,8 +325,13 @@ static int secp256k1_ec_pubkey_sort_cmp(const void* pk1, const void* pk2, void *
   325 }
   326 
   327 int secp256k1_ec_pubkey_sort(const secp256k1_context* ctx, const secp256k1_pubkey **pubkeys, size_t n_pubkeys) {
   328+    size_t i;
   329+
   330     VERIFY_CHECK(ctx != NULL);
   331     ARG_CHECK(pubkeys != NULL);
   332+    for (i = 0; i < n_pubkeys; i++) {
   

   ---

   ---

   

   kevkevinpal commented at 4:40 am on December 8, 2025:

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

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

   ---

   

   real-or-random commented at 8:05 am on December 8, 2025:

   I think it’s okay to sort the empty array.
5. 
   real-or-random commented at 8:05 am on December 8, 2025: contributor

   Concept ACK
6. real-or-random added the label tweak/refactor on Dec 8, 2025
7. real-or-random added the label assurance on Dec 8, 2025
8. test: Add non-NULL checks for "pointer of array" API functions 8bcda186d2
9. 
   theStack commented at 0:49 am on December 9, 2025: contributor

   Added CHECK_ILLEGAL tests to ensure NULL pointers in “array of pointer” arguments for the mentioned five functions are detected. They are placed right after (or at least as close to) successful executions with n_{pubkeys,pubnonces,sigs} >= 2 each and test with a single NULL pointer at each position.
10. real-or-random approved
11. 
    real-or-random commented at 7:51 am on December 9, 2025: contributor

    utACK 8bcda186d20e11ba9a04e6917928f062513b534f
12. 
    kevkevinpal commented at 1:12 pm on December 9, 2025: none

    utACK 8bcda18
13. 
    furszy commented at 4:31 pm on December 9, 2025: member

    As we have to perform these three checks in many places:

    1. Pointer to array is not null.
    2. Provided number of elements in the array is greater than zero.
    3. No element inside the array is null

    What about introducing a new macro?

    0/* Check array of pointers is non-NULL, non-empty, and has no NULL entries */
    1#define ARG_CHECK_ARRAY(arr, n) do {                                    \
    2        size_t _pos;                                                    \
    3        ARG_CHECK((arr) != NULL);                                       \
    4        ARG_CHECK((n) > 0);                                             \
    5        for (_pos = 0; _pos < (n); _pos++) {                            \
    6            ARG_CHECK((arr)[_pos] != NULL);                             \
    7        }                                                               \
    8} while(0)
    

    Could also introduce one for when the array can be empty too.

14. 
    john-moffett commented at 7:18 pm on December 9, 2025: contributor

    utACK 8bcda186d20e11ba9a04e6917928f062513b534f

    I like the macro idea, but I’d make the non-empty part explicit, like ARG_CHECK_ARRAY_NONEMPTY or something.

15. w0xlt approved
16. 
    w0xlt commented at 7:50 am on December 10, 2025: none

    Looks good to me. ACK https://github.com/bitcoin-core/secp256k1/pull/1779/commits/8bcda186d20e11ba9a04e6917928f062513b534f
17. 
    real-or-random commented at 8:24 am on December 10, 2025: contributor

    What about introducing a new macro?

    Concept ~0: It’s more DRY, and it will make it harder for people to forget one of the checks, e.g., that the members are non-NULL, in new code. On the other hand, I believe the code will be more difficult to read with a macro. Having the three checks explicitly is not super verbose, and noone will need to look up or remember the definition of the macro. (Note that we’d use the macro only in a handful of functions, so people may not recall the definiton.)

18. real-or-random merged this on Dec 10, 2025
19. real-or-random closed this on Dec 10, 2025

    ---

20. theStack deleted the branch on Dec 10, 2025

Contributors
theStack furszy kevkevinpal real-or-random john-moffett w0xlt

Labels
assurance tweak/refactor