Don't #include standard library headers unconditionally #1095

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

2. real-or-random renamed this:
   Don't #include <stdio.h> unconditionally
   Don't #include standard library headers unconditionally
   on Mar 28, 2022
3. tcharding referenced this in commit d185a6fcc0 on Nov 2, 2022
4. 
   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

   #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
   

5. tcharding referenced this in commit 6dfbbb44c9 on Nov 4, 2022
6. 
   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... )

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

8. 
   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:

   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.

9. real-or-random referenced this in commit 86e3b38a4a on Nov 16, 2022
10. 
    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.
11. real-or-random added this to the milestone 0.3.1 (or 0.4.0) on Mar 8, 2023
12. real-or-random removed this from the milestone 0.3.1 (or 0.4.0) on Mar 8, 2023
13. real-or-random added this to the milestone stable release (1.0.0-rc.1) on Mar 8, 2023
14. 
    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.

15. real-or-random removed this from the milestone stable release (1.0.0-rc.1) on Apr 10, 2023
16. real-or-random added this to the milestone 0.3.2 (or 0.4.0) on Apr 10, 2023
17. jonasnick removed this from the milestone 0.3.2 (or 0.4.0) on May 12, 2023
18. real-or-random added this to the milestone 0.3.3 (or 0.4.0) on Jun 5, 2023
19. real-or-random added the label bug on Jun 7, 2023
20. real-or-random removed the label bug on Jul 17, 2023
21. real-or-random added the label bug on Jul 17, 2023
22. real-or-random added the label refactor/smell on Jul 17, 2023
23. real-or-random removed the label bug on Jul 17, 2023
24. real-or-random removed this from the milestone 0.3.3 (or 0.4.0) on Aug 29, 2023
25. real-or-random added this to the milestone stable release (1.0.0-rc.1) on Aug 29, 2023
26. jonasnick removed this from the milestone stable release (1.0.0-rc.1) on Sep 20, 2023
27. jonasnick added this to the milestone 0.4.1 or 0.5.0 on Sep 20, 2023

Contributors

real-or-randomtchardingjonasnicksipa

Milestone
0.4.1