call the alloc error handle if we get NULL from the allocator

[elichai]

Found that this was missing in this discussion: #529 (comment)

It is documented here that it returns a NULL on memory exhaustion: https://doc.rust-lang.org/alloc/alloc/trait.GlobalAlloc.html#tymethod.alloc
And you can see that this is called in this example: https://doc.rust-lang.org/alloc/alloc/fn.alloc.html
Docs for the handle itself: https://doc.rust-lang.org/alloc/alloc/fn.handle_alloc_error.html

[Kixunil]

I wonder if we should return null instead since that'd allow custom handling of allocation failures.

[elichai]

I wonder if we should return null instead since that'd allow custom handling of allocation failures.

Generally in rust allocation failures call this handler (see all the collections in liballoc)
And if someone writes code for kernels they probably want to use the preallocated api and allocate memory themselves.
But I've never written a kernel in rust, so I hard for me to say this with confidence

[Kixunil]

I'm more concerned about keeping the same API as the original C function which presumably didn't call into any Rust functions.
I'd be surprised if anyone used this in a kernel.

[elichai]

I'm more concerned about keeping the same API as the original C function which presumably didn't call into any Rust functions.
I'd be surprised if anyone used this in a kernel.

Upstream calls the error callback on that, which aborts by default: https://github.com/bitcoin-core/secp256k1/blob/2286f8090242098a33f0d85b27c48e58d4235df1/src/util.h#L133

[apoelstra]

ACK 8b17fc0

[apoelstra]

@Kixunil if "not calling into Rust functions" violates the C API then we've already violated it, as @elichai says, by replacing the default error/arg callbacks with our own. (We had to do that because the C ones called libc functions that we don't have on WASM.)

My view is that when it comes to allocation-related stuff, it's better to replace C with Rust wherever we can, because Rust is easier to reason about and will be more consistent with the surrounding Rust code that this crate will be compiled into.

Upstream calls the error callback on that, which aborts by default

I think this is a reasonable argument, and I'm happy to ACK/merge this because it "behaves the same as the C code". That is, it calls an error callback which aborts, but this callback can be replaced by the user (in C-land, using context_set_error_callback etc or by replacing the default one at compile time; in Rust-land by some function in the alloc API).

I also think that this is the Rustic way to approach this, because Rust code typically does not expose allocation failures to the user except if they go through contortions like replacing the alloc-failure callback. I'm curious @Kixunil if you disagree.

[Kixunil]

Good argument, ACK 8b17fc0