This came up in #1698.
secp256k1_pubkey_load will only return 0 when it triggers the illegal_callback. The docs of the callback say that if an API call triggers it, then the return value and the output arguments of the API call are “undefined”.
Now, what does this mean? In the case of the return value, this probably means something like “it’s a proper value, but it could be any”. The C standard calls this an “unspecified value” and it cannot have a trap representation. This at least guarantees that there won’t be any UB when using this value.
It’s more complicated when it comes to output arguments, as these are stored in memory, and C has a concept of an “indeterminate representation”, e.g., used for uninitialized stack variables. Reading one of these beasts might be UB, depending on which version of the standard you’re looking at, the compiler (writers) interpretation of it, and whether it’s static memory or not (whether is has “automatic storage duration”), and whether the memory has its address been taken (wtf!). I don’t think we’ll want to enter that territory, so we better read the API docs again as “unspecified value”…
Providing this guarantee that output args are “initialized” may be a good idea, and that means we should overwrite any output args when an ARG_CHECK fires, e.g., with all zeros. This is done, for instance, by the tweak functions. But the here is that this rule is difficult to follow for contributors if we have internal helper functions like secp256k1_ec_pubkey_load() that have ARG_CHECKs in it (or if one API functions calls another one) You can’t tell from its name that secp256k1_pubkey_load may call the illegal callback but you’ll need to know as a caller because you may need to guarantee that all API output args have been initialized before the call.
What could we do to improve this?
- Add a rule that says that
ARG_CHECKmay only appear in the body of API functions, and that API functions may not call each other. It appears to be the case that most instances ofARG_CHECKare at the top of API functions, so perhaps this rule could be implemented. But it will make the code harder to read in a number of places, e.g., we’ll have to create some “internal” versions of API functions. That’s annoying, at least. - Drop the guarantee. If production code is supposed to crash anyway in the callback, I tend to think that this is acceptable. @sipa I wonder what the initial thinking behind the guarantee was (if you remember… this is 10 years ago).