windows: tidy up wincrypt.h / BoringSSL/AWS-LC coexist workaround#20567
Closed
vszakats wants to merge 16 commits intocurl:masterfrom
Closed
windows: tidy up wincrypt.h / BoringSSL/AWS-LC coexist workaround#20567vszakats wants to merge 16 commits intocurl:masterfrom
wincrypt.h / BoringSSL/AWS-LC coexist workaround#20567vszakats wants to merge 16 commits intocurl:masterfrom
Conversation
This reverts commit d1b5072.
wincrypt.h vs. BoringSSL/AWS-LC workaroundwincrypt.h vs. BoringSSL/AWS-LC workarounds
wincrypt.h vs. BoringSSL/AWS-LC workaroundswincrypt.h / BoringSSL/AWS-LC coexist workarounds
wincrypt.h / BoringSSL/AWS-LC coexist workaroundswincrypt.h / BoringSSL/AWS-LC coexist workaround
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
openssl: move and expand explanatory comment.
openssl: drop duplicate workaround.
schannel: drop workaround. Unnecessary, because OpenSSL headers are
not included in or after schannel code.
schannel: drop explicit
wincrypt.hinclude. It's indirectlyincluded by system
<schannel.h>.ldap: drop explicit
wincrypt.hinclude.It isn't used there, and also not required for the workaround.
winldap.hkeeps including it indirectly.Tested with BoringSSL and AWS-LC (MultiSSL with Schannel), also LDAP
enabled, and H3, unity and non-unity, and all tested cases build fine.
In lib in general, the point is to have the
#undefs between the firstwincrypt.hinclude [1] and the first OpenSSL include [2], within asingle compilation unit. For non-unity builds the only such source is
openssl.c. For unity ones, depending on batch size, in theory the#undefshould be used after eachwincrypt.hinclude. In practicethis is overkill and most cases are covered by
#undef-fing first invtls/openssl.c, and#undefinldap.c. It's not impossible thatmore undefs need to be added after more
wincrypt.hincludes to coverso far undiscovered build cases [3]. Though I could not find more with
the current sources and source order.
It's also an option to include OpenSSL first, then
wincrypt.h, asdone in libtests, but for lib and
vtls/openssl.cit's more practicalto do the opposite.
[1] can be indirect, e.g. via
iphlpapi.h,schannel.h,winldap.h.[2] in
openssl/base.h).Original fix removed by BoringSSL in year
2014.
openssl/ssl.h,openssl/x509v3.h, and some more affected,and including
openssl/ossl_typ.hdoes the#undefautomatically.Since 3.1.0+
#undefis done on each inclusion, in 3.0.x (and earlier) only onthe first inclusion. Initially fixed in
0.9.6d
not affected, though to suppress another warning 3.8.2+ and
a define
is necessary.
[3] The order of unity sources are defined by
lib/Makefile.inc.For libtests, the case is simpler: There is always one compilation unit,
with a fixed order, and at the moment
cli_hx_download.cis includingOpenSSL first, then wincrypt, and in this order they don't bother each
other. Also, at the moment
lib758.cis the only other OpenSSL headeruser, but it's compiled after
cli_hx_download.cso the include isskipped there. This needs to be revisited if either header gets included
before it.
All this said it'd be nice if BoringSSL/AWS-LC restored the built-in
workaround to behave like LibreSSL and OpenSSL and not require local
workarounds like these.
Ref: #20556 (comment)
Follow-up to 4c46c82 #9110
Follow-up to fbe07c6 #5669 #5857