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

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.

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

Concept ACK

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.

utACK 8bcda186d20e11ba9a04e6917928f062513b534f

utACK 8bcda18

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?

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

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

utACK 8bcda186d20e11ba9a04e6917928f062513b534f

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

Looks good to me. ACK https://github.com/bitcoin-core/secp256k1/pull/1779/commits/8bcda186d20e11ba9a04e6917928f062513b534f

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.)