Don't #include standard library headers unconditionally

real-or-random commented at 2:55 pm on March 28, 2022: contributor

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.

https://github.com/bitcoin-core/secp256k1/blob/912b7ccc4473b5c969b01d027b8d5dc515435eb5/src/util.h#L16

This is a problem downstream in https://github.com/rust-bitcoin/rust-secp256k1/pull/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

real-or-random cross-referenced this on Mar 28, 2022 from issue Fix wasm build by tcharding

real-or-random renamed this:
~~Don't #include <stdio.h> unconditionally~~
Don't #include standard library headers unconditionally
on Mar 28, 2022

tcharding referenced this in commit d185a6fcc0 on Nov 2, 2022

tcharding cross-referenced this on Nov 2, 2022 from issue Conditionally include `stdio.h` and `stdlib.h` by tcharding

tcharding commented at 3:10 am on November 4, 2022: contributor

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

 0#if !defined PREALLOC_INTERFACE_ONLY
 1#include <stdlib.h>
 2#endif
 3
 4...
 5
 6#ifndef PREALLOC_INTERFACE_ONLY
 7static SECP256K1_INLINE void *checked_malloc(const secp256k1_callback* cb, size_t size) {
 8    void *ret = malloc(size);
 9    if (ret == NULL) {
10        secp256k1_callback_call(cb, "Out of memory");
11    }
12    return ret;
13}
14
15static SECP256K1_INLINE void *checked_realloc(const secp256k1_callback* cb, void *ptr, size_t size) {
16    void *ret = realloc(ptr, size);
17    if (ret == NULL) {
18        secp256k1_callback_call(cb, "Out of memory");
19    }
20    return ret;
21}
22#endif

tcharding cross-referenced this on Nov 4, 2022 from issue Remove usage of CHECK from non-test file by tcharding

tcharding referenced this in commit 6dfbbb44c9 on Nov 4, 2022

real-or-random commented at 1:25 pm on November 4, 2022: contributor

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 commented at 7:52 pm on November 4, 2022: contributor

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 commented at 10:34 am on November 8, 2022: contributor

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:

0secp256k1_context* secp256k1_context_create(unsigned int flags) {
1#ifdef PREALLOC_INTERFACE_ONLY
2    return NULL;
3#else
4    /* ... */
5    checked_malloc(...);
6    /* ... */
7#endif
8}

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

real-or-random referenced this in commit 86e3b38a4a on Nov 16, 2022

sipa commented at 4:12 pm on November 22, 2022: contributor

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 cross-referenced this on Dec 1, 2022 from issue PROTOTYPE Split headers to make it possible to use only prealloc API by real-or-random

real-or-random added this to the milestone 0.3.1 (or 0.4.0) on Mar 8, 2023

real-or-random removed this from the milestone 0.3.1 (or 0.4.0) on Mar 8, 2023

real-or-random added this to the milestone stable release (1.0.0-rc.1) on Mar 8, 2023

real-or-random commented at 9:44 am on March 26, 2023: contributor

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.

real-or-random removed this from the milestone stable release (1.0.0-rc.1) on Apr 10, 2023

real-or-random added this to the milestone 0.3.2 (or 0.4.0) on Apr 10, 2023

jonasnick removed this from the milestone 0.3.2 (or 0.4.0) on May 12, 2023

real-or-random added this to the milestone 0.3.3 (or 0.4.0) on Jun 5, 2023

real-or-random added the label bug on Jun 7, 2023

real-or-random removed the label bug on Jul 17, 2023

real-or-random added the label bug on Jul 17, 2023

real-or-random added the label refactor/smell on Jul 17, 2023

real-or-random removed the label bug on Jul 17, 2023

real-or-random removed this from the milestone 0.3.3 (or 0.4.0) on Aug 29, 2023

real-or-random added this to the milestone stable release (1.0.0-rc.1) on Aug 29, 2023

jonasnick removed this from the milestone stable release (1.0.0-rc.1) on Sep 20, 2023

jonasnick added this to the milestone 0.4.1 or 0.5.0 on Sep 20, 2023

Don’t #include standard library headers unconditionally #1095