Don't #include standard library headers unconditionally

[real-or-random]

We currently include <stdio.h> for fprintf used in the a) tests and b) in the default error callbacks ... We should not include the header unconditionally.

secp256k1/src/util.h

Line 16 in 912b7cc

#include <stdio.h>

This is a problem downstream in rust-bitcoin/rust-secp256k1#421 .

edit:
We have a similar issue for stdlib.h but it is a little bit more complicated. It has abort, and malloc/free. The story for abort is the same as for fprintf. Strictly speaking one doesn't need malloc/free if one uses the prealloc interface but we don't provide consistent include headers for this. (You'll need the normal plus the prealloc header...) So people need to patch our sources which is anything but elegant. https://github.com/rust-bitcoin/rust-secp256k1/blob/master/secp256k1-sys/depend/secp256k1.c.patch

[tcharding]

Would it be acceptable to 'if guard' any code that calls malloc including the stdlib.h includes, or is there a cleaner way to solve this. I had a go but its been 5 years since I wrote C and I got stuck (I do not know the correct syntax and could not set PREALLOC_INTERFACE_ONLY in the makefile).

E.g., in util.h

#if !defined PREALLOC_INTERFACE_ONLY
#include <stdlib.h>
#endif

...

#ifndef PREALLOC_INTERFACE_ONLY
static SECP256K1_INLINE void *checked_malloc(const secp256k1_callback* cb, size_t size) {
    void *ret = malloc(size);
    if (ret == NULL) {
        secp256k1_callback_call(cb, "Out of memory");
    }
    return ret;
}

static SECP256K1_INLINE void *checked_realloc(const secp256k1_callback* cb, void *ptr, size_t size) {
    void *ret = realloc(ptr, size);
    if (ret == NULL) {
        secp256k1_callback_call(cb, "Out of memory");
    }
    return ret;
}
#endif

[real-or-random]

Would it be acceptable to 'if guard' any code that calls malloc including the stdlib.h includes, or is there a cleaner way to solve this.

This is in general the way to go.

But then, when building the library itself, you would then need 'if guard' also functions like secp256k1_context_create(). An issue that arises then is that those functions would still be declared in the normal secp256k1.h header. This is not terrible but when building programs that use the library, the build would fail at link time when they call these functions accidentally. It would be cleaner not to declare these functions at all. Perhaps this can be done with another "user" macro SECP256K1_ONLY_PREALLOC that the user can set before including secp256k1.h but headers that change their behavior based on macros can be nasty.

Maybe a cleaner approach would be to reorganize headers such that most functions are in some header like secp256k1_main.h, which is then included from secp256k1.h (with only malloc functions like secp256k1_context_create()) and secp256k1_preallocated.h (with their prealloc counterparts). Then the user could easily choose between the two interfaces by including either secp256k1.h or secp256k1_preallocated.h.

It would be good to know that @sipa and @jonasnick think about this because reorganizing headers would be a pretty fundamental decision.

(I do not know the correct syntax and could not set PREALLOC_INTERFACE_ONLY in the makefile).
Syntax looks good actually! I think we could take care of the Makefile / build system; I agree that's a large hurdle for people who are not familiar with it (and arguably also for people who are familiar... )

[tcharding]

Great, thanks. I'm happy to work on this under direction but I'm conscious of bumbling around changing things and generally being annoying. Like I said elsewhere I haven't touched C code for 5 years so there will likely be lots of stylistic problems to catch in review.

[jonasnick]

Maybe a cleaner approach would be to reorganize headers such that most functions are in some header like secp256k1_main.h [...]

I really like this idea. On the other hand, I find it difficult to commit to this setup given the uncertain future of the context, the role of malloc and scratch spaces. @real-or-random says:

But then, when building the library itself, you would then need 'if guard' also functions like secp256k1_context_create().

I don't think we have to 'if guard' these functions entirely. We could also just change them to something like:

secp256k1_context* secp256k1_context_create(unsigned int flags) {
#ifdef PREALLOC_INTERFACE_ONLY
    return NULL;
#else
    /* ... */
    checked_malloc(...);
    /* ... */
#endif
}

Less clean for sure than your suggestion, but not terrible imo.

[sipa]

I'm happy with either approach:

• Conditionally stubbing out the body of secp256k1_context_create etc. and have them always return failure when compiled without allocation functions.
• Splitting secp256k1.h into separate parts so it's possible to only include the non-mallocing ones. I think the users who care about non-malloc are probably already looking sufficiently closely at the code that a change like that wouldn't be very burdensome.

[real-or-random]

• Conditionally stubbing out the body of secp256k1_context_create etc. and have them always return failure when compiled without allocation functions.

• Splitting secp256k1.h into separate parts so it's possible to only include the non-mallocing ones. I think the users who care about non-malloc are probably already looking sufficiently closely at the code that a change like that wouldn't be very burdensome.

The conclusion of my prototype for the latter option (#1166) is that we should do the former.