Don’t #include standard library headers unconditionally #1095

issue real-or-random openend this issue on March 28, 2022
  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 cross-referenced this on Mar 28, 2022 from issue Fix wasm build by tcharding
  3. real-or-random renamed this:
    Don't #include <stdio.h> unconditionally
    Don't #include standard library headers unconditionally
    on Mar 28, 2022
  4. tcharding referenced this in commit d185a6fcc0 on Nov 2, 2022
  5. tcharding cross-referenced this on Nov 2, 2022 from issue Conditionally include `stdio.h` and `stdlib.h` by tcharding
  6. 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
    
  7. tcharding cross-referenced this on Nov 4, 2022 from issue Remove usage of CHECK from non-test file by tcharding
  8. tcharding referenced this in commit 6dfbbb44c9 on Nov 4, 2022
  9. 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… )

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

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

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


real-or-random tcharding jonasnick sipa

Labels
refactor/smell

Milestone
0.4.1 or 0.5.0


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 03:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me