Verify non-null data arg in ellswift xdh_hash_function_prefix

[furszy]

The secp256k1_ellswift_xdh API has a data arg that is optional for one
of the hash functions (unused) but required for the other.

This adds a simple check to provide a clear error instead of crashing
due to an out-of-bounds memory access when the user mistakenly provides
data=NULL and selects the xdh_hash_function_prefix() function.

Note: Open to improving the arg description in the API as well. I'm just adding
this check because a better error would have saved me a few minutes.

[sipa]

ACK f4708d2

[real-or-random]

when the user mistakenly provides
data=NULL

Note that VERIFY_CHECKs are only active in the tests.

[furszy]

when the user mistakenly provides
data=NULL

Note that VERIFY_CHECKs are only active in the tests.

We can't use ARG_CHECK because ellswift_xdh_hash_function_prefix() doesn’t receive the context (for now). #1777 will solve that and let us add the proper arg check.

Update: We probably could leave this open until the other PR gets merged.

[real-or-random]

We can't use ARG_CHECK because ellswift_xdh_hash_function_prefix() doesn’t receive the context (for now). #1777 will solve that and let us add the proper arg check.

Unfortunately, not exactly. ellswift_xdh_hash_function_prefix() now gets the static context, hardcoded. But we need the real context to call the right illegal_callback.

I think this is an oversight in the API design.

Whenever there's an argument where the user can provide a hash function (nonce derivation functions and hashing the raw secret in ECDH/Ellswift), we expose a pointer to our built-in functions in the API. These functions don't get a context object, which makes sense: the corresponding function type doesn't allow for a context object because user-provided functions aren't meant to deal with context objects.

But what perhaps doesn't make sense is that we expose these function pointers at all. The user isn't really supposed to call these functions directly. If anything, the user is supposed to pass these function pointers back to our API functions. So they should have been rather something like "handles" or enum values representing the built-in functions.

In fact, we always have NULL as a value representing the default built-in, but at least in the ellswift module, there are two built-in functions, one of them being the default. And one of them even makes use of the data pointer, so we can't even abuse the data pointer to pass the context object...

All of this is a bit unsatisfactory because it means that the runtime SHA256 won't work for nonce derivation and ECDH hashing.

For this PR, using ARG_CHECK will still be okay, I think. Then we call the default illegal callback, which is still better than dereferencing a NULL pointer.

[real-or-random]

My previous comment was a bit mistaken, sorry. We mostly use the override because we compare against NULL and the built-in function pointers (and yes, I remember that this came up during the review.) The only way some built-in functions would not use the override is when user code calls it manually (e.g., when they provide a simple wrapper for logging purposes -- not a great example, I know). This is probably good enough in practice, but we should perhaps document it. See #1835

In either case, the conclusion for this PR here is that ARG_CHECK is good