Enhance TLS testing with SNI, mTLS, certificate chain, and cancellation tests - part 1 #67
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR implements TLS Server Name Indication (SNI) callback support with servername verification, adds certificate chain handling, threads std::stop_token through I/O paths for cancellation in epoll/IOCP and TLS streams, enhances CI with vcpkg integration, and expands test coverage for WolfSSL. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TLS Stream
participant Context
participant SNI Callback
participant Certificate Manager
participant Socket
Client->>TLS Stream: Initiate TLS handshake
TLS Stream->>Socket: Begin handshake
Socket->>TLS Stream: Receive ClientHello with SNI
TLS Stream->>Context: Query SNI handler available?
alt SNI Callback Configured
TLS Stream->>SNI Callback: Invoke with server name
SNI Callback-->>SNI Callback: Verify server name
alt Callback Accepts
SNI Callback-->>TLS Stream: Return true (accepted)
TLS Stream->>Certificate Manager: Select cert for name
Certificate Manager->>Socket: Load matching certificate
Socket->>TLS Stream: Certificate ready
else Callback Rejects
SNI Callback-->>TLS Stream: Return false (rejected)
TLS Stream->>Socket: Abort handshake
Socket-->>Client: TLS error
end
else No SNI Callback
TLS Stream->>Certificate Manager: Use default certificate
end
Socket->>Client: ServerHello + selected certificate
Client->>Socket: Complete handshake
Socket-->>TLS Stream: Handshake complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
An automated preview of the documentation is available at https://67.corosio.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-01-24 14:03:04 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/wolfssl/src/wolfssl_stream.cpp (1)
794-885: Use WolfSSL error constant for zero-return.
SSL_ERROR_ZERO_RETURNis the OpenSSL constant; WolfSSL usesWOLFSSL_ERROR_ZERO_RETURN. The same file correctly usesWOLFSSL_ERROR_ZERO_RETURNelsewhere. Using the wrong constant will cause build failures in non-OpenSSL-compat WolfSSL configurations.Fix
- else if(err == WOLFSSL_ERROR_SYSCALL || err == SSL_ERROR_ZERO_RETURN) + else if(err == WOLFSSL_ERROR_SYSCALL || err == WOLFSSL_ERROR_ZERO_RETURN)src/openssl/src/openssl_stream.cpp (1)
87-143: Guard SNI ex_data index initialization for thread safety.
sni_ctx_data_indexis written without synchronization. When multiple threads create contexts concurrently, they can each callSSL_CTX_get_ex_new_index()and receive different index values. This causes data stored at one index to be retrieved from another, resulting in silent data corruption in the SNI callback. Usestd::call_onceto ensure exactly one thread initializes the index.🐛 Proposed fix
+#include <mutex> @@ static int sni_ctx_data_index = -1; +static std::once_flag sni_ctx_data_once; @@ - if( sni_ctx_data_index < 0 ) - sni_ctx_data_index = SSL_CTX_get_ex_new_index( 0, nullptr, nullptr, nullptr, nullptr ); - - // Store context_data pointer for SNI callback access - SSL_CTX_set_ex_data( ctx_, sni_ctx_data_index, const_cast<context_data*>( &cd ) ); - - // Set SNI callback if provided - if( cd.servername_callback ) - SSL_CTX_set_tlsext_servername_callback( ctx_, sni_callback ); + std::call_once( sni_ctx_data_once, []{ + sni_ctx_data_index = + SSL_CTX_get_ex_new_index( 0, nullptr, nullptr, nullptr, nullptr ); + }); + if( sni_ctx_data_index >= 0 ) + { + // Store context_data pointer for SNI callback access + SSL_CTX_set_ex_data( ctx_, sni_ctx_data_index, const_cast<context_data*>( &cd ) ); + + // Set SNI callback if provided + if( cd.servername_callback ) + SSL_CTX_set_tlsext_servername_callback( ctx_, sni_callback ); + }
🤖 Fix all issues with AI agents
In `@src/wolfssl/src/wolfssl_stream.cpp`:
- Around line 110-127: The SNI callback wolfssl_sni_callback uses an undefined
identifier fatal_return and bare 0s; replace those return values with the
WolfSSL/OpenSSL-compatible constants: return SSL_TLSEXT_ERR_OK where the
function currently returns 0 (e.g., when no SNI or when accepting) and return
SSL_TLSEXT_ERR_ALERT_FATAL where it currently returns fatal_return (when
cd->servername_callback rejects the hostname). Keep the rest of the logic
(calling wolfSSL_get_servername and invoking context_data::servername_callback)
unchanged.
In `@test/unit/tls/test_utils.hpp`:
- Around line 145-172: The file contains embedded PEM private key literals in
test_utils.hpp; mark each embedded key as test-only by adding a clear
annotation/comment like "TEST KEY - do not use in production" adjacent to each
PEM literal (the PEM string blocks) or move the PEM contents into dedicated
testdata files and add allowlisting comments; ensure any constants or variables
that reference these literals (the PEM string blocks in test_utils.hpp) are
updated to indicate they are test keys only and include a note for secret
scanners and CI allowlist entries.
| // SNI callback invoked by WolfSSL during handshake (server-side) | ||
| // Returns SNICbReturn enum: 0 = OK, fatal_return (2) = abort | ||
| static int | ||
| wolfssl_sni_callback( WOLFSSL* ssl, int* /* alert */, void* arg ) | ||
| { | ||
| char const* servername = wolfSSL_get_servername( ssl, WOLFSSL_SNI_HOST_NAME ); | ||
| if( !servername ) | ||
| return 0; // No SNI sent, continue | ||
|
|
||
| auto* cd = static_cast<context_data const*>( arg ); | ||
| if( cd && cd->servername_callback ) | ||
| { | ||
| if( !cd->servername_callback( servername ) ) | ||
| return fatal_return; // Callback rejected hostname | ||
| } | ||
|
|
||
| return 0; // Accept | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the definition of fatal_return in the codebase
rg "fatal_return\s*=" --type cpp --type h -B 2 -A 2Repository: cppalliance/corosio
Length of output: 45
🌐 Web query:
WolfSSL SNI callback return codes documentation WOLFSSL_SNI_HOST_NAME servername callback
💡 Result:
wolfSSL’s public manuals document SNI enable/usage (e.g., WOLFSSL_SNI_HOST_NAME and wolfSSL_CTX_UseSNI() / wolfSSL_UseSNI()), but they do not document the return-code contract of the SNI “servername received” callback itself. [1]
What is publicly documented/visible:
- The server-side callback type is declared as:
typedef int (*CallbackSniRecv)(WOLFSSL *ssl, int *ret, void* exArg);[2] - You register it with
wolfSSL_CTX_set_servername_callback(ctx, cb)(void) or the OpenSSL-compat functionwolfSSL_CTX_set_tlsext_servername_callback(ctx, cb)(returnsWOLFSSL_SUCCESS/WOLFSSL_FAILUREfor the registration call, not the callback execution). [3]
Because wolfSSL doesn’t document what the callback’s int return value (and the int *ret out-parameter) are expected to mean, the authoritative reference is the wolfSSL source code path that invokes ctx->sniRecvCb during ClientHello/SNI processing (or an official example that uses it). The wolfSSL forum thread linked above explicitly notes the callback exists but also highlights the lack of manual documentation for it. [3]
Sources:
[1] wolfSSL manual: SNI APIs and WOLFSSL_SNI_HOST_NAME (ssl.h docs) (wolfssl.com)
[2] Header declaration of CallbackSniRecv (fossies.org)
[3] wolfSSL forum discussion showing the two setter functions and return behavior of the _tlsext_ setter (wolfssl.com)
Citations:
- 1: https://www.wolfssl.com/documentation/manuals/wolfssl/ssl_8h.html?utm_source=openai
- 2: https://fossies.org/linux/wolfssl/wolfssl/ssl.h?utm_source=openai
- 3: https://www.wolfssl.com/forums/topic1806-wolfsslctx-setservernamecalback-virtual-hosting-revisited.html?utm_source=openai
🏁 Script executed:
# Broader search for fatal_return without line restrictions
rg "fatal_return" --type cpp --type h -B 1 -A 1
# Also search for where it might be defined (maybe as `#define`)
rg "define.*fatal_return|fatal_return.*=" --type cpp --type h --type cRepository: cppalliance/corosio
Length of output: 634
🏁 Script executed:
# Check the beginning of the file to see what's imported and if fatal_return is defined there
head -50 src/wolfssl/src/wolfssl_stream.cppRepository: cppalliance/corosio
Length of output: 1758
🏁 Script executed:
# Search the entire wolfssl_stream.cpp file for fatal_return definition
cat -n src/wolfssl/src/wolfssl_stream.cpp | grep -i "fatal_return" -B 5 -A 5
# Also get line count to see if we should check more
wc -l src/wolfssl/src/wolfssl_stream.cppRepository: cppalliance/corosio
Length of output: 863
🏁 Script executed:
# Search for any mention in the entire repo
rg "fatal_return" --type cpp --type h
# Also check WolfSSL headers on system or in includes
find . -name "*.h" -type f -path "*/wolfssl/*" | head -20Repository: cppalliance/corosio
Length of output: 258
🏁 Script executed:
# Search for any enum or constant definitions in WolfSSL that might match SNI callback returns
rg "SNICbReturn|fatal_return|WOLFSSL.*RETURN" --type h
# Check if there's a WolfSSL constant we should use instead
rg "define.*SNI|SNI.*define" --type h
# Look for any error/return codes related to callbacks
rg "CallbackSniRecv|sniRecvCb" --type hRepository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Try to compile the file to see if fatal_return causes an error
# First, check if there's a build system
ls -la | grep -E "CMakeLists|Makefile|build"
# Check if fatal_return is defined as a constexpr or static const anywhere
rg "constexpr|static const" src/wolfssl/src/wolfssl_stream.cpp | head -20Repository: cppalliance/corosio
Length of output: 273
🏁 Script executed:
# Search in the entire repo for where fatal_return might be defined
rg "fatal_return" -i
# Check the WolfSSL source or headers for SNI callback return constants
# Try searching for what fatal_return should be
rg "FATAL.*RETURN|return.*fatal|2.*abort" --type h --type cpp
# Look at the file around line 110-130 more carefully
sed -n '100,150p' src/wolfssl/src/wolfssl_stream.cppRepository: cppalliance/corosio
Length of output: 1981
🏁 Script executed:
# Search for SNI callback implementations or examples in the codebase
rg "wolfssl_sni_callback|sniRecvCb|servername_callback" --type cpp --type h -B 5 -A 5Repository: cppalliance/corosio
Length of output: 11283
🌐 Web query:
WolfSSL SNI callback fatal return error code constant SSL_ALERT_FATAL
💡 Result:
In wolfSSL’s OpenSSL-compat SNI callback (wolfSSL_CTX_set_tlsext_servername_callback()), the “fatal” callback return value is:
SSL_TLSEXT_ERR_ALERT_FATAL(notSSL_ALERT_FATAL). (docs.openssl.org)
SSL_ALERT_FATAL refers to the TLS Alert level (“fatal” vs “warning”) in the protocol, not the return code for the SNI callback. In OpenSSL-compat SNI handling, when you return SSL_TLSEXT_ERR_ALERT_FATAL, you also set *al to the alert description (e.g. SSL_AD_UNRECOGNIZED_NAME, which OpenSSL uses as the default). (docs.openssl.org)
wolfSSL provides the OpenSSL-style API entry point wolfSSL_CTX_set_tlsext_servername_callback() (and also a non-_tlsext_ named variant). (wolfssl.com)
Citations:
- 1: https://docs.openssl.org/3.6/man3/SSL_CTX_set_tlsext_servername_callback/?utm_source=openai
- 2: https://docs.openssl.org/3.6/man3/SSL_CTX_set_tlsext_servername_callback/?utm_source=openai
- 3: https://www.wolfssl.com/forums/topic1806-wolfsslctx-setservernamecalback-virtual-hosting-revisited.html?utm_source=openai
🏁 Script executed:
# Check what constants WolfSSL provides in its headers
# Look for any SNI or error-related enums in included headers
rg "enum|define" src/wolfssl/src/wolfssl_stream.cpp | head -20
# Also check if fatal_return might be from a macro or typedef
grep -r "fatal_return\|SNICbReturn" . 2>/dev/null | head -20Repository: cppalliance/corosio
Length of output: 324
🏁 Script executed:
# Search for SNI-related constants in WolfSSL or what constants are imported
rg "SNI_CBIO|WOLFSSL.*ERROR|define.*0|define.*2" src/wolfssl/src/wolfssl_stream.cpp | head -30
# Look at the actual callback registration to understand which API is being used
sed -n '180,220p' src/wolfssl/src/wolfssl_stream.cpp
# Check if there's a define or constexpr for fatal_return that we might have missed
grep -n "fatal_return\|FATAL\|ERROR" src/wolfssl/src/wolfssl_stream.cpp | head -40Repository: cppalliance/corosio
Length of output: 3166
🌐 Web query:
WolfSSL native SNI callback return codes wolfSSL_CTX_set_servername_callback vs tlsext
💡 Result:
Setter APIs (wolfSSL_CTX_set_servername_callback vs _tlsext_)
wolfSSL provides two setters that both install the same SNI “receive” callback (ctx->sniRecvCb), but differ in their own return type:
-
void wolfSSL_CTX_set_servername_callback(WOLFSSL_CTX* ctx, CallbackSniRecv cb);
Returns nothing (native/wolfSSL-style). It just setsctx->sniRecvCbifctx != NULL. [1] -
int wolfSSL_CTX_set_tlsext_servername_callback(WOLFSSL_CTX* ctx, CallbackSniRecv cb);
ReturnsWOLFSSL_SUCCESSifctx != NULL, otherwiseWOLFSSL_FAILURE. [1]
Callback return codes (what your CallbackSniRecv should return)
For the SNI callback itself, wolfSSL follows the OpenSSL-style “tlsext” return codes:
SSL_TLSEXT_ERR_OK(defined as0) — accept/continue. [2]SSL_TLSEXT_ERR_ALERT_FATAL(defined asalert_fatal) — abort handshake with a fatal alert. [2]SSL_TLSEXT_ERR_NOACK(defined asalert_warning) — don’t ACK / warning-style handling. [2]
Mapping to OpenSSL API (tlsext_*)
In wolfSSL’s OpenSSL compatibility header, SSL_CTX_set_tlsext_servername_callback is mapped onto wolfSSL’s servername callback setter (the exact mapping can vary by build/config, but it maps to a wolfSSL *_set_*servername_callback entry point). [2]
Sources: [1] [2]
🏁 Script executed:
# Search for SSL_TLSEXT_ERR_ALERT_FATAL or alert_fatal definitions in WolfSSL headers
rg "SSL_TLSEXT_ERR|alert_fatal|ALERT_FATAL" --type h
# Also check what's imported in the file
head -30 src/wolfssl/src/wolfssl_stream.cpp | tail -20Repository: cppalliance/corosio
Length of output: 547
🌐 Web query:
WolfSSL SSL_TLSEXT_ERR_ALERT_FATAL constant value definition alert_fatal
💡 Result:
In wolfSSL’s OpenSSL-compat header, SSL_TLSEXT_ERR_ALERT_FATAL is defined to alert_fatal (i.e., it’s not a separate numeric literal) [1]. In wolfSSL core ssl.h, alert_fatal is an enum AlertLevel value defined as 2 (with alert_warning = 1) [2].
So, SSL_TLSEXT_ERR_ALERT_FATAL ultimately evaluates to 2 in wolfSSL. [1][2]
Fix undefined identifier fatal_return; use proper WolfSSL constants.
The callback uses undefined identifier fatal_return which will not compile. Replace with the documented WolfSSL constant SSL_TLSEXT_ERR_ALERT_FATAL (value 2, already available from the included <wolfssl/ssl.h>). Also replace bare 0 with SSL_TLSEXT_ERR_OK for consistency with the OpenSSL-compat SNI callback contract.
Recommended fix
// SNI callback invoked by WolfSSL during handshake (server-side)
// Returns SSL_TLSEXT_ERR_OK to accept, SSL_TLSEXT_ERR_ALERT_FATAL to reject
static int
wolfssl_sni_callback( WOLFSSL* ssl, int* /* alert */, void* arg )
{
char const* servername = wolfSSL_get_servername( ssl, WOLFSSL_SNI_HOST_NAME );
if( !servername )
return SSL_TLSEXT_ERR_OK; // No SNI sent, continue
auto* cd = static_cast<context_data const*>( arg );
if( cd && cd->servername_callback )
{
if( !cd->servername_callback( servername ) )
return SSL_TLSEXT_ERR_ALERT_FATAL; // Callback rejected hostname
}
return SSL_TLSEXT_ERR_OK; // Accept
}🤖 Prompt for AI Agents
In `@src/wolfssl/src/wolfssl_stream.cpp` around lines 110 - 127, The SNI callback
wolfssl_sni_callback uses an undefined identifier fatal_return and bare 0s;
replace those return values with the WolfSSL/OpenSSL-compatible constants:
return SSL_TLSEXT_ERR_OK where the function currently returns 0 (e.g., when no
SNI or when accepting) and return SSL_TLSEXT_ERR_ALERT_FATAL where it currently
returns fatal_return (when cd->servername_callback rejects the hostname). Keep
the rest of the logic (calling wolfSSL_get_servername and invoking
context_data::servername_callback) unchanged.
test/unit/tls/test_utils.hpp
Outdated
| "-----BEGIN PRIVATE KEY-----\n" | ||
| "MIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQC3ldPp/pn9pt8s\n" | ||
| "j4vQxgPlyzOEFr7eHl12VfKkh6sLDGIpHWCeNZp91ab9muIwvTvejNMCGAXRtCDV\n" | ||
| "U9E0N5K/IBRIlCnl08uOrUrgXAUEoC6jkZhZ48Tja/KwCU5onvRGvQXYt6uTx74U\n" | ||
| "kkoZL0A9ayLgV5B9IlzXN84MKJ5K548G2OZtkk972Aaty/x3gqNCjcLSEDaopkhB\n" | ||
| "OSvRseapS26n2bvRblU0l1aAZ02mKsOKAIP/HghmYTM/eYyc/SCnglGwES4ZeLmf\n" | ||
| "N6NynCtQdkv9WagwC9g6wgdTx7A7G4p+VoKrmsmMXW6T+Lb4Kg+IwZ5tJ6QnQnw8\n" | ||
| "AhyoJWujAgMBAAECggEAMVH0pQPrzduzUC7eQn+4E1eUZvOPYm/o7v4nGjGCb4zr\n" | ||
| "oB0O1GIVN6Ia4z3lb2+fMmpF0+WtRomsWnNSnEMjzuno2RjI6sAMCzAeEglWpcf8\n" | ||
| "z5+xPND2l5xsDgPqByxQ9uIYPIEXfLOoKrGka4Cosvdh3sBXhm6hX4ZT+is9X2TC\n" | ||
| "kyoW906lMYXPFX5M9zb+GuGl3HuOXeLbZijwJ1tTMUZnk1fZyWEJt9kms4Fh7yS6\n" | ||
| "CNYzjKNK5LSvqjKMlcirj0x+X3GI4oJ+KWCeUxoUMtokSpHVVVFry/noEa7o1yOr\n" | ||
| "zCYWZQWeIJ5I2RrC3AFTMQATSg2s/DvjHPHazJ7UzQKBgQDhaamCOnPjk3vJBRNh\n" | ||
| "lt8/47rBOLD/Ua/Hh4iKgZ8MNJz6lHBTSd+ESZsSg9PNUCk8wmY8+LLV0CpRI+hF\n" | ||
| "0VDckyjmr1TqVBoc2GBpjPE6skUod/xBZOdQ4Upm2rF8E+JDMbuB8brcCJFCQYLM\n" | ||
| "GG6llHDHIczOgvp2yujCMxWJNQKBgQDQfywH3yQVuePiPbyiGK8ARFuMdHwlVwSP\n" | ||
| "FzivNXVVJp1E6zHoLHAOHIwUsVZYunflDKZriZ3AxjeiSSIMTaLAcPgGp8fP4sdX\n" | ||
| "lvENvjM4QggtYEVyuo5XrmovEtV6at8O5p984dwaAQoznZZv2K9Kt6/gx6a3+zQt\n" | ||
| "H8bdKJCUdwKBgD7KgD2WqtGqM8E7eLqmnGnfthY9BJEa4CxkxNRQZ02vGktzLhcF\n" | ||
| "bQ4csuXlcwquWc5jGLfDT43f/um7ZuiL9kp7c9lO3giohN2kKLc+W7ROFJXBVrOg\n" | ||
| "uA7/swoTwX0ezNiK8gCwpazFdjFOrnDMHYZiY0gVUkf0lHCi9VOjh0xBAoGAEW5A\n" | ||
| "WRwfoS1cTuLIbWjQ4J3WZYSriFehCvFvDL7UY10KEuPy1S055QQf9e7pgBt+wIhx\n" | ||
| "NVZY+O/ZYNjqXsryy1Hmem/2dXvJHJqC5po7H/3tPxXoWHIeSlhLiknxzP04Tr+b\n" | ||
| "H86mHwptNul61TjxVrbKnmkyl/kJYKhicMTeaXsCgYB+wNCxuQ4MIzErm7CXnKCp\n" | ||
| "xQoFFzR0Fhay5x86Ry9hxBYCeio2CSByV+pFX0AOvvJ7hhm3iSD5p91ulIgl3YfL\n" | ||
| "23Ot+Yles5ZYawVJ3cqeFGiG7vPi2KU9EztdnRlmJwF7P7m4XzzcNvvbK/FbOQT5\n" | ||
| "E7D5rHt+zVEyi3BDrCSTZw==\n" | ||
| "-----END PRIVATE KEY-----\n"; |
There was a problem hiding this comment.
Mark embedded private keys as test-only artifacts.
These PEM keys are valid but will trigger secret scanners; please add explicit “test key” annotations or move to testdata files with allowlisting to avoid noise or accidental reuse.
Also applies to: 198-225, 251-278, 304-331, 357-384, 410-437
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 145-172: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🤖 Prompt for AI Agents
In `@test/unit/tls/test_utils.hpp` around lines 145 - 172, The file contains
embedded PEM private key literals in test_utils.hpp; mark each embedded key as
test-only by adding a clear annotation/comment like "TEST KEY - do not use in
production" adjacent to each PEM literal (the PEM string blocks) or move the PEM
contents into dedicated testdata files and add allowlisting comments; ensure any
constants or variables that reference these literals (the PEM string blocks in
test_utils.hpp) are updated to indicate they are test keys only and include a
note for secret scanners and CI allowlist entries.
MungoG
force-pushed
the
tls-testing-fixes
branch
from
d074046 to
6064806
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wolfssl/src/wolfssl_stream.cpp (1)
158-175: Verify certificate-chain buffer format expectations.
wolfSSL_CTX_use_certificate_chain_bufferaccepts PEM-encoded buffers only; it has no format parameter. Ifcertificate_chaincan contain DER/ASN.1 data, this will fail silently. Either enforce thatcertificate_chainmust be PEM-encoded, or add explicit format validation before calling this function. Note: no format-aware variant of this function exists in wolfSSL (format support is only available for single-certificate and CA-chain verification APIs).
🤖 Fix all issues with AI agents
In `@src/wolfssl/src/wolfssl_stream.cpp`:
- Around line 1031-1041: The SNI and hostname-verification calls may fail
silently; after calling wolfSSL_UseNI (wolfSSL_UseSNI) and
wolfSSL_check_domain_name in the wolfssl_stream client branch (when type ==
wolfssl_stream::client and impl.hostname not empty), check their return values
and treat non-success as an initialization failure: if wolfSSL_UseSNI does not
return WOLFSSL_SUCCESS or wolfSSL_check_domain_name does not return SSL_SUCCESS,
log the error and return/propagate the same failure path used elsewhere in this
function (match the existing error-handling pattern for ssl_ setup), ensuring
the session is aborted rather than allowed to continue without SNI/hostname
verification.
♻️ Duplicate comments (1)
src/wolfssl/src/wolfssl_stream.cpp (1)
111-128: Fix undefined SNI callback return value.
fatal_returnis undefined and will not compile; also return codes should use the SNI callback constants for clarity.🐛 Proposed fix
- if( !servername ) - return 0; // No SNI sent, continue + if( !servername ) + return SSL_TLSEXT_ERR_OK; // No SNI sent, continue @@ - if( !cd->servername_callback( servername ) ) - return fatal_return; // Callback rejected hostname + if( !cd->servername_callback( servername ) ) + return SSL_TLSEXT_ERR_ALERT_FATAL; // Callback rejected hostname @@ - return 0; // Accept + return SSL_TLSEXT_ERR_OK; // AcceptwolfSSL SNI callback return codes (SSL_TLSEXT_ERR_OK / SSL_TLSEXT_ERR_ALERT_FATAL) and behavior of wolfSSL_CTX_set_servername_callback
| // Apply per-session config (SNI + hostname verification) from context | ||
| if( type == wolfssl_stream::client && !impl.hostname.empty() ) | ||
| { | ||
| // Set SNI extension so server knows which cert to present | ||
| wolfSSL_UseSNI( ssl_, WOLFSSL_SNI_HOST_NAME, | ||
| impl.hostname.data(), | ||
| static_cast<unsigned short>( impl.hostname.size() ) ); | ||
|
|
||
| // Enable hostname verification (checks CN/SAN in peer cert) | ||
| wolfSSL_check_domain_name( ssl_, impl.hostname.c_str() ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual code around the flagged lines
cat -n src/wolfssl/src/wolfssl_stream.cpp | sed -n '1020,1050p'Repository: cppalliance/corosio
Length of output: 1388
🏁 Script executed:
# Also get more context about the function this code is in
cat -n src/wolfssl/src/wolfssl_stream.cpp | sed -n '1000,1050p'Repository: cppalliance/corosio
Length of output: 2143
🏁 Script executed:
# Search for the function that contains these lines
rg -B 30 "wolfSSL_UseSNI.*WOLFSSL_SNI_HOST_NAME" src/wolfssl/src/wolfssl_stream.cpp | head -60Repository: cppalliance/corosio
Length of output: 1151
🌐 Web query:
WolfSSL_UseSNI return value error handling documentation
💡 Result:
wolfSSL_UseSNI() returns WOLFSSL_SUCCESS on success; on failure it returns a wolfSSL error code (not just 0/1), most notably: BAD_FUNC_ARG (e.g., ssl == NULL, data == NULL, or unknown type) or MEMORY_E (allocation failure). Treat anything other than WOLFSSL_SUCCESS as an error. [1][2]
Recommended pattern:
int ret = wolfSSL_UseSNI(ssl, WOLFSSL_SNI_HOST_NAME, host, (unsigned short)strlen(host));
if (ret != WOLFSSL_SUCCESS) {
/* ret is a specific error code like BAD_FUNC_ARG or MEMORY_E */
}The wolfSSL team has also noted that older docs/examples incorrectly used if (ret != 0)—the correct check is against WOLFSSL_SUCCESS. [3]
Sources: [1][2][3]
🌐 Web query:
WolfSSL_check_domain_name return value failure behavior
💡 Result:
wolfSSL_check_domain_name(ssl, dn) does not fail because the certificate’s name doesn’t match. It only enables the domain-name check that will be performed during the handshake.
-
Return values (from
wolfSSL_check_domain_name)SSL_SUCCESSon successSSL_FAILUREonly if a memory error was encountered while setting up the check (i.e., failure to enable the feature) [1][2]
-
Where “domain mismatch” shows up
- If you call
wolfSSL_check_domain_name()beforewolfSSL_connect()(client side), then a name mismatch causeswolfSSL_connect()to fail (fatal error), andwolfSSL_get_error()returnsDOMAIN_NAME_MISMATCH[3].
- If you call
Implication: if wolfSSL_check_domain_name() returns SSL_SUCCESS but the peer cert doesn’t match dn, you’ll see the failure at wolfSSL_connect()/wolfSSL_accept(), not at wolfSSL_check_domain_name(). [1][3]
References:
[1] wolfSSL Manual: wolfSSL_check_domain_name return values (group__Setup)
[2] wolfSSL Manual: ssl.h docs for wolfSSL_check_domain_name
[3] wolfSSL User Manual Ch.7: domain mismatch behavior (wolfSSL_connect() fatal error, DOMAIN_NAME_MISMATCH)
🏁 Script executed:
# Search for existing wolfSSL error checking patterns in the file
rg "(WOLFSSL_SUCCESS|SSL_SUCCESS|SSL_FAILURE)" src/wolfssl/src/wolfssl_stream.cppRepository: cppalliance/corosio
Length of output: 223
🏁 Script executed:
# Check the wolfSSL includes to understand which constants are available
rg "wolfssl" src/wolfssl/src/wolfssl_stream.cpp | head -20Repository: cppalliance/corosio
Length of output: 1091
🏁 Script executed:
# Look for other wolfSSL_check_domain_name usage in the codebase
rg "wolfSSL_check_domain_name" --type cppRepository: cppalliance/corosio
Length of output: 167
🏁 Script executed:
# Search for error handling patterns with wolfSSL functions
rg "wolfSSL.*!=" src/wolfssl/src/ | head -15Repository: cppalliance/corosio
Length of output: 45
Add error handling for SNI and hostname verification function calls.
wolfSSL_UseSNI() returns WOLFSSL_SUCCESS on success and specific error codes (e.g., BAD_FUNC_ARG, MEMORY_E) on failure. wolfSSL_check_domain_name() returns SSL_SUCCESS on success and SSL_FAILURE if memory allocation fails during setup. Both calls can fail silently, allowing the session to proceed without SNI or hostname verification—a security regression. Check both return values and fail initialization if either fails, following the error-handling pattern used for other SSL setup operations in this function.
Suggested fix pattern
- wolfSSL_UseSNI( ssl_, WOLFSSL_SNI_HOST_NAME,
- impl.hostname.data(),
- static_cast<unsigned short>( impl.hostname.size() ) );
+ if( wolfSSL_UseSNI( ssl_, WOLFSSL_SNI_HOST_NAME,
+ impl.hostname.data(),
+ static_cast<unsigned short>( impl.hostname.size() ) ) != WOLFSSL_SUCCESS )
+ return system::error_code(
+ wolfSSL_get_error( ssl_, 0 ),
+ system::system_category() );
- wolfSSL_check_domain_name( ssl_, impl.hostname.c_str() );
+ if( wolfSSL_check_domain_name( ssl_, impl.hostname.c_str() ) != WOLFSSL_SUCCESS )
+ return system::error_code(
+ wolfSSL_get_error( ssl_, 0 ),
+ system::system_category() );🤖 Prompt for AI Agents
In `@src/wolfssl/src/wolfssl_stream.cpp` around lines 1031 - 1041, The SNI and
hostname-verification calls may fail silently; after calling wolfSSL_UseNI
(wolfSSL_UseSNI) and wolfSSL_check_domain_name in the wolfssl_stream client
branch (when type == wolfssl_stream::client and impl.hostname not empty), check
their return values and treat non-success as an initialization failure: if
wolfSSL_UseSNI does not return WOLFSSL_SUCCESS or wolfSSL_check_domain_name does
not return SSL_SUCCESS, log the error and return/propagate the same failure path
used elsewhere in this function (match the existing error-handling pattern for
ssl_ setup), ensuring the session is aborted rather than allowed to continue
without SNI/hostname verification.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/openssl/src/openssl_stream.cpp`:
- Around line 181-208: The loop that reads intermediate certs with
PEM_read_bio_X509 and passes them to SSL_CTX_add_extra_chain_cert( ctx_, cert )
ignores the function's return value so a failed add leaks the X509*; update the
loop in openssl_stream.cpp to check the int return of
SSL_CTX_add_extra_chain_cert, and if it returns 0 free the cert with
X509_free(cert) (since ownership was not transferred) and handle the error
(e.g., break the loop and/or log via ERR_clear_error()/your logger) to avoid
leaks and undefined state; keep the existing BIO_free(bio) and ERR_clear_error()
behavior.
- Around line 806-815: The calls to SSL_set_tlsext_host_name(ssl_,
impl.hostname.c_str()) and SSL_set1_host(ssl_, impl.hostname.c_str()) must have
their return values checked; if either returns 0, log an error (including
impl.hostname) using the same logging/error pattern used elsewhere in init_ssl()
and abort/init failure (e.g., return false) so the function does not silently
succeed when SNI or hostname verification fails. Ensure you reference
SSL_set_tlsext_host_name and SSL_set1_host and handle their non-1 return by
emitting a clear log message and returning the existing failure value from
init_ssl().
- Around line 121-143: The code doesn't check SSL_CTX_get_ex_new_index() failure
before using sni_ctx_data_index; in the openssl_native_context constructor call
SSL_CTX_get_ex_new_index and if it returns -1 treat it as an error (do not call
SSL_CTX_set_ex_data or set the SNI callback), clean up the newly created ctx_
(SSL_CTX_free) and return/propagate failure appropriately; specifically, after
SSL_CTX_get_ex_new_index() assign to sni_ctx_data_index, check for -1 and handle
as described so subsequent calls to SSL_CTX_set_ex_data and
SSL_CTX_set_tlsext_servername_callback (when cd.servername_callback and
sni_callback are used) are only done with a valid index.
♻️ Duplicate comments (2)
src/wolfssl/src/wolfssl_stream.cpp (2)
111-128: Fix undefined SNI callback return codes.Line 112/124 uses
fatal_returnand bare0; this won’t compile and doesn’t express the SNI callback contract. Use the explicit WolfSSL/OpenSSL-compatible return constants instead (same issue as earlier feedback).🐛 Proposed fix
static int wolfssl_sni_callback( WOLFSSL* ssl, int* /* alert */, void* arg ) { char const* servername = wolfSSL_get_servername( ssl, WOLFSSL_SNI_HOST_NAME ); if( !servername ) - return 0; // No SNI sent, continue + return SSL_TLSEXT_ERR_OK; // No SNI sent, continue @@ if( !cd->servername_callback( servername ) ) - return fatal_return; // Callback rejected hostname + return SSL_TLSEXT_ERR_ALERT_FATAL; // Callback rejected hostname } - return 0; // Accept + return SSL_TLSEXT_ERR_OK; // Accept }wolfSSL servername callback return codes SSL_TLSEXT_ERR_OK SSL_TLSEXT_ERR_ALERT_FATAL
1031-1041: Handle failures fromwolfSSL_UseSNI/wolfSSL_check_domain_name.If these calls fail, SNI or hostname verification can be silently disabled. Check their return values and fail initialization when they don’t report success (same issue as earlier feedback).
🐛 Proposed fix
if( type == wolfssl_stream::client && !impl.hostname.empty() ) { // Set SNI extension so server knows which cert to present - wolfSSL_UseSNI( ssl_, WOLFSSL_SNI_HOST_NAME, - impl.hostname.data(), - static_cast<unsigned short>( impl.hostname.size() ) ); + if( wolfSSL_UseSNI( ssl_, WOLFSSL_SNI_HOST_NAME, + impl.hostname.data(), + static_cast<unsigned short>( impl.hostname.size() ) ) != WOLFSSL_SUCCESS ) + return system::error_code( + wolfSSL_get_error( ssl_, 0 ), + system::system_category() ); @@ // Enable hostname verification (checks CN/SAN in peer cert) - wolfSSL_check_domain_name( ssl_, impl.hostname.c_str() ); + if( wolfSSL_check_domain_name( ssl_, impl.hostname.c_str() ) != WOLFSSL_SUCCESS ) + return system::error_code( + wolfSSL_get_error( ssl_, 0 ), + system::system_category() ); }wolfSSL_UseSNI return value and error handling; wolfSSL_check_domain_name return value behavior
| context_data const* cd_; // For SNI callback access | ||
|
|
||
| explicit | ||
| openssl_native_context( context_data const& cd ) | ||
| : ctx_( nullptr ) | ||
| , cd_( &cd ) | ||
| { | ||
| // Create SSL_CTX supporting both client and server | ||
| ctx_ = SSL_CTX_new( TLS_method() ); | ||
| if( !ctx_ ) | ||
| return; | ||
|
|
||
| // Initialize ex_data index for SNI callback (once) | ||
| if( sni_ctx_data_index < 0 ) | ||
| sni_ctx_data_index = SSL_CTX_get_ex_new_index( 0, nullptr, nullptr, nullptr, nullptr ); | ||
|
|
||
| // Store context_data pointer for SNI callback access | ||
| SSL_CTX_set_ex_data( ctx_, sni_ctx_data_index, const_cast<context_data*>( &cd ) ); | ||
|
|
||
| // Set SNI callback if provided | ||
| if( cd.servername_callback ) | ||
| SSL_CTX_set_tlsext_servername_callback( ctx_, sni_callback ); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and inspect the relevant file
find . -name "openssl_stream.cpp" -type fRepository: cppalliance/corosio
Length of output: 182
🏁 Script executed:
# Read the specific section mentioned in the review (lines 121-143)
sed -n '121,143p' src/openssl/src/openssl_stream.cppRepository: cppalliance/corosio
Length of output: 888
🌐 Web query:
SSL_CTX_get_ex_new_index return value documentation OpenSSL
💡 Result:
SSL_CTX_get_ex_new_index() returns an ex_data index:
- Success: a new index (an
int). Per the generic*_get_ex_new_index()docs, this is “a new index”; 0 is a valid index value. [1] - Failure: -1. [2]
If it returns -1, consult the OpenSSL error queue (e.g., ERR_get_error()) for details. [1]
Where this is documented:
- OpenSSL 3.x documents
SSL_CTX_get_ex_new_index()under the genericTYPE_get_ex_new_index()/SSL_get_ex_new_index(3ssl)/BIO_get_ex_new_index(3)family, which states “returns a new index on success or -1 on error.” [2] - The older OpenSSL 1.0.2
SSL_CTX_get_ex_new_index(3)page points you toRSA_get_ex_new_index(3)for the detailed semantics/return values (including “-1 on failure, 0 is valid”). [1,3]
Sources: [1] [2] [3]
Guard against SSL_CTX_get_ex_new_index failure.
The code calls SSL_CTX_get_ex_new_index() at line 134 but does not check if it returns -1 (failure). Using -1 as an index in the subsequent SSL_CTX_set_ex_data() call is unsafe and may cause undefined behavior.
Suggested guard
if( sni_ctx_data_index < 0 )
- sni_ctx_data_index = SSL_CTX_get_ex_new_index( 0, nullptr, nullptr, nullptr, nullptr );
+ {
+ sni_ctx_data_index = SSL_CTX_get_ex_new_index( 0, nullptr, nullptr, nullptr, nullptr );
+ if( sni_ctx_data_index < 0 )
+ {
+ SSL_CTX_free( ctx_ );
+ ctx_ = nullptr;
+ return;
+ }
+ }🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 121 - 143, The code doesn't
check SSL_CTX_get_ex_new_index() failure before using sni_ctx_data_index; in the
openssl_native_context constructor call SSL_CTX_get_ex_new_index and if it
returns -1 treat it as an error (do not call SSL_CTX_set_ex_data or set the SNI
callback), clean up the newly created ctx_ (SSL_CTX_free) and return/propagate
failure appropriately; specifically, after SSL_CTX_get_ex_new_index() assign to
sni_ctx_data_index, check for -1 and handle as described so subsequent calls to
SSL_CTX_set_ex_data and SSL_CTX_set_tlsext_servername_callback (when
cd.servername_callback and sni_callback are used) are only done with a valid
index.
| // Apply certificate chain if provided (entity cert + intermediates) | ||
| // First cert is entity, rest are intermediates | ||
| if( !cd.certificate_chain.empty() ) | ||
| { | ||
| BIO* bio = BIO_new_mem_buf( | ||
| cd.certificate_chain.data(), | ||
| static_cast<int>( cd.certificate_chain.size() ) ); | ||
| if( bio ) | ||
| { | ||
| // First cert is entity certificate | ||
| X509* entity = PEM_read_bio_X509( bio, nullptr, nullptr, nullptr ); | ||
| if( entity ) | ||
| { | ||
| SSL_CTX_use_certificate( ctx_, entity ); | ||
| X509_free( entity ); | ||
| } | ||
|
|
||
| // Remaining certs are intermediates - add as extra chain certs | ||
| X509* cert; | ||
| while( ( cert = PEM_read_bio_X509( bio, nullptr, nullptr, nullptr ) ) != nullptr ) | ||
| { | ||
| // SSL_CTX_add_extra_chain_cert takes ownership - don't free | ||
| SSL_CTX_add_extra_chain_cert( ctx_, cert ); | ||
| } | ||
| ERR_clear_error(); // Clear expected EOF error from reading end of chain | ||
| BIO_free( bio ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SSL_CTX_add_extra_chain_cert OpenSSL return value ownership semantics
💡 Result:
- Return value:
SSL_CTX_add_extra_chain_cert(ctx, x509)returns 1 on success and 0 on failure (check the OpenSSL error stack for details). [1] - Ownership / lifetime: On success, OpenSSL takes ownership of the
X509 *x509you pass and will free it when theSSL_CTXis destroyed—so the application must not free thatX509afterward. [1] - On failure: Since OpenSSL did not take it, the caller should assume it still owns
x509and must free it (or otherwise manage it). (This follows from “will be freed by the library” applying only once it’s been successfully added.) [1] - If you need to keep using the cert: Prefer APIs with explicit refcount semantics like
SSL_CTX_add1_chain_cert()(increments refcount; caller can still free its copy) vsSSL_CTX_add0_chain_cert()(does not increment; caller must not free). [2]
Sources: OpenSSL manpages for SSL_CTX_add_extra_chain_cert(3) and chain-cert APIs. [1][2]
Check SSL_CTX_add_extra_chain_cert return and free on failure.
If SSL_CTX_add_extra_chain_cert fails (returns 0), ownership is not transferred and the caller must free the certificate. The current code ignores the return value, leaking the certificate on failure.
🛠️ Suggested fix
while( ( cert = PEM_read_bio_X509( bio, nullptr, nullptr, nullptr ) ) != nullptr )
{
- // SSL_CTX_add_extra_chain_cert takes ownership - don't free
- SSL_CTX_add_extra_chain_cert( ctx_, cert );
+ // SSL_CTX_add_extra_chain_cert takes ownership on success
+ if( SSL_CTX_add_extra_chain_cert( ctx_, cert ) != 1 )
+ {
+ X509_free( cert );
+ break;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Apply certificate chain if provided (entity cert + intermediates) | |
| // First cert is entity, rest are intermediates | |
| if( !cd.certificate_chain.empty() ) | |
| { | |
| BIO* bio = BIO_new_mem_buf( | |
| cd.certificate_chain.data(), | |
| static_cast<int>( cd.certificate_chain.size() ) ); | |
| if( bio ) | |
| { | |
| // First cert is entity certificate | |
| X509* entity = PEM_read_bio_X509( bio, nullptr, nullptr, nullptr ); | |
| if( entity ) | |
| { | |
| SSL_CTX_use_certificate( ctx_, entity ); | |
| X509_free( entity ); | |
| } | |
| // Remaining certs are intermediates - add as extra chain certs | |
| X509* cert; | |
| while( ( cert = PEM_read_bio_X509( bio, nullptr, nullptr, nullptr ) ) != nullptr ) | |
| { | |
| // SSL_CTX_add_extra_chain_cert takes ownership - don't free | |
| SSL_CTX_add_extra_chain_cert( ctx_, cert ); | |
| } | |
| ERR_clear_error(); // Clear expected EOF error from reading end of chain | |
| BIO_free( bio ); | |
| } | |
| } | |
| // Apply certificate chain if provided (entity cert + intermediates) | |
| // First cert is entity, rest are intermediates | |
| if( !cd.certificate_chain.empty() ) | |
| { | |
| BIO* bio = BIO_new_mem_buf( | |
| cd.certificate_chain.data(), | |
| static_cast<int>( cd.certificate_chain.size() ) ); | |
| if( bio ) | |
| { | |
| // First cert is entity certificate | |
| X509* entity = PEM_read_bio_X509( bio, nullptr, nullptr, nullptr ); | |
| if( entity ) | |
| { | |
| SSL_CTX_use_certificate( ctx_, entity ); | |
| X509_free( entity ); | |
| } | |
| // Remaining certs are intermediates - add as extra chain certs | |
| X509* cert; | |
| while( ( cert = PEM_read_bio_X509( bio, nullptr, nullptr, nullptr ) ) != nullptr ) | |
| { | |
| // SSL_CTX_add_extra_chain_cert takes ownership on success | |
| if( SSL_CTX_add_extra_chain_cert( ctx_, cert ) != 1 ) | |
| { | |
| X509_free( cert ); | |
| break; | |
| } | |
| } | |
| ERR_clear_error(); // Clear expected EOF error from reading end of chain | |
| BIO_free( bio ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 181 - 208, The loop that
reads intermediate certs with PEM_read_bio_X509 and passes them to
SSL_CTX_add_extra_chain_cert( ctx_, cert ) ignores the function's return value
so a failed add leaks the X509*; update the loop in openssl_stream.cpp to check
the int return of SSL_CTX_add_extra_chain_cert, and if it returns 0 free the
cert with X509_free(cert) (since ownership was not transferred) and handle the
error (e.g., break the loop and/or log via ERR_clear_error()/your logger) to
avoid leaks and undefined state; keep the existing BIO_free(bio) and
ERR_clear_error() behavior.
| // Apply per-session config (SNI + hostname verification) from context | ||
| if( !impl.hostname.empty() ) | ||
| { | ||
| // Set SNI extension so server knows which cert to present | ||
| SSL_set_tlsext_host_name( ssl_, impl.hostname.c_str() ); | ||
|
|
||
| // Enable hostname verification (checks CN/SAN in peer cert) | ||
| // Available in OpenSSL 1.0.2+ | ||
| SSL_set1_host( ssl_, impl.hostname.c_str() ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file around lines 806-815 to understand the context
cat -n src/openssl/src/openssl_stream.cpp | sed -n '795,830p'Repository: cppalliance/corosio
Length of output: 182
🏁 Script executed:
# Let's also see what function this code is in
cat -n src/openssl/src/openssl_stream.cpp | sed -n '750,820p'Repository: cppalliance/corosio
Length of output: 2812
🌐 Web query:
OpenSSL SSL_set_tlsext_host_name return value documentation
💡 Result:
SSL_set_tlsext_host_name(ssl, name) (SNI) is documented to return:
1on success0on error (docs.openssl.org)
Citations:
🌐 Web query:
OpenSSL SSL_set1_host return value documentation
💡 Result:
SSL_set1_host() returns 1 on success and 0 on failure (same for SSL_add1_host()). [1][2]
Authoritative docs: OpenSSL manpage “SSL_set1_host”. [1][2]
Check return values for SNI and hostname verification functions.
SSL_set_tlsext_host_name and SSL_set1_host return 0 on failure and 1 on success. Currently, both are called without checking return values (lines 810, 814), which is inconsistent with the error handling pattern in the rest of init_ssl() and creates a silent failure mode. If these calls fail, SNI and hostname verification will not be enabled despite the function returning success.
Proposed fix
if( !impl.hostname.empty() )
{
// Set SNI extension so server knows which cert to present
- SSL_set_tlsext_host_name( ssl_, impl.hostname.c_str() );
+ if( SSL_set_tlsext_host_name( ssl_, impl.hostname.c_str() ) != 1 )
+ return system::error_code(
+ static_cast<int>( ERR_get_error() ),
+ system::system_category() );
// Enable hostname verification (checks CN/SAN in peer cert)
// Available in OpenSSL 1.0.2+
- SSL_set1_host( ssl_, impl.hostname.c_str() );
+ if( SSL_set1_host( ssl_, impl.hostname.c_str() ) != 1 )
+ return system::error_code(
+ static_cast<int>( ERR_get_error() ),
+ system::system_category() );
}🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 806 - 815, The calls to
SSL_set_tlsext_host_name(ssl_, impl.hostname.c_str()) and SSL_set1_host(ssl_,
impl.hostname.c_str()) must have their return values checked; if either returns
0, log an error (including impl.hostname) using the same logging/error pattern
used elsewhere in init_ssl() and abort/init failure (e.g., return false) so the
function does not silently succeed when SNI or hostname verification fails.
Ensure you reference SSL_set_tlsext_host_name and SSL_set1_host and handle their
non-1 return by emitting a clear log message and returning the existing failure
value from init_ssl().
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 358-366: The CI uses lukka/run-vcpkg with vcpkgJsonGlob
'**/corosio-root/vcpkg.json' but that glob matches no manifest so TLS libs
aren't installed; either add a vcpkg.json manifest at corosio-root/vcpkg.json
listing the required packages (wolfssl and openssl) so the action
(vcpkgDirectory, vcpkgJsonGlob) finds and installs them for Windows, or update
vcpkgJsonGlob to point to the actual manifest location used by your repository
so CMakeLists.txt's find_package(WolfSSL) and find_package(OpenSSL) succeed
during configuration.
♻️ Duplicate comments (3)
src/openssl/src/openssl_stream.cpp (3)
155-164: Guard againstSSL_CTX_get_ex_new_indexfailure.
SSL_CTX_get_ex_new_index()returns-1on failure. The current code doesn't check this, and using-1as an index inSSL_CTX_set_ex_data()causes undefined behavior.Suggested fix
// Initialize ex_data index for SNI callback (once) if( sni_ctx_data_index < 0 ) + { sni_ctx_data_index = SSL_CTX_get_ex_new_index( 0, nullptr, nullptr, nullptr, nullptr ); + if( sni_ctx_data_index < 0 ) + { + SSL_CTX_free( ctx_ ); + ctx_ = nullptr; + return; + } + } // Store context_data pointer for SNI callback access SSL_CTX_set_ex_data( ctx_, sni_ctx_data_index, const_cast<context_data*>( &cd ) );
220-227: CheckSSL_CTX_add_extra_chain_certreturn and free on failure.
SSL_CTX_add_extra_chain_certreturns 0 on failure and does not take ownership. The current code ignores the return value, leaking the certificate on failure.Suggested fix
// Remaining certs are intermediates - add as extra chain certs X509* cert; while( ( cert = PEM_read_bio_X509( bio, nullptr, nullptr, nullptr ) ) != nullptr ) { - // SSL_CTX_add_extra_chain_cert takes ownership - don't free - SSL_CTX_add_extra_chain_cert( ctx_, cert ); + // SSL_CTX_add_extra_chain_cert takes ownership on success + if( SSL_CTX_add_extra_chain_cert( ctx_, cert ) != 1 ) + { + X509_free( cert ); + break; + } }
870-879: Check return values for SNI and hostname verification functions.
SSL_set_tlsext_host_nameandSSL_set1_hostreturn 0 on failure. The current code ignores these return values, which means SNI and hostname verification could silently fail whileinit_ssl()reports success.Suggested fix
// Apply per-session config (SNI + hostname verification) from context if( !impl.hostname.empty() ) { // Set SNI extension so server knows which cert to present - SSL_set_tlsext_host_name( ssl_, impl.hostname.c_str() ); - + if( SSL_set_tlsext_host_name( ssl_, impl.hostname.c_str() ) != 1 ) + { + unsigned long err = ERR_get_error(); + return system::error_code( + static_cast<int>( err ), system::system_category() ); + } + // Enable hostname verification (checks CN/SAN in peer cert) // Available in OpenSSL 1.0.2+ - SSL_set1_host( ssl_, impl.hostname.c_str() ); + if( SSL_set1_host( ssl_, impl.hostname.c_str() ) != 1 ) + { + unsigned long err = ERR_get_error(); + return system::error_code( + static_cast<int>( err ), system::system_category() ); + } }
🧹 Nitpick comments (3)
src/openssl/src/openssl_stream.cpp (1)
32-50: Debug scaffolding: consider gating or removing before release.This debug logging infrastructure is useful for CI timeout investigation, but for production code:
- The
std::cerroutput and environment variable check add minor runtime overhead- Consider wrapping with
#ifdef COROSIO_DEBUGor similar to exclude from release buildsIf this is intended to remain, this is acceptable as-is since the check is cached and output is conditional.
src/wolfssl/src/wolfssl_stream.cpp (1)
158-175: Certificate loading functions lack error handling.
wolfSSL_CTX_use_certificate_chain_bufferandwolfSSL_CTX_use_certificate_buffercan fail (invalid format, memory errors). Silent failures during context setup could result in TLS connections proceeding without a valid certificate, which would cause handshake failures later with unclear error origins.Consider returning an error or logging on failure, similar to how the OpenSSL variant behaves.
.github/workflows/ci.yml (1)
37-39: Consider making TLS debug flags opt‑in to reduce CI log noise.If these are mainly for troubleshooting, sourcing them from repo variables (or workflow inputs) keeps routine runs quieter.
💡 Example using repository variables
- COROSIO_TLS_TEST_VERBOSE: "1" - COROSIO_TLS_DEBUG: "1" + COROSIO_TLS_TEST_VERBOSE: ${{ vars.COROSIO_TLS_TEST_VERBOSE || '0' }} + COROSIO_TLS_DEBUG: ${{ vars.COROSIO_TLS_DEBUG || '0' }}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 384-412: The workflow hardcodes the MSVC triplet via the
x64-windows path causing MinGW jobs to use incompatible binaries; before calling
the lukka/run-vcpkg step set VCPKG_DEFAULT_TRIPLET conditionally based on
runner/compiler (e.g., set to x64-mingw or x64-windows) and then compute
vcpkg_installed from that variable (use ${VCPKG_DEFAULT_TRIPLET} in the path
instead of x64-windows); update subsequent environment exports
(CMAKE_TOOLCHAIN_FILE, WOLFSSL_INCLUDE, WOLFSSL_LIBRARY_PATH, OPENSSL_INCLUDE,
OPENSSL_LIBRARY_PATH) to reference the computed vcpkg_installed variable so
MinGW jobs pick the correct triplet artifacts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 414-433: The workflow hardcodes "x64-linux" for vcpkg; instead
derive a VCPKG_TRIPLET from the existing matrix.x86 (use "x86-linux" when
matrix.x86==32, otherwise "x64-linux"), then replace all hardcoded occurrences
by using that triplet when computing vcpkg_installed (vcpkg_installed="${{
github.workspace }}/vcpkg/vcpkg_installed/${VCPKG_TRIPLET}") and when exporting
CMAKE_PREFIX_PATH, WOLFSSL_INCLUDE and WOLFSSL_LIBRARY_PATH environment
variables; update references to the symbols vcpkg_installed, CMAKE_PREFIX_PATH,
WOLFSSL_INCLUDE and WOLFSSL_LIBRARY_PATH and ensure matrix.x86 is used to select
the triplet.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
385-412: Select a MinGW vcpkg triplet instead of hardcodingx64-windows.
Hardcoding the MSVC triplet and path will make the MinGW job consume incompatible binaries. SetVCPKG_DEFAULT_TRIPLET(and host triplet) based onmatrix.compiler, then derivevcpkg_installedfrom it.🛠️ Proposed fix
+ - name: Select vcpkg triplet (Windows) + if: runner.os == 'Windows' + shell: bash + run: | + if [[ "${{ matrix.compiler }}" == "mingw" ]]; then + echo "VCPKG_DEFAULT_TRIPLET=x64-mingw-dynamic" >> $GITHUB_ENV + echo "VCPKG_DEFAULT_HOST_TRIPLET=x64-mingw-dynamic" >> $GITHUB_ENV + else + echo "VCPKG_DEFAULT_TRIPLET=x64-windows" >> $GITHUB_ENV + fi + - name: Setup vcpkg uses: lukka/run-vcpkg@v11 with: vcpkgDirectory: ${{ github.workspace }}/vcpkg vcpkgGitCommitId: bd52fac7114fdaa2208de8dd1227212a6683e562 vcpkgJsonGlob: '**/corosio-root/vcpkg.json' runVcpkgInstall: true - name: Set vcpkg paths (Windows) if: runner.os == 'Windows' shell: bash run: | - vcpkg_installed="${{ github.workspace }}/vcpkg/vcpkg_installed/x64-windows" + vcpkg_installed="${{ github.workspace }}/vcpkg/vcpkg_installed/${VCPKG_DEFAULT_TRIPLET}"What are the supported vcpkg MinGW triplet names and the recommended VCPKG_DEFAULT_HOST_TRIPLET for Windows MinGW builds?
| - name: Set vcpkg paths (Linux) | ||
| if: runner.os == 'Linux' | ||
| shell: bash | ||
| run: | | ||
| vcpkg_installed="${{ github.workspace }}/vcpkg/vcpkg_installed/x64-linux" | ||
| # Debug: show what was installed | ||
| echo "Checking vcpkg installed packages:" | ||
| ls -la "${vcpkg_installed}/" || true | ||
| ls -la "${vcpkg_installed}/include/" || true | ||
| ls -la "${vcpkg_installed}/lib/" || true | ||
| # For CMake - use system OpenSSL, vcpkg WolfSSL via prefix path | ||
| # (Don't use vcpkg toolchain to avoid building OpenSSL from source) | ||
| echo "CMAKE_PREFIX_PATH=${vcpkg_installed}" >> $GITHUB_ENV | ||
| # For B2 - WolfSSL from vcpkg, OpenSSL from system | ||
| echo "WOLFSSL_INCLUDE=${vcpkg_installed}/include" >> $GITHUB_ENV | ||
| echo "WOLFSSL_LIBRARY_PATH=${vcpkg_installed}/lib" >> $GITHUB_ENV | ||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && pwdRepository: cppalliance/corosio
Length of output: 67
🏁 Script executed:
# First, let's check if the file exists and get its general structure
if [ -f ".github/workflows/ci.yml" ]; then
echo "File exists"
wc -l .github/workflows/ci.yml
else
echo "File not found, searching for it"
find . -name "ci.yml" -type f 2>/dev/null | head -5
fiRepository: cppalliance/corosio
Length of output: 103
🏁 Script executed:
# Let's look at the matrix configuration first
rg "matrix:" -A 10 .github/workflows/ci.yml | head -50Repository: cppalliance/corosio
Length of output: 667
🏁 Script executed:
# Search for all hardcoded x64-linux references
rg "x64-linux" .github/workflows/ci.yml -nRepository: cppalliance/corosio
Length of output: 751
🏁 Script executed:
# Check lines around 414-433
sed -n '410,440p' .github/workflows/ci.ymlRepository: cppalliance/corosio
Length of output: 1730
🏁 Script executed:
# Check the other referenced lines
sed -n '470,480p' .github/workflows/ci.yml
sed -n '505,515p' .github/workflows/ci.yml
sed -n '528,540p' .github/workflows/ci.yml
sed -n '553,565p' .github/workflows/ci.ymlRepository: cppalliance/corosio
Length of output: 2674
🏁 Script executed:
# Search for x86 in the matrix configuration
rg "x86" .github/workflows/ci.yml -B 2 -A 2Repository: cppalliance/corosio
Length of output: 850
🏁 Script executed:
# Get the full matrix configuration
sed -n '30,200p' .github/workflows/ci.yml | grep -A 150 "matrix:"Repository: cppalliance/corosio
Length of output: 4441
🌐 Web query:
vcpkg triplet x86-linux 32-bit Linux
💡 Result:
vcpkg’s built-in / CI-tested Linux triplet is x64-linux (Linux x64 only). 32-bit Linux (x86) isn’t an officially tested built-in target, so you typically use a community triplet (if present in your vcpkg checkout) or create your own. [1]
1) Check whether your vcpkg already has x86-linux
Run:
vcpkg help triplet(or look under triplets/community/). Community triplets live in triplets\community and are not CI-guaranteed. [2]
2) If it’s missing: create an x86-linux overlay triplet (recommended)
Copy an existing Linux triplet and change the architecture:
mkdir -p custom-triplets
cp vcpkg/triplets/x64-linux.cmake custom-triplets/x86-linux.cmakeEdit custom-triplets/x86-linux.cmake to use x86 + Linux, for example:
set(VCPKG_TARGET_ARCHITECTURE x86)
set(VCPKG_CMAKE_SYSTEM_NAME Linux)
# keep or adjust linkage as you need:
# set(VCPKG_LIBRARY_LINKAGE static) # or dynamicThen install using the overlay triplet:
vcpkg install <port>:x86-linux --overlay-triplets=custom-tripletsOverlay triplets are the documented way to add/replace triplets. [2]
Use the correct vcpkg triplet for both 32‑bit and 64‑bit Linux builds.
The workflow includes an x86 build (Clang 20: C++20-23 (x86)) but hardcodes x64-linux in five places: the vcpkg_installed variable (line 418) and four CMAKE_PREFIX_PATH invocations (lines 477, 510, 535, 559). This will attempt to link 64‑bit libraries into a 32‑bit build, causing linker failures.
Select the vcpkg triplet dynamically based on matrix.x86 (the workflow already uses this variable for address-model). The correct triplet name is x86-linux for 32‑bit Linux, which is available as a community triplet in vcpkg.
🛠️ Proposed fix
+ - name: Select vcpkg triplet (Linux)
+ if: runner.os == 'Linux'
+ shell: bash
+ run: |
+ if [[ "${{ matrix.x86 }}" == "true" ]]; then
+ echo "VCPKG_DEFAULT_TRIPLET=x86-linux" >> $GITHUB_ENV
+ else
+ echo "VCPKG_DEFAULT_TRIPLET=x64-linux" >> $GITHUB_ENV
+ fi
+
- name: Set vcpkg paths (Linux)
if: runner.os == 'Linux'
shell: bash
run: |
- vcpkg_installed="${{ github.workspace }}/vcpkg/vcpkg_installed/x64-linux"
+ vcpkg_installed="${{ github.workspace }}/vcpkg/vcpkg_installed/${VCPKG_DEFAULT_TRIPLET}"
+ echo "VCPKG_INSTALLED=${vcpkg_installed}" >> $GITHUB_ENV
...
echo "CMAKE_PREFIX_PATH=${vcpkg_installed}" >> $GITHUB_ENV
...
extra-args: |
-D Boost_VERBOSE=ON
-D BOOST_INCLUDE_LIBRARIES="${{ steps.patch.outputs.module }}"
- ${{ runner.os == 'Linux' && format('-D CMAKE_PREFIX_PATH={0}/vcpkg/vcpkg_installed/x64-linux', github.workspace) || '' }}
+ ${{ runner.os == 'Linux' && format('-D CMAKE_PREFIX_PATH={0}/vcpkg/vcpkg_installed/{1}', github.workspace, env.VCPKG_DEFAULT_TRIPLET) || '' }}
...
extra-args: |
-D BOOST_CI_INSTALL_TEST=ON
- -D CMAKE_PREFIX_PATH=${{ steps.patch.outputs.workspace_root }}/.local${{ runner.os == 'Linux' && format(';{0}/vcpkg/vcpkg_installed/x64-linux', github.workspace) || '' }}
+ -D CMAKE_PREFIX_PATH=${{ steps.patch.outputs.workspace_root }}/.local${{ runner.os == 'Linux' && format(';{0}/vcpkg/vcpkg_installed/{1}', github.workspace, env.VCPKG_DEFAULT_TRIPLET) || '' }}
...
extra-args: |
-D BOOST_CI_INSTALL_TEST=OFF
- ${{ runner.os == 'Linux' && format('-D CMAKE_PREFIX_PATH={0}/vcpkg/vcpkg_installed/x64-linux', github.workspace) || '' }}
+ ${{ runner.os == 'Linux' && format('-D CMAKE_PREFIX_PATH={0}/vcpkg/vcpkg_installed/{1}', github.workspace, env.VCPKG_DEFAULT_TRIPLET) || '' }}
...
extra-args: |
- ${{ runner.os == 'Linux' && format('-D CMAKE_PREFIX_PATH={0}/vcpkg/vcpkg_installed/x64-linux', github.workspace) || '' }}
+ ${{ runner.os == 'Linux' && format('-D CMAKE_PREFIX_PATH={0}/vcpkg/vcpkg_installed/{1}', github.workspace, env.VCPKG_DEFAULT_TRIPLET) || '' }}🤖 Prompt for AI Agents
In @.github/workflows/ci.yml around lines 414 - 433, The workflow hardcodes
"x64-linux" for vcpkg; instead derive a VCPKG_TRIPLET from the existing
matrix.x86 (use "x86-linux" when matrix.x86==32, otherwise "x64-linux"), then
replace all hardcoded occurrences by using that triplet when computing
vcpkg_installed (vcpkg_installed="${{ github.workspace
}}/vcpkg/vcpkg_installed/${VCPKG_TRIPLET}") and when exporting
CMAKE_PREFIX_PATH, WOLFSSL_INCLUDE and WOLFSSL_LIBRARY_PATH environment
variables; update references to the symbols vcpkg_installed, CMAKE_PREFIX_PATH,
WOLFSSL_INCLUDE and WOLFSSL_LIBRARY_PATH and ensure matrix.x86 is used to select
the triplet.
2d1bd97 to
04696a6
Compare
MungoG
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wolfssl/src/wolfssl_stream.cpp (1)
158-175: Missing error handling for certificate chain loading.
wolfSSL_CTX_use_certificate_chain_bufferreturnsWOLFSSL_SUCCESSon success and an error code on failure. The return value is ignored, which could silently fail certificate chain setup.Suggested fix
if( !cd.certificate_chain.empty() ) { - wolfSSL_CTX_use_certificate_chain_buffer( ctx, + int ret = wolfSSL_CTX_use_certificate_chain_buffer( ctx, reinterpret_cast<unsigned char const*>( cd.certificate_chain.data() ), static_cast<long>( cd.certificate_chain.size() ) ); + (void)ret; // TODO: Consider logging or propagating failure }
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/epoll/sockets.cpp`:
- Around line 99-112: The code sets op.registered (registered.store(true))
before calling svc_.scheduler().register_fd(fd_), which allows
cancel_single_op() to flip registered to false while the fd is not yet
registered, causing unregister_fd() to be called on a non-registered fd and
leaving a stale fd registered later; fix by moving the registered.store(true) to
immediately after a successful svc_.scheduler().register_fd(fd_) call, then
re-check op.cancelled (using op.registered.exchange(false) as currently done)
and perform the same cleanup sequence (svc_.scheduler().unregister_fd(fd_), set
op.impl_ptr via shared_from_this(), svc_.post(&op), svc_.work_finished()) if it
is cancelled; apply this same change pattern in the other occurrences in
read_some, write_some, and accept to ensure registration is only visible after
register_fd() succeeds and cancellation is handled consistently.
In `@src/corosio/src/tls/detail/context_impl.hpp`:
- Around line 74-78: Add a direct include for <string_view> in the header that
defines servername_callback so the dependency on std::string_view is explicit;
update the top-of-file includes in the context_impl.hpp header (where the member
std::function<bool(std::string_view)> servername_callback is declared) to
`#include` <string_view> to avoid relying on transitive includes from
boost/corosio/tls/context.hpp.
♻️ Duplicate comments (14)
src/wolfssl/src/wolfssl_stream.cpp (2)
111-128: Fix undefined identifierfatal_return; use proper WolfSSL constants.The callback uses undefined identifier
fatal_returnat line 124, which will cause a compilation error. The comment at line 112 referencesSNICbReturn enumbutfatal_returnis not a valid constant in WolfSSL.Recommended fix using WolfSSL OpenSSL-compat constants
// SNI callback invoked by WolfSSL during handshake (server-side) -// Returns SNICbReturn enum: 0 = OK, fatal_return (2) = abort +// Returns SSL_TLSEXT_ERR_OK (0) to accept, SSL_TLSEXT_ERR_ALERT_FATAL (2) to reject static int wolfssl_sni_callback( WOLFSSL* ssl, int* /* alert */, void* arg ) { char const* servername = wolfSSL_get_servername( ssl, WOLFSSL_SNI_HOST_NAME ); if( !servername ) - return 0; // No SNI sent, continue + return SSL_TLSEXT_ERR_OK; // No SNI sent, continue auto* cd = static_cast<context_data const*>( arg ); if( cd && cd->servername_callback ) { if( !cd->servername_callback( servername ) ) - return fatal_return; // Callback rejected hostname + return SSL_TLSEXT_ERR_ALERT_FATAL; // Callback rejected hostname } - return 0; // Accept + return SSL_TLSEXT_ERR_OK; // Accept }
1039-1049: Add error handling for SNI and hostname verification function calls.
wolfSSL_UseSNI()returnsWOLFSSL_SUCCESSon success and specific error codes on failure.wolfSSL_check_domain_name()returnsSSL_SUCCESSon success. Both calls are currently unchecked, which could silently skip SNI/hostname verification setup.Suggested fix
// Apply per-session config (SNI + hostname verification) from context if( type == wolfssl_stream::client && !impl.hostname.empty() ) { // Set SNI extension so server knows which cert to present - wolfSSL_UseSNI( ssl_, WOLFSSL_SNI_HOST_NAME, + if( wolfSSL_UseSNI( ssl_, WOLFSSL_SNI_HOST_NAME, impl.hostname.data(), - static_cast<unsigned short>( impl.hostname.size() ) ); + static_cast<unsigned short>( impl.hostname.size() ) ) != WOLFSSL_SUCCESS ) + { + int err = wolfSSL_get_error( ssl_, 0 ); + return system::error_code( err, system::system_category() ); + } // Enable hostname verification (checks CN/SAN in peer cert) - wolfSSL_check_domain_name( ssl_, impl.hostname.c_str() ); + if( wolfSSL_check_domain_name( ssl_, impl.hostname.c_str() ) != WOLFSSL_SUCCESS ) + { + int err = wolfSSL_get_error( ssl_, 0 ); + return system::error_code( err, system::system_category() ); + } }src/openssl/src/openssl_stream.cpp (3)
155-165: Guard againstSSL_CTX_get_ex_new_indexfailure.
SSL_CTX_get_ex_new_index()returns-1on failure. Using-1as an index inSSL_CTX_set_ex_data()is unsafe. The code should check for failure before proceeding.Suggested guard
// Initialize ex_data index for SNI callback (once) if( sni_ctx_data_index < 0 ) + { sni_ctx_data_index = SSL_CTX_get_ex_new_index( 0, nullptr, nullptr, nullptr, nullptr ); + if( sni_ctx_data_index < 0 ) + { + SSL_CTX_free( ctx_ ); + ctx_ = nullptr; + return; + } + }
219-230: CheckSSL_CTX_add_extra_chain_certreturn and free on failure.If
SSL_CTX_add_extra_chain_certfails (returns 0), ownership is not transferred and the caller must free the certificate to avoid a memory leak.Suggested fix
// Remaining certs are intermediates - add as extra chain certs X509* cert; while( ( cert = PEM_read_bio_X509( bio, nullptr, nullptr, nullptr ) ) != nullptr ) { - // SSL_CTX_add_extra_chain_cert takes ownership - don't free - SSL_CTX_add_extra_chain_cert( ctx_, cert ); + // SSL_CTX_add_extra_chain_cert takes ownership on success + if( SSL_CTX_add_extra_chain_cert( ctx_, cert ) != 1 ) + { + X509_free( cert ); + break; + } }
870-879: Check return values for SNI and hostname verification functions.
SSL_set_tlsext_host_nameandSSL_set1_hostreturn 0 on failure and 1 on success. Currently, both are called without checking return values, creating a silent failure mode for security-critical functionality.Proposed fix
// Apply per-session config (SNI + hostname verification) from context if( !impl.hostname.empty() ) { // Set SNI extension so server knows which cert to present - SSL_set_tlsext_host_name( ssl_, impl.hostname.c_str() ); + if( SSL_set_tlsext_host_name( ssl_, impl.hostname.c_str() ) != 1 ) + return system::error_code( + static_cast<int>( ERR_get_error() ), + system::system_category() ); // Enable hostname verification (checks CN/SAN in peer cert) // Available in OpenSSL 1.0.2+ - SSL_set1_host( ssl_, impl.hostname.c_str() ); + if( SSL_set1_host( ssl_, impl.hostname.c_str() ) != 1 ) + return system::error_code( + static_cast<int>( ERR_get_error() ), + system::system_category() ); }.github/workflows/ci.yml (2)
393-413: Hardcodedx64-windowstriplet will break MinGW builds.The workflow includes a MinGW configuration (line 93-107:
generator: MinGW Makefiles) but the vcpkg path at line 397 hardcodesx64-windows, which provides MSVC-compiled binaries incompatible with MinGW.Suggested fix: Select triplet based on compiler
- name: Set vcpkg paths (Windows) if: runner.os == 'Windows' shell: bash run: | - vcpkg_installed="${{ github.workspace }}/vcpkg/vcpkg_installed/x64-windows" + if [[ "${{ matrix.compiler }}" == "mingw" ]]; then + vcpkg_triplet="x64-mingw-dynamic" + else + vcpkg_triplet="x64-windows" + fi + vcpkg_installed="${{ github.workspace }}/vcpkg/vcpkg_installed/${vcpkg_triplet}"
414-454: Hardcodedx64-linuxtriplet will break 32-bit builds.The workflow includes an x86 build (line 247-261:
Clang 20: C++20-23 (x86)withx86: true) but lines 423-431 hardcodex64-linux, which provides 64-bit libraries incompatible with 32-bit builds.Suggested fix: Select triplet based on matrix.x86
- name: Set vcpkg paths (Linux) if: runner.os == 'Linux' id: vcpkg-paths-linux shell: bash run: | + # Select triplet based on architecture + if [[ "${{ matrix.x86 }}" == "true" ]]; then + vcpkg_triplet="x86-linux" + else + vcpkg_triplet="x64-linux" + fi + # lukka/run-vcpkg sets VCPKG_INSTALLED_DIR with a UUID-based path # Use that directly instead of trying to find it echo "Debug: VCPKG_INSTALLED_DIR=${VCPKG_INSTALLED_DIR:-not set}" - if [ -n "${VCPKG_INSTALLED_DIR}" ] && [ -d "${VCPKG_INSTALLED_DIR}/x64-linux" ]; then - vcpkg_installed="${VCPKG_INSTALLED_DIR}/x64-linux" + if [ -n "${VCPKG_INSTALLED_DIR}" ] && [ -d "${VCPKG_INSTALLED_DIR}/${vcpkg_triplet}" ]; then + vcpkg_installed="${VCPKG_INSTALLED_DIR}/${vcpkg_triplet}" else # Fallback: try to find it - vcpkg_installed=$(find "${GITHUB_WORKSPACE}" -type d -path "*/vcpkg_installed/x64-linux" 2>/dev/null | head -1) + vcpkg_installed=$(find "${GITHUB_WORKSPACE}" -type d -path "*/vcpkg_installed/${vcpkg_triplet}" 2>/dev/null | head -1)src/corosio/src/detail/epoll/sockets.cpp (7)
137-138: Duplicate of Line 81-82 (bind op to impl).
217-218: Duplicate of Line 81-82 (bind op to impl).
393-394: Duplicate of Line 81-82 (bind op to impl).
178-192: Duplicate of Line 99-112 (cancellation window).
254-267: Duplicate of Line 99-112 (cancellation window).
425-438: Duplicate of Line 99-112 (cancellation window).
469-489: Duplicate of Line 321-344 (per‑op cancellation implementation).
🧹 Nitpick comments (3)
example/https-client/https_client.cpp (2)
23-25: Missing high-level overview comment after includes.As per coding guidelines, there should be a
/* */section after the includes providing a high-level overview of how the example works. Consider adding something like:namespace corosio = boost::corosio; namespace capy = boost::capy; + +/* + * This example demonstrates a simple HTTPS client using corosio's + * TLS/WolfSSL stream. It connects to a server, performs a TLS handshake + * with hostname verification (SNI), sends an HTTP GET request, and + * prints the response to stdout. + */
70-73: Inconsistent error handling style.Lines 72-73 still use
.value()which will throw on failure, while the rest of the changed code uses explicit error code checks with structured bindings. Consider updating these for consistency:- ctx.set_default_verify_paths().value(); - ctx.set_verify_mode(corosio::tls::verify_mode::peer).value(); + if (auto ec = ctx.set_default_verify_paths()) + throw boost::system::system_error(ec); + if (auto ec = ctx.set_verify_mode(corosio::tls::verify_mode::peer)) + throw boost::system::system_error(ec);src/corosio/src/detail/epoll/sockets.cpp (1)
30-46: Add the required file overview block after includes.
The new canceller section is clear, but this .cpp still lacks the mandated /* */ high‑level overview immediately after the includes. As per coding guidelines, please add it.
MungoG
force-pushed
the
tls-testing-fixes
branch
from
04696a6 to
9a416ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/epoll/op.hpp`:
- Around line 113-116: Make the impl pointer members epoll_socket_impl*
socket_impl_ and epoll_acceptor_impl* acceptor_impl_ atomic to avoid UB from
concurrent stop_callback reads and start() writes: change them to
std::atomic<epoll_socket_impl*> and std::atomic<epoll_acceptor_impl*> and ensure
all assignments use store(..., std::memory_order_release); then update
epoll_op::canceller::operator() in src/corosio/src/detail/epoll/sockets.cpp to
read these pointers with load(std::memory_order_acquire) (or capture the impl
pointer into the canceller at construction) so the canceller observes a
consistent, race-free value; apply the same atomicization to the other
occurrences noted (lines 171-201).
♻️ Duplicate comments (1)
src/corosio/src/detail/epoll/sockets.cpp (1)
96-111:registeredstill set beforeregister_fd; race persists.If cancellation hits between
registered.store(true)andregister_fd(),cancel_single_op()can unregister a not-yet-registered fd and the laterregister_fd()leaves a stale registration. The new post-registration check doesn’t fix that path becauseregisteredis already false. Move theregistered.store(true)to after successful registration, then re-checkcancelled.🛠️ Example fix (apply same pattern in read/write/accept)
- op.registered.store(true, std::memory_order_release); - svc_.scheduler().register_fd(fd_, &op, EPOLLOUT | EPOLLET); + svc_.scheduler().register_fd(fd_, &op, EPOLLOUT | EPOLLET); + op.registered.store(true, std::memory_order_release); // If cancelled was set before we registered, handle it now. if (op.cancelled.load(std::memory_order_acquire)) { bool was_registered = op.registered.exchange(false, std::memory_order_acq_rel); if (was_registered) { svc_.scheduler().unregister_fd(fd_); op.impl_ptr = shared_from_this(); svc_.post(&op); svc_.work_finished(); } }Also applies to: 177-193, 254-269, 425-439
🧹 Nitpick comments (1)
cmake/FindWolfSSL.cmake (1)
13-22: Use PATH_SUFFIXES for portable lib/lib64 handlingCMake's
find_library()should usePATH_SUFFIXESrather than hardcoded environment variable expansions for better portability across lib and lib64 layouts:💡 Recommended approach
find_library(WolfSSL_LIBRARY NAMES "wolfssl" "libwolfssl" HINTS "$ENV{WOLFSSL_LIBRARY_PATH}" "$ENV{WOLFSSL_ROOT}" + PATH_SUFFIXES + lib + lib64 )This allows CMake's built-in search logic to handle both standard and non-standard installations consistently. The
PATH_SUFFIXESmechanism is the idiomatic CMake approach for such cases.
| // For stop_token cancellation - pointer to owning socket/acceptor impl. | ||
| // When stop is requested, we call back to the impl to perform actual I/O cancellation. | ||
| epoll_socket_impl* socket_impl_ = nullptr; | ||
| epoll_acceptor_impl* acceptor_impl_ = nullptr; |
There was a problem hiding this comment.
Make impl pointers atomic to avoid stop_token data races.
stop_callback can run on another thread while start() writes socket_impl_ / acceptor_impl_. Unsynchronized read/write is UB. Use atomics (or capture the impl pointer inside the canceller). Also update the canceller to load() the pointers.
🛠️ Suggested fix (op.hpp)
- epoll_socket_impl* socket_impl_ = nullptr;
- epoll_acceptor_impl* acceptor_impl_ = nullptr;
+ std::atomic<epoll_socket_impl*> socket_impl_{nullptr};
+ std::atomic<epoll_acceptor_impl*> acceptor_impl_{nullptr};- socket_impl_ = nullptr;
- acceptor_impl_ = nullptr;
+ socket_impl_.store(nullptr, std::memory_order_release);
+ acceptor_impl_.store(nullptr, std::memory_order_release);- socket_impl_ = impl;
- acceptor_impl_ = nullptr;
+ socket_impl_.store(impl, std::memory_order_release);
+ acceptor_impl_.store(nullptr, std::memory_order_release);- socket_impl_ = nullptr;
- acceptor_impl_ = impl;
+ socket_impl_.store(nullptr, std::memory_order_release);
+ acceptor_impl_.store(impl, std::memory_order_release);Also adjust epoll_op::canceller::operator() in src/corosio/src/detail/epoll/sockets.cpp to load(std::memory_order_acquire) when reading these pointers.
Also applies to: 171-201
🤖 Prompt for AI Agents
In `@src/corosio/src/detail/epoll/op.hpp` around lines 113 - 116, Make the impl
pointer members epoll_socket_impl* socket_impl_ and epoll_acceptor_impl*
acceptor_impl_ atomic to avoid UB from concurrent stop_callback reads and
start() writes: change them to std::atomic<epoll_socket_impl*> and
std::atomic<epoll_acceptor_impl*> and ensure all assignments use store(...,
std::memory_order_release); then update epoll_op::canceller::operator() in
src/corosio/src/detail/epoll/sockets.cpp to read these pointers with
load(std::memory_order_acquire) (or capture the impl pointer into the canceller
at construction) so the canceller observes a consistent, race-free value; apply
the same atomicization to the other occurrences noted (lines 171-201).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 297-299: The workflow contains a no-op conditional `${{ matrix.x86
&& '' || '' }}`—replace it with a real conditional that installs 32-bit
dependencies when `matrix.x86` is true: add `dpkg --add-architecture i386` (or
equivalent step) before apt-get and append the i386 variants of needed packages
(e.g. `libc6:i386`, `libssl-dev:i386`, and any other runtime libs) to the
package list so the x86 matrix builds can install 32-bit libs; update the
package-list line (the one currently including `libssl-dev`) to include the
conditional `matrix.x86` variants instead of the empty expression.
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
394-429: MinGW builds use incompatible vcpkg triplet.Line 399 hardcodes
x64-windows(MSVC triplet), but the matrix includes a MinGW configuration. MinGW builds requirex64-mingw-dynamicorx64-mingw-statictriplet to get compatible binaries.
431-471: x86 builds use incompatible vcpkg triplet.The step hardcodes
x64-linuxtriplet, but the matrix includes an x86 build configuration. 32-bit builds requirex86-linuxtriplet (available as a vcpkg community triplet).
🧹 Nitpick comments (4)
.github/workflows/ci.yml (4)
38-40: Consider removing debug flags once CI stabilizes.These environment variables are helpful for diagnosing timeout issues, but verbose logging in CI increases log noise and may slow execution. Consider removing or making them conditional (e.g., only on failure reruns) after the TLS tests are stable.
550-555: Missing Windows WolfSSL paths (inconsistent with Boost CMake Workflow).Lines 553-554 only pass WolfSSL paths for Linux, while the "Boost CMake Workflow" (lines 514-517) passes them for both platforms. While Windows may work via the vcpkg toolchain, this inconsistency could cause issues if the toolchain's package discovery fails.
Suggested fix for consistency
extra-args: | -D BOOST_CI_INSTALL_TEST=ON -D CMAKE_PREFIX_PATH=${{ steps.patch.outputs.workspace_root }}/.local ${{ runner.os == 'Linux' && format('-D WolfSSL_INCLUDE_DIR={0}', steps.vcpkg-paths-linux.outputs.wolfssl_include) || '' }} ${{ runner.os == 'Linux' && format('-D WolfSSL_LIBRARY={0}', steps.vcpkg-paths-linux.outputs.wolfssl_library) || '' }} + ${{ runner.os == 'Windows' && format('-D WolfSSL_INCLUDE_DIR={0}', steps.vcpkg-paths-windows.outputs.wolfssl_include) || '' }} + ${{ runner.os == 'Windows' && format('-D WolfSSL_LIBRARY={0}', steps.vcpkg-paths-windows.outputs.wolfssl_library) || '' }}
576-580: Missing Windows WolfSSL paths (same inconsistency).Similar to the previous workflow, this only passes WolfSSL paths for Linux. Consider adding Windows paths for consistency.
Suggested fix
extra-args: | -D BOOST_CI_INSTALL_TEST=OFF ${{ runner.os == 'Linux' && format('-D WolfSSL_INCLUDE_DIR={0}', steps.vcpkg-paths-linux.outputs.wolfssl_include) || '' }} ${{ runner.os == 'Linux' && format('-D WolfSSL_LIBRARY={0}', steps.vcpkg-paths-linux.outputs.wolfssl_library) || '' }} + ${{ runner.os == 'Windows' && format('-D WolfSSL_INCLUDE_DIR={0}', steps.vcpkg-paths-windows.outputs.wolfssl_include) || '' }} + ${{ runner.os == 'Windows' && format('-D WolfSSL_LIBRARY={0}', steps.vcpkg-paths-windows.outputs.wolfssl_library) || '' }}
602-605: Missing Windows WolfSSL paths (same inconsistency).Same issue as the previous two workflows.
Suggested fix
extra-args: | ${{ runner.os == 'Linux' && format('-D WolfSSL_INCLUDE_DIR={0}', steps.vcpkg-paths-linux.outputs.wolfssl_include) || '' }} ${{ runner.os == 'Linux' && format('-D WolfSSL_LIBRARY={0}', steps.vcpkg-paths-linux.outputs.wolfssl_library) || '' }} + ${{ runner.os == 'Windows' && format('-D WolfSSL_INCLUDE_DIR={0}', steps.vcpkg-paths-windows.outputs.wolfssl_include) || '' }} + ${{ runner.os == 'Windows' && format('-D WolfSSL_LIBRARY={0}', steps.vcpkg-paths-windows.outputs.wolfssl_library) || '' }}
| libssl-dev | ||
| curl zip unzip tar pkg-config | ||
| ${{ matrix.x86 && '' || '' }} |
There was a problem hiding this comment.
Dead code and missing 32-bit library support.
Line 299 is a no-op (${{ matrix.x86 && '' || '' }} always evaluates to empty). For the x86 build matrix entry, you likely need 32-bit versions of dependencies:
Suggested fix
apt-get: >-
${{ matrix.install }}
build-essential
libssl-dev
curl zip unzip tar pkg-config
- ${{ matrix.x86 && '' || '' }}
+ ${{ matrix.x86 && 'libssl-dev:i386' || '' }}🤖 Prompt for AI Agents
In @.github/workflows/ci.yml around lines 297 - 299, The workflow contains a
no-op conditional `${{ matrix.x86 && '' || '' }}`—replace it with a real
conditional that installs 32-bit dependencies when `matrix.x86` is true: add
`dpkg --add-architecture i386` (or equivalent step) before apt-get and append
the i386 variants of needed packages (e.g. `libc6:i386`, `libssl-dev:i386`, and
any other runtime libs) to the package list so the x86 matrix builds can install
32-bit libs; update the package-list line (the one currently including
`libssl-dev`) to include the conditional `matrix.x86` variants instead of the
empty expression.
68580b2 to
32c7bc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/epoll/sockets.cpp`:
- Around line 324-347: The unregister_fd call can be invoked with fd_ == -1
(race between cancel_single_op and close_socket), causing ::epoll_ctl(epoll_fd_,
EPOLL_CTL_DEL, -1, nullptr) and a silent EBADF; update the
scheduler::unregister_fd (the implementation called by
svc_.scheduler().unregister_fd in cancel_single_op) to validate the fd before
calling epoll_ctl (e.g. return early if fd < 0) or check epoll_ctl's return and
handle EBADF explicitly so unregister_fd(fd_) never invokes epoll_ctl with a
negative fd and does not silently ignore errors during shutdown.
♻️ Duplicate comments (8)
src/wolfssl/src/wolfssl_stream.cpp (2)
111-128: Fix undefined identifierfatal_return; use proper WolfSSL constants.The callback uses undefined identifier
fatal_returnon line 124, which will cause a compile error. Replace with the documented WolfSSL constantSSL_TLSEXT_ERR_ALERT_FATAL(value 2). Also replace bare0withSSL_TLSEXT_ERR_OKfor consistency with the OpenSSL-compat SNI callback contract.Recommended fix
static int wolfssl_sni_callback( WOLFSSL* ssl, int* /* alert */, void* arg ) { char const* servername = wolfSSL_get_servername( ssl, WOLFSSL_SNI_HOST_NAME ); if( !servername ) - return 0; // No SNI sent, continue + return SSL_TLSEXT_ERR_OK; // No SNI sent, continue auto* cd = static_cast<context_data const*>( arg ); if( cd && cd->servername_callback ) { if( !cd->servername_callback( servername ) ) - return fatal_return; // Callback rejected hostname + return SSL_TLSEXT_ERR_ALERT_FATAL; // Callback rejected hostname } - return 0; // Accept + return SSL_TLSEXT_ERR_OK; // Accept }
1039-1049: Add error handling for SNI and hostname verification function calls.
wolfSSL_UseSNI()returnsWOLFSSL_SUCCESSon success and specific error codes on failure.wolfSSL_check_domain_name()returnsSSL_SUCCESSon success andSSL_FAILUREon memory allocation failure. Both calls can fail silently, allowing the session to proceed without SNI or hostname verification—a security regression.Suggested fix
// Apply per-session config (SNI + hostname verification) from context if( type == wolfssl_stream::client && !impl.hostname.empty() ) { // Set SNI extension so server knows which cert to present - wolfSSL_UseSNI( ssl_, WOLFSSL_SNI_HOST_NAME, + if( wolfSSL_UseSNI( ssl_, WOLFSSL_SNI_HOST_NAME, impl.hostname.data(), - static_cast<unsigned short>( impl.hostname.size() ) ); + static_cast<unsigned short>( impl.hostname.size() ) ) != WOLFSSL_SUCCESS ) + return system::error_code( + wolfSSL_get_error( ssl_, 0 ), + system::system_category() ); // Enable hostname verification (checks CN/SAN in peer cert) - wolfSSL_check_domain_name( ssl_, impl.hostname.c_str() ); + if( wolfSSL_check_domain_name( ssl_, impl.hostname.c_str() ) != WOLFSSL_SUCCESS ) + return system::error_code( + wolfSSL_get_error( ssl_, 0 ), + system::system_category() ); }src/openssl/src/openssl_stream.cpp (3)
155-165: Guard againstSSL_CTX_get_ex_new_indexfailure.
SSL_CTX_get_ex_new_index()returns-1on failure. Using-1as an index in the subsequentSSL_CTX_set_ex_data()call is unsafe. The guard should check after assignment.Suggested fix
// Initialize ex_data index for SNI callback (once) if( sni_ctx_data_index < 0 ) + { sni_ctx_data_index = SSL_CTX_get_ex_new_index( 0, nullptr, nullptr, nullptr, nullptr ); + if( sni_ctx_data_index < 0 ) + { + SSL_CTX_free( ctx_ ); + ctx_ = nullptr; + return; + } + }
220-227: CheckSSL_CTX_add_extra_chain_certreturn and free on failure.If
SSL_CTX_add_extra_chain_certfails (returns 0), ownership is not transferred and the caller must free the certificate. The current code ignores the return value, leaking the certificate on failure.Suggested fix
while( ( cert = PEM_read_bio_X509( bio, nullptr, nullptr, nullptr ) ) != nullptr ) { - // SSL_CTX_add_extra_chain_cert takes ownership - don't free - SSL_CTX_add_extra_chain_cert( ctx_, cert ); + // SSL_CTX_add_extra_chain_cert takes ownership on success + if( SSL_CTX_add_extra_chain_cert( ctx_, cert ) != 1 ) + { + X509_free( cert ); + break; + } }
870-879: Check return values for SNI and hostname verification functions.
SSL_set_tlsext_host_nameandSSL_set1_hostreturn 0 on failure. Both are called without checking return values, which is inconsistent with the error handling pattern in the rest ofinit_ssl()and creates a silent failure mode.Suggested fix
if( !impl.hostname.empty() ) { // Set SNI extension so server knows which cert to present - SSL_set_tlsext_host_name( ssl_, impl.hostname.c_str() ); + if( SSL_set_tlsext_host_name( ssl_, impl.hostname.c_str() ) != 1 ) + return system::error_code( + static_cast<int>( ERR_get_error() ), + system::system_category() ); // Enable hostname verification (checks CN/SAN in peer cert) - SSL_set1_host( ssl_, impl.hostname.c_str() ); + if( SSL_set1_host( ssl_, impl.hostname.c_str() ) != 1 ) + return system::error_code( + static_cast<int>( ERR_get_error() ), + system::system_category() ); }.github/workflows/ci.yml (2)
296-298: Dead code in package list.Line 298
${{ matrix.x86 && '' || '' }}always evaluates to empty string. For x86 builds, consider installing 32-bit library variants (e.g.,libssl-dev:i386).
441-481: Hardcoded x64-linux triplet may break x86 builds.The workflow has an x86 build configuration ("Clang 20: C++20-23 (x86)") but this step hardcodes
x64-linuxfor the vcpkg triplet. This will attempt to link 64-bit libraries into a 32-bit build. Consider deriving the triplet frommatrix.x86.src/corosio/src/detail/epoll/op.hpp (1)
113-116: Data race concern flagged in prior review: impl pointers accessed across threads.The
stop_callbackmay fire on a different thread than the one callingstart(). Whilestd::stop_callbackprovides synchronization between registration and invocation, making these pointersstd::atomicwould provide explicit guarantees and eliminate any ambiguity about cross-thread visibility.That said, the current code should be safe because
stop_callback's constructor synchronizes-with callback invocation, establishing happens-before from the writes instart()to the reads incanceller::operator(). Confirm this is the intended reliance onstop_callbacksemantics.
| void | ||
| epoll_socket_impl:: | ||
| cancel_single_op(epoll_op& op) noexcept | ||
| { | ||
| // Called from stop_token callback to cancel a specific pending operation. | ||
| // This performs actual I/O cancellation, not just setting a flag. | ||
| bool was_registered = op.registered.exchange(false, std::memory_order_acq_rel); | ||
| op.request_cancel(); | ||
|
|
||
| if (was_registered) | ||
| { | ||
| svc_.scheduler().unregister_fd(fd_); | ||
|
|
||
| // Keep impl alive until op completes | ||
| try { | ||
| op.impl_ptr = shared_from_this(); | ||
| } catch (const std::bad_weak_ptr&) { | ||
| // Impl is being destroyed, op will be orphaned but that's ok | ||
| } | ||
|
|
||
| svc_.post(&op); | ||
| svc_.work_finished(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the unregister_fd implementation
rg -n 'unregister_fd' ./src/corosio/src/detail/epoll/scheduler.cpp -A 15 -B 2Repository: cppalliance/corosio
Length of output: 955
🏁 Script executed:
# Find close_socket implementation in the sockets.cpp file
rg -n 'close_socket' ./src/corosio/src/detail/epoll/sockets.cpp -A 20 -B 2Repository: cppalliance/corosio
Length of output: 3957
🏁 Script executed:
# Check cancel_single_op calls and stop_token coordination
rg -n 'cancel_single_op\|stop_token' ./src/corosio/src/detail/epoll/sockets.cpp -B 3 -A 3Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Search for the stop_token callback setup in operations
rg -n 'cancel_single_op' ./src/corosio/src/detail/epoll/sockets.cpp -B 5 -A 15Repository: cppalliance/corosio
Length of output: 2246
Fix unregister_fd() to handle invalid file descriptors gracefully.
The race condition described is valid. When cancel_single_op() is invoked from a stop_token callback after close_socket() has already set fd_ = -1, the call to unregister_fd(fd_) will invoke ::epoll_ctl(epoll_fd_, EPOLL_CTL_DEL, -1, nullptr). The current implementation silently ignores the resulting EBADF error without validation or explicit handling.
Add a check in unregister_fd() to validate that fd >= 0 before calling epoll_ctl, or verify and handle the return value to prevent silent failures during shutdown.
🤖 Prompt for AI Agents
In `@src/corosio/src/detail/epoll/sockets.cpp` around lines 324 - 347, The
unregister_fd call can be invoked with fd_ == -1 (race between cancel_single_op
and close_socket), causing ::epoll_ctl(epoll_fd_, EPOLL_CTL_DEL, -1, nullptr)
and a silent EBADF; update the scheduler::unregister_fd (the implementation
called by svc_.scheduler().unregister_fd in cancel_single_op) to validate the fd
before calling epoll_ctl (e.g. return early if fd < 0) or check epoll_ctl's
return and handle EBADF explicitly so unregister_fd(fd_) never invokes epoll_ctl
with a negative fd and does not silently ignore errors during shutdown.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 560-562: The workflow only injects WolfSSL CMake variables for
Linux using steps.vcpkg-paths-linux.outputs.wolfssl_include and wolfssl_library,
so add equivalent conditional expressions for Windows builds that reference
steps.vcpkg-paths-windows.outputs.wolfssl_include and
steps.vcpkg-paths-wolfssl_library (matching the pattern used for Boost) so the
CMake invocation (toolchain: CMAKE_TOOLCHAIN_FILE) receives WolfSSL include and
library paths on Windows as well as Linux.
- Around line 583-588: The YAML extra-args block for the Subdirectory
Integration step only injects WolfSSL include/library paths for Linux; add
conditional branches for Windows similar to the Find Package Integration step by
including ${ runner.os == 'Windows' && format('-D WolfSSL_INCLUDE_DIR={0}',
steps.vcpkg-paths-windows.outputs.wolfssl_include) || '' } and ${ runner.os ==
'Windows' && format('-D WolfSSL_LIBRARY={0}',
steps.vcpkg-paths-windows.outputs.wolfssl_library) || '' } alongside the
existing Linux conditions so that the extra-args line passes WolfSSL paths on
Windows as well.
- Around line 609-612: The CI extra-args block only injects Linux WolfSSL CMake
variables (format('-D WolfSSL_INCLUDE_DIR={0}' and format('-D
WolfSSL_LIBRARY={0}') using steps.vcpkg-paths-linux.outputs.*), so add
equivalent conditional extra-args for Windows (using runner.os == 'Windows')
referencing the Windows vcpkg step outputs (e.g.,
steps.vcpkg-paths-windows.outputs.wolfssl_include and
steps.vcpkg-paths-windows.outputs.wolfssl_library) alongside the existing Linux
condition so CMake receives WolfSSL include/library paths on Windows as well;
ensure these follow the same formatting pattern and coexist with toolchain: ${{
env.CMAKE_TOOLCHAIN_FILE }}.
In `@src/openssl/src/openssl_stream.cpp`:
- Around line 569-573: The local variable `iteration` in openssl_stream.cpp is
unused; remove its declaration "int iteration = 0;" and the increment
"++iteration;" inside the loop in the function containing the
while(!token.stop_requested()) so the loop only checks token.stop_requested()
without the unused counter; verify there are no other references to `iteration`
before committing.
♻️ Duplicate comments (6)
.github/workflows/ci.yml (3)
293-295: Dead code on line 295 still present.The expression
${{ matrix.x86 && '' || '' }}always evaluates to empty string. For x86 builds, 32-bit dependencies likelibssl-dev:i386should be installed.
399-406: MinGW builds will use incompatible MSVC binaries.The code hardcodes
x64-windowstriplet, but the matrix includes MinGW which requiresx64-mingw-dynamicorx64-mingw-static. MSVC-built libraries are ABI-incompatible with MinGW.
447-454: x86 builds will link against 64-bit libraries.The code hardcodes
x64-linuxtriplet, but the matrix includes an x86 build (Clang 20: C++20-23 (x86)withx86: true) which requiresx86-linuxtriplet.src/openssl/src/openssl_stream.cpp (3)
133-143: Guard againstSSL_CTX_get_ex_new_indexfailure.As previously noted,
SSL_CTX_get_ex_new_index()returns-1on failure. The code should check this before using the index inSSL_CTX_set_ex_data().
199-204: CheckSSL_CTX_add_extra_chain_certreturn and free on failure.As previously noted, if
SSL_CTX_add_extra_chain_certreturns 0, ownership is not transferred and the certificate must be freed to avoid a leak.
816-825: Check return values for SNI and hostname verification functions.As previously noted,
SSL_set_tlsext_host_nameandSSL_set1_hostreturn 0 on failure. The current code ignores return values, creating a silent failure mode for SNI and hostname verification setup.
| int iteration = 0; | ||
|
|
||
| while(!token.stop_requested()) | ||
| { | ||
| ++iteration; |
There was a problem hiding this comment.
Remove unused iteration variable.
The iteration variable is incremented but never used. This appears to be leftover debug code.
Suggested fix
{
system::error_code ec;
- int iteration = 0;
while(!token.stop_requested())
{
- ++iteration;
ERR_clear_error();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int iteration = 0; | |
| while(!token.stop_requested()) | |
| { | |
| ++iteration; | |
| while(!token.stop_requested()) | |
| { | |
| ERR_clear_error(); |
🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 569 - 573, The local
variable `iteration` in openssl_stream.cpp is unused; remove its declaration
"int iteration = 0;" and the increment "++iteration;" inside the loop in the
function containing the while(!token.stop_requested()) so the loop only checks
token.stop_requested() without the unused counter; verify there are no other
references to `iteration` before committing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 506-507: Add a TODO comment directly above the conditional "if:
${{ !matrix.coverage && runner.os != 'Windows' }}" in the CI workflow explaining
this is a temporary skip for Windows B2, include a tracking ticket/issue ID (or
create one) and explicit re-enable criteria (e.g., "re-enable when CMake Windows
build passes on CI or after TASK-1234 is resolved"), and ensure the comment
includes an expected review/expiry date or condition so the skip is not left
open-ended.
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
286-296: x86 Linux builds still don’t install 32‑bit OpenSSL dev libs.The conditional package entry is still a no‑op, so the x86 job will keep pulling only 64‑bit
libssl-dev. Add the i386 variant (and any other 32‑bit deps you need) whenmatrix.x86is true.On Ubuntu 24.04/25.04, is “libssl-dev:i386” the correct 32‑bit OpenSSL dev package name?🛠️ Proposed fix
build-essential libssl-dev curl zip unzip tar pkg-config - ${{ matrix.x86 && '' || '' }} + ${{ matrix.x86 && 'libssl-dev:i386' || '' }}
463-503: Linux vcpkg paths are hardcoded to x64-linux (x86 job will pick wrong libs).The x86 build will still resolve 64‑bit vcpkg artifacts. Please derive the triplet from
VCPKG_DEFAULT_TRIPLET(and set it for the x86 matrix entry, e.g.,x86-linux) so the path discovery follows the active triplet.Does vcpkg provide an “x86-linux” triplet (community or built‑in) compatible with the pinned vcpkg commit?🛠️ Proposed fix (use active triplet)
- if [ -n "${VCPKG_INSTALLED_DIR}" ] && [ -d "${VCPKG_INSTALLED_DIR}/x64-linux" ]; then - vcpkg_installed="${VCPKG_INSTALLED_DIR}/x64-linux" + triplet="${VCPKG_DEFAULT_TRIPLET:-x64-linux}" + if [ -n "${VCPKG_INSTALLED_DIR}" ] && [ -d "${VCPKG_INSTALLED_DIR}/${triplet}" ]; then + vcpkg_installed="${VCPKG_INSTALLED_DIR}/${triplet}" else # Fallback: try to find it - vcpkg_installed=$(find "${GITHUB_WORKSPACE}" -type d -path "*/vcpkg_installed/x64-linux" 2>/dev/null | head -1) + vcpkg_installed=$(find "${GITHUB_WORKSPACE}" -type d -path "*/vcpkg_installed/${triplet}" 2>/dev/null | head -1) if [ -z "${vcpkg_installed}" ]; then - vcpkg_installed=$(find "/__w" -type d -path "*/vcpkg_installed/x64-linux" 2>/dev/null | head -1) + vcpkg_installed=$(find "/__w" -type d -path "*/vcpkg_installed/${triplet}" 2>/dev/null | head -1) fi fi
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@CMakeLists.txt`:
- Around line 208-211: The target_link_libraries call currently links crypt32 as
INTERFACE which doesn't ensure the boost_corosio_wolfssl target itself is linked
to crypt32; change the linkage scope so that crypt32 is linked PUBLIC to
boost_corosio_wolfssl (update the target_link_libraries invocation for target
boost_corosio_wolfssl to use PUBLIC and keep the WIN32 conditional intact) so
both the library and its consumers get the dependency.
- Around line 234-237: The CMake snippet uses INTERFACE for both ws2_32 and
crypt32 which prevents crypt32 from being linked into boost_corosio_openssl when
building shared libs; update the target_link_libraries call for target
boost_corosio_openssl so that crypt32 is linked with PUBLIC (e.g., keep/ws2_32
as INTERFACE if desired but change crypt32 to PUBLIC) within the WIN32 block in
CMakeLists.txt to ensure the library itself gets crypt32 at link time.
♻️ Duplicate comments (3)
.github/workflows/ci.yml (3)
290-296: 32‑bit Linux still installs only amd64 OpenSSL dev libsFor the x86 matrix entry,
apt-get-add-architecture: i386is set but the package list doesn’t include i386 variants, so 32‑bit builds can still fail to link against OpenSSL.🛠️ Suggested adjustment
apt-get: >- ${{ matrix.install }} build-essential libssl-dev curl zip unzip tar pkg-config - ${{ matrix.x86 && '' || '' }} + ${{ matrix.x86 && 'libssl-dev:i386' || '' }}Ubuntu 22.04/24.04/25.04: Is `libssl-dev:i386` available and sufficient for 32‑bit builds, or are additional i386 packages required?
383-388: Linux vcpkg triplet is still hardcoded to x64The vcpkg install/lookup logic uses
x64-linux, which will misconfigure the x86 Linux build in the matrix. Ensure the triplet is set tox86-linuxfor the 32‑bit job and reused consistently for install and lookup.🛠️ Suggested adjustment
- - name: Set vcpkg triplet - if: matrix.vcpkg-triplet + - name: Set vcpkg triplet + if: matrix.vcpkg-triplet || matrix.x86 shell: bash run: | - echo "VCPKG_DEFAULT_TRIPLET=${{ matrix.vcpkg-triplet }}" >> $GITHUB_ENV + if [[ "${{ matrix.x86 }}" == "true" ]]; then + echo "VCPKG_DEFAULT_TRIPLET=x86-linux" >> $GITHUB_ENV + else + echo "VCPKG_DEFAULT_TRIPLET=${{ matrix.vcpkg-triplet }}" >> $GITHUB_ENV + fi- if [ -n "${VCPKG_INSTALLED_DIR}" ] && [ -d "${VCPKG_INSTALLED_DIR}/x64-linux" ]; then - vcpkg_installed="${VCPKG_INSTALLED_DIR}/x64-linux" + triplet="${{ matrix.x86 && 'x86-linux' || 'x64-linux' }}" + if [ -n "${VCPKG_INSTALLED_DIR}" ] && [ -d "${VCPKG_INSTALLED_DIR}/${triplet}" ]; then + vcpkg_installed="${VCPKG_INSTALLED_DIR}/${triplet}" else - vcpkg_installed=$(find "${GITHUB_WORKSPACE}" -type d -path "*/vcpkg_installed/x64-linux" 2>/dev/null | head -1) + vcpkg_installed=$(find "${GITHUB_WORKSPACE}" -type d -path "*/vcpkg_installed/${triplet}" 2>/dev/null | head -1)What is the correct vcpkg triplet name for 32‑bit Linux, and how should it be enabled in CI?Also applies to: 474-480
506-509: Temporary Windows B2 skip needs a tracking hookPlease add a TODO with a ticket/criteria to re-enable Windows B2 runs.
🛠️ Suggested adjustment
- # TEMP: Skip B2 on Windows to test if CMake builds pass + # TODO(ISSUE-XXXX): Re-enable Windows B2 when CMake builds are stable.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CMakeLists.txt`:
- Around line 67-70: The fallback find_package call doesn't request filesystem
or asio components when BOOST_COROSIO_BUILD_TESTS is true, which leads to
incorrect aliasing of compiled libs to Boost::headers; update the CMake logic so
that when BOOST_COROSIO_BUILD_TESTS is enabled you add "filesystem" and "asio"
to the list of Boost components passed into find_package(Boost REQUIRED
COMPONENTS ...) (the variable used to build that components list, e.g.
BOOST_COROSIO_INCLUDE_LIBRARIES), and remove/adjust the fallback aliasing that
maps Boost::filesystem/Boost::asio to Boost::headers so that Boost::filesystem
and Boost::asio are discovered as proper linkable targets and can be linked
normally.
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
294-296: x86 package conditional is still a no‑opLine 296 still resolves to an empty string, so the x86 build only installs 64‑bit
libssl-dev. That can break 32‑bit linking. Consider adding the i386 variant.🛠️ Proposed fix
- ${{ matrix.x86 && '' || '' }} + ${{ matrix.x86 && 'libssl-dev:i386' || '' }}
481-521: Linux vcpkg path hardcodesx64-linux; breaks x86 buildsLine 490 assumes
x64-linux. For thematrix.x86build (Line 255), this will pick 64‑bit WolfSSL and can cause linker failures. Please select the triplet dynamically.🛠️ Proposed fix
- if [ -n "${VCPKG_INSTALLED_DIR}" ] && [ -d "${VCPKG_INSTALLED_DIR}/x64-linux" ]; then - vcpkg_installed="${VCPKG_INSTALLED_DIR}/x64-linux" + triplet="${{ matrix.x86 && 'x86-linux' || 'x64-linux' }}" + if [ -n "${VCPKG_INSTALLED_DIR}" ] && [ -d "${VCPKG_INSTALLED_DIR}/${triplet}" ]; then + vcpkg_installed="${VCPKG_INSTALLED_DIR}/${triplet}" else - vcpkg_installed=$(find "${GITHUB_WORKSPACE}" -type d -path "*/vcpkg_installed/x64-linux" 2>/dev/null | head -1) + vcpkg_installed=$(find "${GITHUB_WORKSPACE}" -type d -path "*/vcpkg_installed/${triplet}" 2>/dev/null | head -1) if [ -z "${vcpkg_installed}" ]; then - vcpkg_installed=$(find "/__w" -type d -path "*/vcpkg_installed/x64-linux" 2>/dev/null | head -1) + vcpkg_installed=$(find "/__w" -type d -path "*/vcpkg_installed/${triplet}" 2>/dev/null | head -1) fi fivcpkg triplet x86-linux availability
| # Include asio and filesystem which are needed by corosio's tests | ||
| if (BOOST_COROSIO_BUILD_TESTS) | ||
| list(APPEND BOOST_COROSIO_INCLUDE_LIBRARIES asio filesystem) | ||
| endif () |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the CMakeLists.txt file
find . -name "CMakeLists.txt" -type f | head -5Repository: cppalliance/corosio
Length of output: 228
🏁 Script executed:
# Read the CMakeLists.txt file with line numbers
cat -n CMakeLists.txt | head -250Repository: cppalliance/corosio
Length of output: 12110
🌐 Web query:
Boost CMake find_package asio filesystem components official documentation
💡 Result:
CMake find_package(Boost ...) for Asio and Filesystem (official docs)
Boost.Filesystem (compiled library)
Use a Boost component and link the imported target:
find_package(Boost REQUIRED COMPONENTS filesystem)
target_link_libraries(my_target PRIVATE Boost::filesystem)CMake provides Boost::<component> imported targets, and it also auto-adds implicit deps (e.g., Boost::filesystem → Boost::system) when using these targets. [1]
Boost.Asio (header-only by default)
Asio is header-only unless you opt into separate compilation. [2] So you typically don’t request an asio component; just use Boost headers:
find_package(Boost REQUIRED) # or: find_package(Boost REQUIRED COMPONENTS headers)
target_link_libraries(my_target PRIVATE Boost::headers) # alias of Boost::boostIf your code (or other Boost libs you use) requires Boost.System: Boost.System has been header-only since Boost 1.69 (a compatibility “stub” library may still exist). [3]
Primary references
- CMake
FindBoostmodule (components +Boost::...targets, implicit deps): [1] - Boost.Asio “Using” docs (header-only by default; optional separate compilation): [2]
- Boost Getting Started (Boost.System header-only since 1.69): [3]
Sources: [1] https://cmake.org/cmake/help/latest/module/FindBoost.html | [2] https://www.boost.org/doc/libs/latest/doc/html/boost_asio/using.html | [3] https://www.boost.org/doc/libs/boost_1_81_0/more/getting_started/unix-variants.html
Filesystem and Asio must be included in find_package when tests are enabled
Lines 68-70 add asio and filesystem to BOOST_COROSIO_INCLUDE_LIBRARIES when tests are enabled, but the fallback find_package(Boost REQUIRED COMPONENTS capy url system) at line 111 does not request these components. The subsequent fallback mechanism (lines 112-116) aliases missing targets to Boost::headers, which is incorrect for Boost::filesystem—a compiled library that requires proper linkage separate from headers alone.
Proposed fix
+ set(BOOST_COROSIO_FIND_COMPONENTS capy url system)
+ if (BOOST_COROSIO_BUILD_TESTS)
+ list(APPEND BOOST_COROSIO_FIND_COMPONENTS asio filesystem)
+ endif ()
- find_package(Boost REQUIRED COMPONENTS capy url system)
+ find_package(Boost REQUIRED COMPONENTS ${BOOST_COROSIO_FIND_COMPONENTS})🤖 Prompt for AI Agents
In `@CMakeLists.txt` around lines 67 - 70, The fallback find_package call doesn't
request filesystem or asio components when BOOST_COROSIO_BUILD_TESTS is true,
which leads to incorrect aliasing of compiled libs to Boost::headers; update the
CMake logic so that when BOOST_COROSIO_BUILD_TESTS is enabled you add
"filesystem" and "asio" to the list of Boost components passed into
find_package(Boost REQUIRED COMPONENTS ...) (the variable used to build that
components list, e.g. BOOST_COROSIO_INCLUDE_LIBRARIES), and remove/adjust the
fallback aliasing that maps Boost::filesystem/Boost::asio to Boost::headers so
that Boost::filesystem and Boost::asio are discovered as proper linkable targets
and can be linked normally.
68fc0ff to
1db177f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 522-526: The comment saying "TEMP: Skip B2 on Windows to test if
CMake builds pass" is stale and misleading relative to the Boost B2 Workflow
step named "Boost B2 Workflow" where the condition is currently if: ${{
!matrix.coverage }}; remove the stale comment lines (the two lines beginning
with "# TEMP: Skip B2 on Windows..." and "# if: ${{ !matrix.coverage &&
runner.os != 'Windows' }}") so the comment no longer contradicts the actual if
expression, or alternatively restore the intended behavior by changing the
condition to if: ${{ !matrix.coverage && runner.os != 'Windows' }} if you truly
intend to skip Windows.
- Around line 305-319: The workflow clones boostorg/asio and boostorg/filesystem
as steps named "Clone Asio" and "Clone Filesystem" but never patches them into
boost-root before the Patch Boost step that currently only injects corosio-root
and capy-root into boost-root/libs; update the Patch Boost step to also copy
asio-root into boost-root/libs/asio and filesystem-root into
boost-root/libs/filesystem (mirroring how capy-root is copied) so CMake can find
asio and filesystem, or alternatively remove the "Clone Asio"/"Clone Filesystem"
steps if you prefer not to include those libs.
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
294-296: Dead conditional expression remains.Line 296 contains
${{ matrix.x86 && '' || '' }}which always evaluates to an empty string. This was flagged in a previous review. For x86 builds, you may need 32-bit variants of system packages (e.g.,libssl-dev:i386), or this line should be removed to avoid confusion.
481-507: Linux x86 build uses hardcoded x64-linux triplet.The vcpkg path discovery hardcodes
x64-linuxtriplet, but the matrix includes an x86 build (Clang 20: C++20-23 (x86)withx86: true). This will cause the x86 build to use 64-bit libraries or fail to find the vcpkg directory.Consider adding triplet selection based on
matrix.x86similar to the Windows approach, or confirm that the x86 build intentionally skips vcpkg-provided libraries.
| # Test dependency of corosio | ||
| - name: Clone Asio | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: boostorg/asio | ||
| ref: ${{ (github.ref_name == 'master' && github.ref_name) || 'develop' }} | ||
| path: asio-root | ||
|
|
||
| # Test dependency of corosio | ||
| - name: Clone Filesystem | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: boostorg/filesystem | ||
| ref: ${{ (github.ref_name == 'master' && github.ref_name) || 'develop' }} | ||
| path: filesystem-root |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if asio-root or filesystem-root are referenced anywhere in the workflow or build files
echo "=== References to asio-root ==="
rg -n "asio-root" .github/workflows/ci.yml
echo ""
echo "=== References to filesystem-root ==="
rg -n "filesystem-root" .github/workflows/ci.yml
echo ""
echo "=== Check CMakeLists for Asio/Filesystem dependencies ==="
fd CMakeLists.txt -t f --exec grep -l -i "asio\|filesystem" {} \; 2>/dev/null | head -10Repository: cppalliance/corosio
Length of output: 275
🏁 Script executed:
cat -n CMakeLists.txtRepository: cppalliance/corosio
Length of output: 13445
🏁 Script executed:
sed -n '336,370p' .github/workflows/ci.yml | cat -nRepository: cppalliance/corosio
Length of output: 1467
Patch asio-root and filesystem-root into boost-root, or remove the unnecessary clones.
The repositories are cloned as "test dependencies" (lines 305-319) but are never copied into boost-root in the Patch Boost step. CMakeLists.txt (lines 68-70) expects asio and filesystem to be available in the Boost library tree when tests are built, and the Patch Boost step (lines 336-370) only patches corosio-root and capy-root into boost-root/libs/. Without patching asio-root and filesystem-root, test builds will fail to find these dependencies. Either copy them into boost-root/libs/asio and boost-root/libs/filesystem (like capy), or remove the unused clones.
🤖 Prompt for AI Agents
In @.github/workflows/ci.yml around lines 305 - 319, The workflow clones
boostorg/asio and boostorg/filesystem as steps named "Clone Asio" and "Clone
Filesystem" but never patches them into boost-root before the Patch Boost step
that currently only injects corosio-root and capy-root into boost-root/libs;
update the Patch Boost step to also copy asio-root into boost-root/libs/asio and
filesystem-root into boost-root/libs/filesystem (mirroring how capy-root is
copied) so CMake can find asio and filesystem, or alternatively remove the
"Clone Asio"/"Clone Filesystem" steps if you prefer not to include those libs.
2fa09bf to
33fdb66
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/iocp/sockets.cpp`:
- Around line 410-420: The unconditional call to svc_.work_finished() can
underflow if IOCP wins the race; change logic so svc_.work_finished() is only
called when the CAS wins (i.e., when ::InterlockedCompareExchange(&op.ready_, 1,
0) returns 0), moving svc_.work_finished() inside that success branch alongside
setting op.dwError and svc_.post(&op); apply the same pattern to the other
synchronous-completion branches so work_finished() is only decremented when this
code path actually handles the completion (referencing op.ready_,
::InterlockedCompareExchange, svc_.work_finished(), svc_.post(&op), and
on_completion()).
♻️ Duplicate comments (8)
src/corosio/src/detail/iocp/sockets.cpp (3)
491-502: Same double-work_finished()risk as the connect sync pathApply the same CAS‑guarded
work_finished()fix here as in the Line 410‑420 comment.
570-578: Same double-work_finished()risk as the connect sync pathApply the same CAS‑guarded
work_finished()fix here as in the Line 410‑420 comment.
1054-1061: Same double-work_finished()risk as the connect sync pathApply the same CAS‑guarded
work_finished()fix here as in the Line 410‑420 comment.CMakeLists.txt (1)
67-70: Ensure Boostfind_packageincludes asio/filesystem when tests are enabled.You append
asio/filesystemtoBOOST_COROSIO_INCLUDE_LIBRARIES, but the fallbackfind_package(Boost ...)still only requestscapy url system. When Boost is sourced from a package (not the monorepo), this can aliasBoost::filesystemtoBoost::headers, which is incorrect for a compiled library and can break linking.🛠️ Suggested fix
- find_package(Boost REQUIRED COMPONENTS capy url system) + set(BOOST_COROSIO_FIND_COMPONENTS capy url system) + if (BOOST_COROSIO_BUILD_TESTS) + list(APPEND BOOST_COROSIO_FIND_COMPONENTS asio filesystem) + endif () + find_package(Boost REQUIRED COMPONENTS ${BOOST_COROSIO_FIND_COMPONENTS})#!/bin/bash # Verify Boost components requested vs. test-only dependencies rg -n "BOOST_COROSIO_BUILD_TESTS|BOOST_COROSIO_INCLUDE_LIBRARIES|find_package\\(Boost" CMakeLists.txt.github/workflows/ci.yml (4)
294-296: Install 32‑bit SSL dev package for x86 builds.The conditional
${{ matrix.x86 && '' || '' }}is a no‑op, so 32‑bit builds don’t get the i386 SSL headers/libs. That can break the x86 job once TLS is built/linked.🛠️ Suggested fix
- ${{ matrix.x86 && '' || '' }} + ${{ matrix.x86 && 'libssl-dev:i386' || '' }}#!/bin/bash # Show x86 matrix entry + install list rg -n "x86: true|apt-get-add-architecture|libssl-dev" .github/workflows/ci.yml
305-319: Patch cloned Asio/Filesystem into boost-root (or drop the clones).You clone Asio and Filesystem but never copy them into
boost-root/libs/. If tests expect these libs inside the Boost tree, builds will fail or the clones become dead weight.🛠️ Suggested fix (patch into boost-root)
# Patch boost-root with capy dependency cp -r "$workspace_root"/capy-root "libs/capy" + + # Patch boost-root with test dependencies + cp -r "$workspace_root"/asio-root "libs/asio" + cp -r "$workspace_root"/filesystem-root "libs/filesystem"#!/bin/bash # Verify clones exist and whether patch step copies them rg -n "Clone Asio|Clone Filesystem|Patch boost-root" -n .github/workflows/ci.yml
481-521: Use an x86 vcpkg triplet for 32‑bit Linux builds.The x86 Linux job still uses
x64-linuxin the vcpkg path discovery, which will pick 64‑bit libs and can cause link failures. Use a triplet derived frommatrix.x86and reference it everywhere you computevcpkg_installed.🛠️ Suggested fix (triplet selection)
+ - name: Select vcpkg triplet (Linux) + if: runner.os == 'Linux' + shell: bash + run: | + if [[ "${{ matrix.x86 }}" == "true" ]]; then + echo "VCPKG_DEFAULT_TRIPLET=x86-linux" >> $GITHUB_ENV + else + echo "VCPKG_DEFAULT_TRIPLET=x64-linux" >> $GITHUB_ENV + fi - name: Set vcpkg paths (Linux) if: runner.os == 'Linux' id: vcpkg-paths-linux shell: bash run: | - if [ -n "${VCPKG_INSTALLED_DIR}" ] && [ -d "${VCPKG_INSTALLED_DIR}/x64-linux" ]; then - vcpkg_installed="${VCPKG_INSTALLED_DIR}/x64-linux" + triplet="${VCPKG_DEFAULT_TRIPLET:-x64-linux}" + if [ -n "${VCPKG_INSTALLED_DIR}" ] && [ -d "${VCPKG_INSTALLED_DIR}/${triplet}" ]; then + vcpkg_installed="${VCPKG_INSTALLED_DIR}/${triplet}" else - vcpkg_installed=$(find "${GITHUB_WORKSPACE}" -type d -path "*/vcpkg_installed/x64-linux" 2>/dev/null | head -1) + vcpkg_installed=$(find "${GITHUB_WORKSPACE}" -type d -path "*/vcpkg_installed/${triplet}" 2>/dev/null | head -1) if [ -z "${vcpkg_installed}" ]; then - vcpkg_installed=$(find "/__w" -type d -path "*/vcpkg_installed/x64-linux" 2>/dev/null | head -1) + vcpkg_installed=$(find "/__w" -type d -path "*/vcpkg_installed/${triplet}" 2>/dev/null | head -1) fi fi#!/bin/bash # Confirm x86 matrix entry and hardcoded x64-linux in vcpkg paths rg -n "x86: true|vcpkg_installed/x64-linux|VCPKG_DEFAULT_TRIPLET" .github/workflows/ci.yml
524-525: Remove or align the temporary Windows B2 skip comment.The comment indicates Windows is skipped, but the condition does not. Either delete the comment or update the condition to match the intended behavior.
🧹 Suggested fix (remove stale comment)
- # TEMP: Skip B2 on Windows to test if CMake builds pass - # if: ${{ !matrix.coverage && runner.os != 'Windows' }} if: ${{ !matrix.coverage }}
| // Synchronous completion - with FILE_SKIP_COMPLETION_PORT_ON_SUCCESS, | ||
| // IOCP shouldn't post a packet. But if the flag failed to set or under | ||
| // certain conditions, IOCP might still deliver a completion. Use CAS | ||
| // to race with IOCP: only set fields and post if we win (CAS returns 0). | ||
| // If IOCP wins, it already set the fields via complete() and processed. | ||
| svc_.work_finished(); | ||
| op.dwError = 0; | ||
| svc_.post(&op); | ||
| if (::InterlockedCompareExchange(&op.ready_, 1, 0) == 0) | ||
| { | ||
| op.dwError = 0; | ||
| svc_.post(&op); | ||
| } |
There was a problem hiding this comment.
Guard work_finished() with the CAS win to prevent double decrement
Line 415 currently calls work_finished() unconditionally. If IOCP still posts a completion and wins the race, on_completion() will call work_finished() again, which can underflow the outstanding-work counter and end the run loop early. Move work_finished() inside the CAS‑success path (and mirror this pattern in the other sync‑completion branches).
🛠️ Proposed fix (connect sync path)
- svc_.work_finished();
- if (::InterlockedCompareExchange(&op.ready_, 1, 0) == 0)
- {
- op.dwError = 0;
- svc_.post(&op);
- }
+ if (::InterlockedCompareExchange(&op.ready_, 1, 0) == 0)
+ {
+ svc_.work_finished();
+ op.dwError = 0;
+ svc_.post(&op);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Synchronous completion - with FILE_SKIP_COMPLETION_PORT_ON_SUCCESS, | |
| // IOCP shouldn't post a packet. But if the flag failed to set or under | |
| // certain conditions, IOCP might still deliver a completion. Use CAS | |
| // to race with IOCP: only set fields and post if we win (CAS returns 0). | |
| // If IOCP wins, it already set the fields via complete() and processed. | |
| svc_.work_finished(); | |
| op.dwError = 0; | |
| svc_.post(&op); | |
| if (::InterlockedCompareExchange(&op.ready_, 1, 0) == 0) | |
| { | |
| op.dwError = 0; | |
| svc_.post(&op); | |
| } | |
| // Synchronous completion - with FILE_SKIP_COMPLETION_PORT_ON_SUCCESS, | |
| // IOCP shouldn't post a packet. But if the flag failed to set or under | |
| // certain conditions, IOCP might still deliver a completion. Use CAS | |
| // to race with IOCP: only set fields and post if we win (CAS returns 0). | |
| // If IOCP wins, it already set the fields via complete() and processed. | |
| if (::InterlockedCompareExchange(&op.ready_, 1, 0) == 0) | |
| { | |
| svc_.work_finished(); | |
| op.dwError = 0; | |
| svc_.post(&op); | |
| } |
🤖 Prompt for AI Agents
In `@src/corosio/src/detail/iocp/sockets.cpp` around lines 410 - 420, The
unconditional call to svc_.work_finished() can underflow if IOCP wins the race;
change logic so svc_.work_finished() is only called when the CAS wins (i.e.,
when ::InterlockedCompareExchange(&op.ready_, 1, 0) returns 0), moving
svc_.work_finished() inside that success branch alongside setting op.dwError and
svc_.post(&op); apply the same pattern to the other synchronous-completion
branches so work_finished() is only decremented when this code path actually
handles the completion (referencing op.ready_, ::InterlockedCompareExchange,
svc_.work_finished(), svc_.post(&op), and on_completion()).
…on tests API additions: - Add context::set_servername_callback() for server-side SNI handling - Server can now accept/reject connections based on client's requested hostname Bug fixes: - Fix WolfSSL shutdown: remove double wolfSSL_get_error() call that consumed error state, add socket read after flushing close_notify to match OpenSSL's bidirectional shutdown semantics - Fix stop token propagation in both OpenSSL and WolfSSL stream implementations New tests: - testStopTokenCancellation: cancel during handshake, read, and write operations - testSocketErrorPropagation: socket.cancel() and connection reset handling - testCertificateValidation: untrusted CA and expired certificate rejection - testSni: hostname verification with correct and incorrect hostnames - testSniCallback: server-side SNI callback accepting/rejecting connections - testMtls: mutual TLS success, missing client cert, and wrong CA scenarios - testCertificateChain: intermediate certificate chain handling Test infrastructure: - Add embedded test certificates for chain, mTLS, and expired cert scenarios - Add context factory functions for various test configurations - Add deterministic test helpers using socket I/O synchronization - Failsafe timeouts now report test failures via BOOST_TEST(!failsafe_hit) - Replace timer-based delays with socket read synchronization for determinism - Re-enable testFailureCases (was disabled due to cancellation issues) Test count increased from ~100 to 798 assertions.
The previous implementation used a non-atomic counter with a narrow port range (100 ports), causing Address already in use errors when: - Tests run in parallel (ctest -j) - Ports remain in TCP TIME_WAIT from previous test runs Changes: - Use std::atomic for thread-safe port counter - Expand port range to full ephemeral range (49152-65535) - Add retry logic (up to 20 attempts) when bind fails This fixes intermittent CI failures where openssl_stream and other tests using make_socket_pair would abort with EADDRINUSE.
Previously, when std::stop_token was signaled on the epoll backend, the stop_callback only set a cancelled flag. The I/O operation remained blocked on epoll_wait indefinitely, requiring the failsafe timer or explicit socket.cancel() to interrupt it. This fix implements proper stop_token cancellation for epoll: - Add cancel_single_op() to epoll_socket_impl and epoll_acceptor_impl that unregisters the fd from epoll and posts the operation for completion with cancellation error - Store socket/acceptor impl pointer in epoll_op so the stop_callback can call back to perform actual cancellation - Fix race condition where stop is requested before the operation registers with epoll: after register_fd(), check if cancelled was already set and handle it immediately - Implement canceller::operator() out-of-line to call cancel_single_op This makes stop_token cancellation behave consistently with IOCP (Windows) where CancelIoEx provides immediate kernel-level cancellation.
When I/O operations complete synchronously (returning 0 instead of WSA_IO_PENDING), FILE_SKIP_COMPLETION_PORT_ON_SUCCESS should prevent IOCP from posting a completion packet. However, if the flag failed to set or under certain edge conditions, both the direct path and IOCP completion handler could process the same operation. Use InterlockedCompareExchange to atomically race for ownership of the completion. Only the winner (CAS returns 0) proceeds to set the result fields and post to the scheduler. This prevents double coroutine resumption and potential state corruption. Applies to connect, read_some, write_some, and accept operations.
MungoG
force-pushed
the
tls-testing-fixes
branch
from
33fdb66 to
3784fc8
Compare
2 participants
API additions:
Bug fixes:
New tests:
Test infrastructure:
Test count increased from ~100 to 798 assertions.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.