Copilot commented on code in PR #12943:
URL: https://github.com/apache/trafficserver/pull/12943#discussion_r2892142983
##########
CMakeLists.txt:
##########
@@ -593,6 +593,19 @@ check_cxx_source_compiles(
}"
HAVE_CRYPTO_EX_DUP_TYPE1
)
+check_cxx_source_compiles(
+ "#include <openssl/asn1.h>
+ namespace bssl {
+ DECLARE_ASN1_ITEM(GENERAL_NAME)
+ };
+ int main() {
+ if (&bssl::GENERAL_NAME_it == reinterpret_cast<void *>(0x01)) {
+ return 1;
+ }
+ return 0;
+ }"
+ HAVE_GENERAL_NAME_IN_BSSL_NAMESPACE
+)
Review Comment:
The probe currently declares `GENERAL_NAME_it` inside `namespace bssl`
itself, so it doesn’t actually detect whether the upstream headers place
`GENERAL_NAME`/`GENERAL_NAME_it` in the `bssl` namespace—this will tend to
compile regardless of the real upstream layout (and can yield a misleading
`HAVE_GENERAL_NAME_IN_BSSL_NAMESPACE=1`). Instead, the check should *reference*
`bssl::GENERAL_NAME` (or `bssl::GENERAL_NAME_it`) without declaring it, and
include the header that actually declares `GENERAL_NAME` (commonly
`<openssl/x509v3.h>` in OpenSSL/BoringSSL-derived stacks). If you need to avoid
link-time dependency in the probe, prefer an unevaluated context (e.g.,
`decltype(bssl::GENERAL_NAME_it)` or `sizeof(bssl::GENERAL_NAME)`), rather than
taking the symbol address in a way that may require the library definition to
be linked.
##########
src/iocore/net/OCSPStapling.cc:
##########
@@ -97,7 +103,11 @@ using TS_OCSP_REQINFO = struct ocsp_req_info_st {
DECLARE_ASN1_FUNCTIONS(TS_OCSP_REQINFO)
ASN1_SEQUENCE(TS_OCSP_REQINFO) = {
ASN1_EXP_OPT(TS_OCSP_REQINFO, version, ASN1_INTEGER, 0),
+#if HAVE_GENERAL_NAME_IN_BSSL_NAMESPACE
+ ASN1_EXP_OPT(TS_OCSP_REQINFO, requestorName, bssl::GENERAL_NAME, 1),
+#else
ASN1_EXP_OPT(TS_OCSP_REQINFO, requestorName, GENERAL_NAME, 1),
+#endif
Review Comment:
Passing a qualified name like `bssl::GENERAL_NAME` into OpenSSL-style
`ASN1_*` macros can be fragile because many of these macros historically use
token-pasting / identifier-based tricks that assume the “type” argument is a
single identifier token. A more robust pattern is to introduce an unqualified
local alias (e.g., `using GENERAL_NAME_COMPAT = bssl::GENERAL_NAME;`) and pass
that alias into the ASN.1 macros. This keeps the preprocessor surface stable
while still selecting the correct underlying type.
```suggestion
#if HAVE_GENERAL_NAME_IN_BSSL_NAMESPACE
using GENERAL_NAME_COMPAT = bssl::GENERAL_NAME;
#else
using GENERAL_NAME_COMPAT = GENERAL_NAME;
#endif
DECLARE_ASN1_FUNCTIONS(TS_OCSP_REQINFO)
ASN1_SEQUENCE(TS_OCSP_REQINFO) = {
ASN1_EXP_OPT(TS_OCSP_REQINFO, version, ASN1_INTEGER, 0),
ASN1_EXP_OPT(TS_OCSP_REQINFO, requestorName, GENERAL_NAME_COMPAT, 1),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
