Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add implicit gcc flag to all feature probes #4074

Merged
merged 4 commits into from
Jun 27, 2023

Conversation

goatgoose
Copy link
Contributor

@goatgoose goatgoose commented Jun 24, 2023

Description of changes:

A recent refactor of our feature probes in #3921 correctly caused the rust bindings to probe for more features. However, due to how some of the feature probes work, building s2n-tls-sys currently fails when linked with some libcryptos.

In CMake, a feature is enabled after successfully compiling the feature test and linking it with the libcrypto. Some feature probes, such as S2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH, rely on the linker failing to disable the feature:

CMakeFiles/cmTC_e26ac.dir/S2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH.c.o: In function `main':
S2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH.c:(.text+0x8): undefined reference to `EVP_md5_sha1'
collect2: error: ld returned 1 exit status

In the rust bindings, the feature probe is compiled but not linked with the libcrypto. This causes the compilation to produce a warning, but the feature probe is still enabled:

[s2n-tls-sys 0.0.32] === Testing feature S2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH ===
[s2n-tls-sys 0.0.32] lib/tests/features/S2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH.c: In function ‘main’:
[s2n-tls-sys 0.0.32] lib/tests/features/S2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH.c:18:5: warning: implicit declaration of function ‘EVP_md5_sha1’; did you mean ‘LN_md5_sha1’? [-Wimplicit-function-declaration]
[s2n-tls-sys 0.0.32]      EVP_md5_sha1();
[s2n-tls-sys 0.0.32]      ^~~~~~~~~~~~
[s2n-tls-sys 0.0.32]      LN_md5_sha1
[s2n-tls-sys 0.0.32] S2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH: true

To fix this, the -Werror=implicit-function-declaration gcc flag was added, causing the compiler to error when a function isn't declared without relying on the linker to fail. Since this kind of check is common with feature probing, this flag was added to a new GLOBAL.flags file which gets applied to all of the feature probes.

Call-outs:

  • Ideally, the rust feature probe implementation would match the c implementation, and link to the libcrypto after compiling each feature test. However, I wasn't sure how to obtain the path to the libcrypto library artifact from openssl-sys to pass to the compiler command, so I left this change out of this PR. It seems like we're able to get the header path from openssl-sys with DEP_OPENSSL_INCLUDE, but I couldn't find an equivalent environment variable for the library directory.

Testing:

This change is tested with a new OpenSSL 1.0.2 generate job added in this PR. OpenSSL 1.0.2 does not define EVP_md5_sha1, so the S2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH feature should not be enabled. Without this change, S2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH is incorrectly enabled and this CI job fails:

warning: lib/crypto/s2n_hash.c:60:20: warning: implicit declaration of function ‘EVP_md5_sha1’; did you mean ‘NID_md5_sha1’? [-Wimplicit-function-declaration]
warning:    60 |             return EVP_md5_sha1();
warning:       |                    ^~~~~~~~~~~~
warning:       |                    NID_md5_sha1
warning: lib/crypto/s2n_hash.c:60:20: warning: returning ‘int’ from a function with return type ‘const EVP_MD *’ {aka ‘const struct env_md_st *’} makes pointer from integer without a cast [-Wint-conversion]
warning:    60 |             return EVP_md5_sha1();
warning:       |                    ^~~~~~~~~~~~~~
   Compiling s2n-tls v0.0.32 (/home/runner/work/s2n-tls/s2n-tls/bindings/rust/s2n-tls)
error: linking with `cc` failed: exit status: 1

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Jun 24, 2023
@goatgoose goatgoose force-pushed the global-feature-flags branch 2 times, most recently from 9834235 to d2e3291 Compare June 27, 2023 14:25
@goatgoose goatgoose force-pushed the global-feature-flags branch 4 times, most recently from 8f930a5 to ac2931e Compare June 27, 2023 15:43
@goatgoose goatgoose marked this pull request as ready for review June 27, 2023 16:20
@goatgoose goatgoose requested review from camshaft and lrstewart June 27, 2023 16:20
@goatgoose goatgoose force-pushed the global-feature-flags branch from ac2931e to 244448e Compare June 27, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants