Allow to use external default callbacks

real-or-random commented at 3:30 pm on March 4, 2019: contributor

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

gmaxwell commented at 11:43 pm on March 4, 2019: contributor

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 commented at 12:37 pm on March 5, 2019: contributor

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.

real-or-random cross-referenced this on Mar 6, 2019 from issue Make WINDOW_G configurable by real-or-random

in configure.ac:518 in 952908989b outdated

520-echo "  with coverage       = $enable_coverage"
521-echo "  module ecdh         = $enable_module_ecdh"
522-echo "  module recovery     = $enable_module_recovery"
523+echo "  with endomorphism       = $use_endomorphism"
524+echo "  with ecmult precomp     = $set_precomp"
525+echo "  with external callbacks = $set_precomp"

jonasnick commented at 10:22 pm on March 6, 2019:

s/set_precomp/use_external_default_callbacks

jonasnick commented at 11:21 pm on March 6, 2019: contributor

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 commented at 0:26 am on March 7, 2019: contributor

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.

real-or-random force-pushed on Mar 7, 2019

gmaxwell commented at 2:39 am on March 7, 2019: contributor

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 commented at 3:22 pm on March 7, 2019: contributor

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 commented at 10:36 am on March 9, 2019: contributor

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 commented at 10:45 am on March 9, 2019: contributor

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.

elichai cross-referenced this on Mar 11, 2019 from issue Add no-std support by elichai

sipa commented at 2:06 am on March 13, 2019: contributor

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 commented at 3:07 am on March 13, 2019: contributor

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 commented at 4:13 pm on March 13, 2019: contributor

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 on Mar 13, 2019

real-or-random commented at 4:59 pm on March 13, 2019: contributor

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

real-or-random commented at 3:23 pm on March 18, 2019: contributor

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

real-or-random cross-referenced this on Apr 1, 2019 from issue Changes necessary for usage on Trezor by real-or-random

in include/secp256k1.h:242 in a6add6cc8b outdated

237+ *  has been configured with --enable-external-default-callbacks. Then the
238+ *  following two symbols must be provided to link against:
239+ *   - void secp256k1_default_illegal_callback_fn(const char* message, void* data);
240+ *   - void secp256k1_default_error_callback_fn(const char* message, void* data);
241+ *  The library can call these default handlers even before a proper callback data
242+ *  pointer could have been using secp256k1_context_set_illegal_callback or

jonasnick commented at 2:23 pm on April 9, 2019:

It seems like a verb is missing here? “could have been [set] using”?

jonasnick commented at 9:51 am on April 10, 2019: contributor

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

elichai cross-referenced this on May 13, 2019 from issue Enable context creation in preallocated memory by real-or-random

gmaxwell commented at 10:14 pm on May 22, 2019: contributor

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 force-pushed on May 23, 2019

real-or-random commented at 1:19 pm on May 23, 2019: contributor

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 commented at 0:36 am on May 24, 2019: contributor

utACK

gmaxwell referenced this in commit 36698dcfee on May 25, 2019

gmaxwell commented at 10:46 pm on May 25, 2019: contributor

Needs rebase (sorry!)

Replace CHECKs for no_precomp ctx by ARG_CHECKs without a return 6095a863fa

Allow usage of external default callbacks 5db782e655

Include stdio.h and stdlib.h explicitly in secp256k1.c 908bdce64e

Add secp256k1_ prefix to default callback functions 77defd2c3b

Add missing #(un)defines to base-config.h e49f7991c2

real-or-random force-pushed on May 26, 2019

real-or-random commented at 8:40 pm on May 26, 2019: contributor

rebased

gmaxwell merged this on May 27, 2019

gmaxwell closed this on May 27, 2019

gmaxwell referenced this in commit 143dc6e9ee on May 27, 2019

elichai cross-referenced this on May 28, 2019 from issue Updating secp256k1 and supporting full no-std features by elichai

real-or-random cross-referenced this on Jun 13, 2019 from issue Adding message length to the callbacks by elichai

real-or-random cross-referenced this on Jun 17, 2019 from issue Low-footprint mode by gmaxwell

sipa cross-referenced this on Jun 9, 2020 from issue Update libsecp256k1 subtree by sipa

fanquake referenced this in commit 8c97780db8 on Jun 13, 2020

sidhujag referenced this in commit 8a3a072968 on Jun 13, 2020

ComputerCraftr referenced this in commit b98f1c6e6c on Jun 16, 2020

UdjinM6 referenced this in commit 9d36ba6570 on Aug 10, 2021

5tefan referenced this in commit 8ded2caa74 on Aug 12, 2021

elichai cross-referenced this on Nov 3, 2021 from issue New context API by real-or-random

gades referenced this in commit d855cc511d on May 8, 2022

Allow to use external default callbacks #595