Implement secp256k1 error callbacks #89
Conversation
It must be 0,1,2 or 3, this is an explicit requirement of the secp256k1 library.
These callbacks are only triggered either by arguments do not match explicit rquirements of the sepc256k1 library, or by hardware failures, memory corruption or bug in secp256k1, and not by misuse of the library. In theory we do not need to implement them, except to find bugs in our own code, but the default callbacks print a message to stderr and call abort() which is not nice especially on mobile apps. => Here we introduce 2 specific exceptions, Secp256k1ErrorCallbackException and Secp256k1IllegalCallbackException, which are thrown when the error callback or illegal callback are called.
sstone
changed the title
| private typealias MyHandler = (String) -> Unit | ||
|
|
||
| private object CallbackHandler { | ||
| var illegalCallBackMessage: String? = null |
There was a problem hiding this comment.
Isn't it an issue that this value (and errorCallBackMessage) isn't thread-safe? Shouldn't we use an atomic ref or something synchronized?
There was a problem hiding this comment.
Using a global callback is a wrong approach, it has to "point" to something that is local to the call site as in the JNI implementation, it's harder here because of the limitations of kotlin's c-interop but I'll rework this.
There was a problem hiding this comment.
In 0364ec7 I use callbacks that are local to the call site.
|
There is an issue since the secp256k1 context is global: none of this is thread-safe, which can be exploited since we have many concurrent threads doing secp256k1 operations. If an attacker injects invalid input that would trigger one of those callbacks, depending on timing, it may fail another unrelated operation that was perfectly valid. I don't think there's any simple way around this, so we should probably just scan all |
|
Yes, with our current model and one global native context (and I don't see yet what else we could do), implementing callbacks does not really work. |
|
I checked and bitcoin-core also uses a global secp256k1 context and does not set error callbacks. |
|
Good idea to check bitcoin core, I think we're making the right move! |
These callbacks are only triggered either by arguments do not match explicit requirements of the sepc256k1 library, or by hardware failures, memory corruption or bug in secp256k1, and not by misuse of the library.
In theory we do not need to implement them, except to find bugs in our own code, but the default callbacks print a message to stderr and call abort() which is not nice especially on mobile apps.
=> Here we introduce 2 specific exceptions,
Secp256k1ErrorCallbackExceptionandSecp256k1IllegalCallbackException, which are thrown when the error callback or illegal callback are called.