Allow to use external default callbacks

[real-or-random]

This is intended for environments without implementations for abort(), fprintf(), and stderr. e.g., embedded systems. Those can provide their own implementations of default_illegal_callback_fn and default_error_callback_fn at compile time.

If you want to use your own default callback, things will be somewhat inconsistent unfortunately: We cannot make the callback data extern too, because then the initialization lists for default_illegal_callback won't contain only constants. (const variables are not compile-time constants). So you cannot take callback data in your own default callback function.

As a more drastic/breaking alternative I suggest to remove the callback data entirely. I don't think it's a big loss and I would be surprised if anyone uses it. Additionally, we could even remove the possibility to set the callback function at runtime after this PR. This will simplify things a lot, and again I don't think it's a big loss.

Note that abort(), fprintf(), and stderr are also used in CHECK, which is still used in production code if we rely on gmp for scalar and field inversions (e.g., https://github.com/bitcoin-core/secp256k1/blob/master/src/scalar_impl.h#L240). This is not an issue for embedded system which probably don't want to use gmp anyway, but it is probably an issue for the reasons explained in #566 (comment).

(related downstream: rust-bitcoin/rust-secp256k1#100 @elichai)

[gmaxwell]

The kinds of enviroments where you couldn't use the defaults are also ones where you're going to be custom compiling this and not using it as a system library.

Having a compile mode where the callbacks get converted to static dispatches to a compile time specified function sounds good to me (function pointers in structs on the stack the #2 way for stack overwrites to get escalated into attacker control of execution flow (#1 is smashing a return address)).

[real-or-random]

As a more drastic/breaking alternative I suggest to remove the callback data entirely. I don't think it's a big loss and I would be surprised if anyone uses it. Additionally, we could even remove the possibility to set the callback function at runtime after this PR. This will simplify things a lot, and again I don't think it's a big loss.

Another advantage of this will be that we could call the error functions from everywhere without carrying around the context. This would make it easier to replace the CHECKs in scalar and field inversions without passing the context deeply (and somewhat arbitrarily) into arithmetic functions.

[jonasnick]

So the idea is that you link your own implementations of default_{illegal,error}_callback_fn if libsecp is configured with --enable-external-default-callback. CHECK is replaced by ARG_CHECK_NO_RETURN because CHECK requires abort, fprintf, stderr. ARG_CHECK doesn't work because the functions it's used in return void.

@real-or-random The remaining use of CHECK in secp256k1_fe_inv_var isn't a problem?

As far as I can evaluate LGTM but I haven't tried to link against my own implementation.

[real-or-random]

Thanks for adding a better description of the changes!

@real-or-random The remaining use of CHECK in secp256k1_fe_inv_var isn't a problem?

It's not great but it's not a problem if you want to get rid of the standard library symbols: all the remaining uses of CHECK can be turned off by appropriate defines such as USE_FIELD_INV_BUILTIN.

Yes, I also haven't tried to link against my own implementation. I should test this before this PR can be considered ready.

[gmaxwell]

Any reason to not change the optional internal use of CHECK to work like malloc failure does?

(FWIW, those inverse things should go away because we should get an internal fast inverse and ditch GMP... but thats a lot more than a couple line change :) )

[real-or-random]

Any reason to not change the optional internal use of CHECK to work like malloc failure does?

I think then you need the callback, which means that you need to pass the context or the callback pointer into these arithmetic functions, probably multiple levels deep. It's doable but it does not seem to be a great solution.

[real-or-random]

As a more drastic/breaking alternative I suggest to remove the callback data entirely. I don't think it's a big loss and I would be surprised if anyone uses it. Additionally, we could even remove the possibility to set the callback function at runtime after this PR. This will simplify things a lot, and again I don't think it's a big loss.

What's your opinion on this? Too breaking? If the callbacks were not configurable that, we could replace the internal CHECKs easily.

[real-or-random]

Interfacing this with external callback functions works but it feels indeed somewhat cumbersome:

• the functions don't have a secp256_ prefix (fixed with latest commit)
• the functions take a data pointer but they need to be prepared ignore it because at least before calling one of the set_x_callback functions, it will always be NULL (see previous comment). I don't think it's a showstopper though.

[sipa]

If you want to use your own default callback, things will be somewhat inconsistent unfortunately: We cannot make the callback data extern too, because then the initialization lists for default_illegal_callback won't contain only constants. (const variables are not compile-time constants). So you cannot take callback data in your own default callback function.

I don't think this is a problem. The reason for the data pointer is so that you can pass runtime dependent data into the callback. In cases where such runtime data exist, it's always possible to call secp256k1_context_set_illegal_callback/secp256k1_context_set_error_callback.

As a more drastic/breaking alternative I suggest to remove the callback data entirely. I don't think it's a big loss and I would be surprised if anyone uses it.

Having searched around a bit, I think this is fine. I'm a bit unclear about how to roll it out - do we just start ignoring the data argument (and cause failure (?) when it's non-null in the set_*_callback functions), or do we break the API and rip out the arguments? It's unclear what kind of breaks downstream library tolerate here. There is at least one place I found that actually does call the set_*_callback outside of tests: https://github.com/ethereum/go-ethereum/blob/master/crypto/secp256k1/secp256.go .

Additionally, we could even remove the possibility to set the callback function at runtime after this PR. This will simplify things a lot, and again I don't think it's a big loss.

I think that's going too far. There are some places where the library is used in a shared library context, where anything but runtime-set callbacks is unclear at least. Also in situations where the library is used by different layers of a program (say, indrectly through a library like libwally, but also directly for network ECDH negotiations) and those layers want to have distinct behavior.

(FWIW, those inverse things should go away because we should get an internal fast inverse and ditch GMP... but thats a lot more than a couple line change :) )

That would be great.

[gmaxwell]

Also in situations where the library is used by different layers of a program (say, indrectly through a library like libwally, but also directly for network ECDH negotiations) and those layers want to have distinct behavior.

Is that actually the case? I think that for the cases where the callbacks are called, only something terrible has happened and you're going to want to shut down ... even if the context was some totally boring network facing thing. Simply because it means that your memory might be corrupted and attempting to continue could be bad.

[real-or-random]

I see the point with the shared library context (no matter if we different layers need different callbacks). But given that, I think we should keep the API as it is. After playing around with the code, I'm rather convinced that having data pointers in default callbacks is fine; it just needs to be documented properly that the default callbacks always need to be prepared to get a NULL pointer. I'll add a commit that updates the header file.

By the way, I've experimented also with a branch that allows external pointers to callback data too (in the end the stupid C99 restriction is not essential and can be worked around). But I don't think it's very helpful. It just adds complexity to the API for no real benefit.

[real-or-random]

Force-pushed. All the changes are in comments in secp256k1.h, which hasn't been touched before.

[real-or-random]

I've added a commit that adds this as #undef to base-config.h (and a few more which were missing).

[jonasnick]

I can confirm that linking external default callbacks works. ACK mod nit.

[gmaxwell]

The only use for the data argument that I'm aware of is the use that we have in the tests: carrying in a counter for test instrumentation. This could be replaced in our tests by providing a callback function that modifies a global.

Is anyone aware of anything else?

Regarding compatibility, removing an argument is the most minor of API changes. I don't think we should let that hold us back if we think its the right change.

[real-or-random]

I force-pushed to address @jonasnick's nit.

@gmaxwell
As I said above, I don't think that the data pointer is a real issue in the current version of this PR, so this is fine as it is. If someone replaces the default callbacks, then he know what he's doing and has read the docs.

[gmaxwell]

utACK

[gmaxwell]

Needs rebase (sorry!)

[real-or-random]

rebased