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

build: Add missing prefix to BoringSSL delete_if stack symbols #245

Merged

Conversation

simonjbeaumont
Copy link
Contributor

@simonjbeaumont simonjbeaumont commented Jul 2, 2024

Motivation

When building projects that depend on both Swift Crypto and JWTKit on Linux, we hit a symbol clash. Both modules vendor in their own copy of BoringSSL and go to lengths to namespace the symbols, but there were some symbols that were not prefixed, which resulted in compilation failures due to clashing macro expansions, e.g.

...
/pwd/.build/checkouts/swift-crypto/Sources/CCryptoBoringSSL/include/CCryptoBoringSSL_x509.h:2964:1: error: 'sk_X509_OBJECT_delete_if' has different definitions in different modules; definition in module 'CCryptoBoringSSL' first difference is function body
DEFINE_STACK_OF(X509_OBJECT)
^
/pwd/.build/checkouts/swift-crypto/Sources/CCryptoBoringSSL/include/CCryptoBoringSSL_stack.h:100:31: note: expanded from macro 'DEFINE_STACK_OF'
#define DEFINE_STACK_OF(type) DEFINE_NAMED_STACK_OF(type, type)
                              ^
/pwd/.build/checkouts/swift-crypto/Sources/CCryptoBoringSSL/include/CCryptoBoringSSL_stack.h:93:3: note: expanded from macro 'DEFINE_NAMED_STACK_OF'
  BORINGSSL_DEFINE_STACK_OF_IMPL(name, type *, const type *) \
  ^
/pwd/.build/checkouts/swift-crypto/Sources/CCryptoBoringSSL/include/CCryptoBoringSSL_stack.h:486:23: note: expanded from macro 'BORINGSSL_DEFINE_STACK_OF_IMPL'
  OPENSSL_INLINE void sk_##name##_delete_if(                                   \
                      ^
<scratch space>:146:1: note: expanded from here
sk_X509_OBJECT_delete_if
^
/pwd/.build/checkouts/jwt-kit/Sources/CJWTKitBoringSSL/include/CJWTKitBoringSSL_x509.h:4332:1: note: but in 'CJWTKitBoringSSL' found a different body
DEFINE_STACK_OF(X509_OBJECT)
^
/pwd/.build/checkouts/jwt-kit/Sources/CJWTKitBoringSSL/include/CJWTKitBoringSSL_stack.h:100:31: note: expanded from macro 'DEFINE_STACK_OF'
#define DEFINE_STACK_OF(type) DEFINE_NAMED_STACK_OF(type, type)
                              ^
/pwd/.build/checkouts/jwt-kit/Sources/CJWTKitBoringSSL/include/CJWTKitBoringSSL_stack.h:93:3: note: expanded from macro 'DEFINE_NAMED_STACK_OF'
  BORINGSSL_DEFINE_STACK_OF_IMPL(name, type *, const type *) \
  ^
/pwd/.build/checkouts/jwt-kit/Sources/CJWTKitBoringSSL/include/CJWTKitBoringSSL_stack.h:486:23: note: expanded from macro 'BORINGSSL_DEFINE_STACK_OF_IMPL'
  OPENSSL_INLINE void sk_##name##_delete_if(                                   \
                      ^
<scratch space>:110:1: note: expanded from here
sk_X509_OBJECT_delete_if
^
<unknown>:0: error: too many errors emitted, stopping now

This appears to happen for the following symbols:

root@674ddc61f160:/pwd# less -R build.log | grep -a error: | cut -d' ' -f2-3 | sort | uniq -c
     26 error: 'sk_ASN1_INTEGER_delete_if'
     26 error: 'sk_ASN1_OBJECT_delete_if'
     26 error: 'sk_ASN1_TYPE_delete_if'
     26 error: 'sk_ASN1_VALUE_delete_if'
     26 error: 'sk_BIO_delete_if'
     26 error: 'sk_CONF_VALUE_delete_if'
     26 error: 'sk_GENERAL_NAME_delete_if'
     26 error: 'sk_OPENSSL_STRING_delete_if'
     26 error: 'sk_X509_ALGOR_delete_if'
     26 error: 'sk_X509_ATTRIBUTE_delete_if'
     26 error: 'sk_X509_CRL_delete_if'
     26 error: 'sk_X509_EXTENSION_delete_if'
     26 error: 'sk_X509_INFO_delete_if'
     26 error: 'sk_X509_NAME_ENTRY_delete_if'
     26 error: 'sk_X509_NAME_delete_if'
     26 error: 'sk_X509_OBJECT_delete_if'
     26 error: 'sk_X509_REVOKED_delete_if'
     26 error: 'sk_X509_delete_if'
     26 error: 'sk_void_delete_if'

Looks like all the *_delete_if functions generated by the many uses of DEFINE_STACK_OF macro were not prefixed.

Modifications

Update the vendor script and rerun to ensure delete_if stack functions are prefixed.

Result

Can depend on both Swift Crypto and JWTKit in the same project.

Notes

Following this fix we should take the time to see if we can statically audit for non-prefixed symbols.

@simonjbeaumont simonjbeaumont marked this pull request as ready for review July 2, 2024 15:09
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contents of this file are automatically generated. We need to understand why the most recent run of the vendoring script didn’t catch these.

@simonjbeaumont
Copy link
Contributor Author

The contents of this file are automatically generated. We need to understand why the most recent run of the vendoring script didn’t catch these.

Because you since approved the PR, I'm assuming you missed the part of the patch that updated the vendor script and are now satisfied that the patch is sound.

@Lukasa
Copy link
Collaborator

Lukasa commented Jul 2, 2024

That assumption is exactly right. I blame the beach being too bright for me to see my phone screen. 🙈

@Lukasa Lukasa enabled auto-merge (squash) July 2, 2024 18:05
@Lukasa Lukasa merged commit c370ad8 into apple:main Jul 2, 2024
8 of 9 checks passed
@glbrntt glbrntt added the semver/patch No public API change. label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants