Don't #include standard library headers unconditionally

real-or-random commented at 8:43 pm on December 13, 2023: contributor

This is supposed to resolve #1095 and help with restricted settings such as embedded systems or WASM.

The approach is to detect the availability of <stdio.h> and <stdio.h>, both when building the library or building against the API. The effects depend on the case:

When building the library:

If <stdio.h> is not available, disable fprintf(stderr, ...) in callbacks and emit a note during compilation (#pragma message)
If <stdlib.h> is not available and external default callbacks are not used, then we don’t have abort. Raise an error in compilation and tell the user to use external callbacks
If <stdlib.h> is not available, we don’t have malloc (and free). Disable all API functions that need it.

When building against our API:

If <stdlib.h> is not available, make an educated guess that it wasn’t available when the library was built, and tell the user that certain API functions are not available.

We use different methods of detecting the presence of the headers, see the code. The user can always override our detection mechanism as a last resort.

The other kind of standard library functionality we use is memset and memcpy from <string.h> (but not memcmp because it’s a policy to use `secp256k1_memcmp_var). There’s not much we can do for these two, unless we want to provide our own implementations, but in practice these functions are pretty common. C23 has adopted a proposal to make these mandatory even for freestanding implementations (i.e., embedded systems in standard-speak).

A good way to test this is to change the filename in _has_include to simulate the case when headers are unavailable.

I went through a lot of back and forth when working on this. Here are some alternatives and why I didn’t choose them for now:

Detecting <stdlib.h> for both abort and malloc and is rather crude. I’m sure there are embedded platforms out there which have malloc and no abort, or the other way around. However, dealing with this will probably need more machinery (e.g., special non-standard headers), and compilers don’t have automatic mechanism to detect the availability of functions. Moreover, I don’t want to overshoot in the dark with my limited experience in embedded system and given the fact that noone has complained so far. After this PR, if you have a nonstandard <stdlib.h> with malloc but without abort, you can get away with providing external callbacks. If you have a nonstandard <stdlib.h> with abort but without malloc, you can get away with overriding the detection of <stdlib.h> to “no” while at the same time providing external callbacks (that may still call abort then.) For a different, much more flexible approach, see for example Mbed TLS, which has a focus on embedded system.
I’m not entirely convinced that it’s a good idea to warn the user when building against our API. We could be wrong and headers were available when building the library and just not now. But in that case, the user can still override, so I think it’s better to loud here.
I’m also not sure that it’s a good idea to let the compiler issue a “note” message when we disable features when compiling the library, also taking into account that it’s apparently difficult to disable this kind of messages in GCC. But again, I felt it’s a bit better to be loud here.

@tcharding This is supposed to improve the situation for rust-secp256k1 wasm significantly. Perhaps except for the aforementioned string.h, but if you’re using wasi, then I think you should just use the wabi sysroot and not your own sysroot then. Searching the web also tells me that clang may need some compiler option. I don’t know, I haven’t tried this.

hash: Avoid including stdlib.h 5c55a27cd2

tcharding commented at 11:18 pm on December 13, 2023: contributor

Epic, this is awesome. I can confirm that I was able to build the code on this branch in rust-secp256k1 after using a modified version of the vendor script in rust-secp256k1/secp256k1-sys (that removed all the source file patching). I was also able to remove the printf stuff from build.rs and delete the stdio.h and stdlib.h files from rust-secp256k1/secp256k1-sys/wasm/wasm-sysroot. Massive win, thanks man.

in src/util.h:18 in dd6fd522a5 outdated

16+#include <stddef.h>
17+#include <stdint.h>
18+#if SECP256K1_HAVE_STDIO_H
19+#  include <stdio.h>
20+#else
21+#  pragma message "<stdio.h> not available, disabling debugging output for fatal library errors."

real-or-random commented at 3:51 pm on December 14, 2023:

This one should only be displayed when the default callbacks are active.

(Or I change my mind and just get rid of it entirely… I’m not sure if these notes are too annoying/intrusive if it’s too hard to silence them.)

real-or-random commented at 4:00 pm on December 14, 2023: contributor

@tcharding If I understand correctly, this means that the custom string.h would be the only hack left on the rust-secp256k1 side? Have you tried pointing it to the wasi sysroot?

If that’s not desirable for whatever reason, we could also consider “fixing” the issue here: We could simply avoid including the header and then put the required functions declarations in secp256k1 directly. I just would want to put this behind some #ifdef because I think it shouldn’t be enabled by default.

If you ask me, I think it’s philosophically the user’s responsibility to configure their sysroot correctly, and if they have memcpy, to provide a header that declares it. But pragmatically, a workaround will add 5 lines to our code, so why not…

real-or-random added the label build on Dec 14, 2023

real-or-random added the label refactor/smell on Dec 14, 2023

tcharding commented at 8:31 pm on December 14, 2023: contributor

Have you tried pointing it to the wasi sysroot?

I went to try [0] but the wasm32-wasi target is not is not supported by wasm-pack which we use in CI. I didn’t get any further than that.

[0] (I’m know basically nothing about wasm)

ref: https://github.com/rustwasm/wasm-pack/issues/654

tcharding cross-referenced this on Dec 14, 2023 from issue fails to compile to wasm by ulrichard

in include/secp256k1.h:223 in dd6fd522a5 outdated

219@@ -190,6 +220,17 @@ typedef int (*secp256k1_nonce_function)(
220 # define SECP256K1_DEPRECATED(_msg)
221 #endif
222 
223+/* Attribute for marking functions, types, and variables as deprecated */

jonasnick commented at 8:07 pm on December 15, 2023:

0/* Attribute for marking functions, types, and variables as unavailable */

real-or-random commented at 12:49 pm on December 20, 2023:

fixed

in src/modinv32_impl.h:28 in dd6fd522a5 outdated

23+static int64_t secp256k1_modinv32_abs(int32_t v) {
24+    VERIFY_CHECK(v > INT32_MIN);
25+    if (v < 0) return -v;
26+    return v;
27+}
28+

jonasnick commented at 8:08 pm on December 15, 2023:

This seems currently unused. Shouldn’t we replace the use of labs below with this?

real-or-random commented at 12:49 pm on December 20, 2023:

fixed

jonasnick commented at 8:13 pm on December 15, 2023: contributor

Concept ACK. I played around with it a bit and it works as expected.

Should we test in CI that SECP256K1_HAVE_STDIO_H=0 works and/or that --disable-exhaustive-tests --disable-tests --disable-benchmark --enable-external-default-callbacks CFLAGS=-DSECP256K1_HAVE_STDLIB_H=0 builds?

I’m not entirely convinced that it’s a good idea to warn the user when building against our API. We could be wrong and headers were available when building the library and just not now. But in that case, the user can still override, so I think it’s better to loud here.

Agreed

real-or-random commented at 3:11 pm on December 18, 2023: contributor

Will address all comments.

If you ask me, I think it’s philosophically the user’s responsibility to configure their sysroot correctly, and if they have memcpy, to provide a header that declares it. But pragmatically, a workaround will add 5 lines to our code, so why not…

I said this above, but I think I changed my mind. If we do it, we’ll have to support it basically forever. If you can keep the workaround for that one header, you could remove it once wasm support in Rust is better.

I’m also not sure that it’s a good idea to let the compiler issue a “note” message when we disable features when compiling the library, also taking into account that it’s apparently difficult to disable this kind of messages in GCC. But again, I felt it’s a bit better to be loud here.

We could also add a simple _QUIET macro, but not sure if this is overkill. I lean towards removing the messages. Silent builds are a nice thing, and we don’t want to be annoying.

sipa commented at 3:12 pm on December 18, 2023: contributor

Concept ACK. I agree with @jonasnick’s review comments. Do the warning messages add anything, as there are already deprecated markers on the unavailable functions?

modinv: Avoid including stdlib.h 5e84c28b54

secp256k1.h: Guess availability of standard lib c4e58322b5

Use standard lib functionality only when available f8573bb5b3

Mark API as "unavailable" if headers are missing bc6521c9bf

real-or-random force-pushed on Dec 20, 2023

ci: Add jobs without <stdio.h> or <stdlib.h> b4ec838a35

real-or-random force-pushed on Dec 20, 2023

real-or-random commented at 1:17 pm on December 20, 2023: contributor

Should we test in CI that SECP256K1_HAVE_STDIO_H=0 works

Done.

and/or that --disable-exhaustive-tests --disable-tests --disable-benchmark --enable-external-default-callbacks CFLAGS=-DSECP256K1_HAVE_STDLIB_H=0 builds?

Done, we don’t need to disable the tests and benchmarks, they ignore the external callbacks setting anyway.

I also moved the pragma message for building the library to secp256k1.h. This seems wrong because we could print it in the actual code instead of the public header. But this approach avoids having two separate macros for what the user wants and what we have autodetected. (Only in the preprocessor block in secp256k1.h., we still have three values: undefined, true and false, where undefined means “auto”. After the block, we’ll reduce this to just true and false…) @sipa In contrast do what we said on IRC, what I haven’t done now is to add autotools/CMake options. I’m a bit unsure. So the concept of this PR is to use the preprocessor for detection, and I think that’s the way to go in the long run, also for other config options if possible, because the preprocessor is build system agnostic and works even without a build system.

Of course, we could add --with-stdio=[yes|no|auto] (default: auto) and the like, but I don’t think this adds a lot to the this simple approach of this PR: If you have a modern and not too exotic compiler, the result of our auto detection will just be correct, and there’s really no need to override it. In other words: If the compiler says it doesn’t have stdlib.h for inclusion, then just overriding this and still issuing an #include won’t magically make the compiler work. (If you still have a malloc somewhere, this PR anyway won’t help you.) And even if you have a strange compiler, it could very well happen that the build systems will fail in other ways and you’ll anyway need to invoke the compiler manually. The only reason to override our detection would be to disable functionality that is actually available, but then you’ll probably just want the external callbacks… What do you think?

real-or-random commented at 3:35 pm on December 20, 2023: contributor

and/or that --disable-exhaustive-tests --disable-tests --disable-benchmark --enable-external-default-callbacks CFLAGS=-DSECP256K1_HAVE_STDLIB_H=0 builds?

Done, we don’t need to disable the tests and benchmarks, they ignore the external callbacks setting anyway.

CI disagrees. Never mind.

And this is, in fact, an argument for adding flags like--with-stdlib to build system… (If stdlib is not is available, then don’t build tests, benchmarks etc…)

Marking as draft for now. I’ll need to look at this again with a fresh mind.

real-or-random marked this as a draft on Dec 20, 2023

real-or-random added the label needs-changelog on Dec 21, 2023

Don’t #include standard library headers unconditionally #1461