Coderabbit fixes for PR 34 #37
Conversation
- Add socket_pair.hpp header declaring make_socket_pair function - Add socket_pair.cpp implementation using loopback TCP connection - Add unit tests for socket pair creation and bidirectional I/O
📝 WalkthroughWalkthroughRefactors native error mapping into make_err(), introduces internal/wrapper lifetime model for IOCP sockets/acceptors, requires tls::context at TLS-stream construction, adds a tls::context implementation, and expands test utilities and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test (caller)
participant Socket as Socket wrapper
participant Internal as win_socket_impl_internal
participant Service as win_sockets service
participant OS as OS I/O
Test->>Socket: open()
Socket->>Service: create_impl() -> construct internal + wrapper
Service-->>Socket: wrapper (holds shared_ptr to Internal)
Socket->>Service: open_socket(internal)
Service->>Internal: register & issue open
Service->>OS: socket()/bind()/listen
OS-->>Service: success / native_err
Service->>Internal: complete(open result) using make_err(native_err)
Internal->>Socket: notify open complete
Test->>Socket: async read_some(...)
Socket->>Internal: start read op (keep internal_ptr)
Internal->>OS: readv/WSARecv
OS-->>Internal: completion (bytes, native_err)
Internal->>Service: post completion
Service->>Internal: on_completion -> convert native_err via make_err -> resume task
Internal-->>Test: read result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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://37.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-20 20:42:56 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/corosio/src/test/mocket.cpp (1)
430-443: Same thread-safety concern with static port counter.Similar to
socket_pair.cpp, this staticnext_test_portcounter has a data race if accessed from multiple threads concurrently. Consider usingstd::atomic<std::uint16_t>for thread-safe access.src/wolfssl/src/wolfssl_stream.cpp (2)
922-938: Same silent failure concern as OpenSSL constructor.The constructor silently returns on
init_ssl()failure, leaving the object in an unusable state with no error indication to the caller. This should be addressed consistently withopenssl_stream.
100-150: Add cipher suite configuration to WolfSSL implementation for feature parity with OpenSSL.The
context_data::ciphersuitesfield is part of the public API, and the OpenSSL implementation applies it viaSSL_CTX_set_cipher_list()when provided (lines 186-197 in openssl_stream.cpp). The WolfSSL implementation does not handle cipher suite configuration at all, leavingcd.ciphersuitesunused. This creates an API inconsistency where cipher suite settings are silently ignored for WolfSSL contexts while working for OpenSSL contexts.src/corosio/src/detail/iocp/sockets.cpp (1)
191-223: Guard peer_wrapper release on failure to avoid null deref.
If accept setup fails beforeop.peer_wrapperis assigned (e.g., WSASocket/IOCP association error),peer_wrappercan be null here, causing a crash.🐛 Suggested fix
- // Release the peer wrapper on failure - peer_wrapper->release(); - peer_wrapper = nullptr; + // Release the peer wrapper on failure (if created/assigned) + if (peer_wrapper) + { + peer_wrapper->release(); + peer_wrapper = nullptr; + }
🤖 Fix all issues with AI agents
In `@CMakeLists.txt`:
- Around line 216-218: The CMake guard uses the incorrect all-caps variable
OPENSSL_FOUND after calling find_package(OpenSSL), which breaks config-mode
usage; replace the variable check with a target-based check (e.g., use if
(TARGET OpenSSL::SSL) or if (TARGET OpenSSL::Crypto) or similar) so the
subsequent logic around find_package(OpenSSL) and the file(GLOB_RECURSE
BOOST_COROSIO_OPENSSL_HEADERS ...) block runs when the OpenSSL targets are
available regardless of module vs config mode; update any references that relied
on OPENSSL_FOUND to instead rely on the presence of the
OpenSSL::SSL/OpenSSL::Crypto targets.
In `@README.adoc`:
- Line 12: The "GitHub Actions" link in README.adoc currently points to the
generic https://github.com/; update that anchor so it points to the repository's
Actions tab (replace the generic URL with the repo-specific Actions URL, e.g.,
the repository base URL plus /actions or use an AsciiDoc attribute like
{repo_url}/actions). Edit the "GitHub Actions" link text in README.adoc so it
references the correct Actions page instead of the root GitHub site.
- Around line 40-41: Replace the Markdown-style link
"[LICENSE_1_0.txt](LICENSE_1_0.txt)" in README.adoc with AsciiDoc link syntax
(for example use link:LICENSE_1_0.txt[LICENSE_1_0.txt] or
xref:LICENSE_1_0.txt[]), updating the line that currently references
LICENSE_1_0.txt so the file uses consistent AsciiDoc link formatting.
In `@src/corosio/src/test/socket_pair.cpp`:
- Around line 80-91: The accept-failure branch neglects to close the
already-opened s2 socket, leaking the resource; update the block that handles
(!accept_done || accept_ec) to close s2 (in addition to acc) before throwing,
and also review the connect-failure branch to ensure both s1 and s2 are closed
there as well so all opened sockets (s2, s1, acc) are cleaned up on error.
- Around line 26-36: The mutable global next_test_port poses a data race; change
its type to std::atomic<std::uint16_t> and update get_test_port() to perform an
atomic fetch-add (e.g., auto idx = next_test_port.fetch_add(1,
std::memory_order_relaxed)) then compute port = test_port_base + (idx %
test_port_range) and return that cast to std::uint16_t; keep test_port_base and
test_port_range unchanged and preserve noexcept on get_test_port().
In `@src/openssl/src/openssl_stream.cpp`:
- Around line 756-770: The constructor openssl_stream(...) currently returns
silently when impl->init_ssl() returns an error, leaving impl_ unset; change
this to surface the failure by throwing an exception instead of returning: after
calling impl->init_ssl() on the newly created openssl_stream_impl_ instance, if
ec indicates failure, capture the error details, delete impl and throw a
std::runtime_error (or a specific exception type) including ec.message() and
context ("openssl_stream init_ssl failed"); ensure impl_ is only assigned on
success. Alternatively, if you prefer non-throwing semantics, add a boolean
member (e.g., is_valid_) initialized false, set it true only after successful
init_ssl(), and document and expose an is_valid() accessor so callers can detect
construction failure — update openssl_stream constructor, openssl_stream_impl_,
init_ssl() usage, and the impl_ assignment accordingly.
- Around line 186-197: The current code unconditionally calls
SSL_CTX_set_security_level(ctx_, 0) whenever cd.ciphersuites is non-empty, which
is overly permissive; change the logic in the initialization where
cd.ciphersuites, SSL_CTX_set_cipher_list and SSL_CTX_set_security_level are used
so you only lower the security level when the provided cipher string explicitly
requests a lower sec level (e.g. contains "@SECLEVEL=N") or when the cipher
string contains known weak tokens (e.g.
"RC4","DES","NULL","EXPORT","LOW","MD5","aNULL") that require lowering the
level; alternatively prefer trying SSL_CTX_set_cipher_list first and only call
SSL_CTX_set_security_level(ctx_, 0) as a fallback when set_cipher_list fails due
to security policy, and document in the public API (e.g. set_ciphersuites())
that omitting `@SECLEVEL` defaults to preserving OpenSSL's security level unless
weak ciphers are detected or explicitly requested (see make_anon_context() for
handling `@SECLEVEL`).
In `@test/unit/Jamfile`:
- Around line 23-26: The glob-based test discovery using "glob-tree-ex . :
*.cpp" in the for local f loop picks up header self-containment files (e.g.,
read.cpp, write.cpp) that have no main() and fail compilation; update the
Jamfile to exclude those files from the glob (e.g., modify the glob-tree-ex
invocation to exclude read.cpp and write.cpp) or move the header verification
files into a separate directory (e.g., compile_tests/) and handle them
separately so the run $(f) loop only iterates over compilable test sources.
In `@test/unit/test/socket_pair.cpp`:
- Around line 39-80: The testBidirectional coroutine uses read_some (called on
socket variables a and b) which can return partial reads; change each read block
to loop until the expected byte count is accumulated: after writing (via
a.write_some and b.write_some) repeatedly call read_some into a buffer with the
proper offset and remaining length, accumulate n returned and break on error or
when total == expected length, and assert the final total and content; ensure
you check error codes (ec) on each iteration and use the same expected lengths
(5 for "hello"/"world") when comparing the accumulated result.
In `@test/unit/tls/test_utils.hpp`:
- Around line 65-94: The inline constant server_key_pem embeds a real private
key and should be removed from source; replace its usage by either (a)
generating a test key at runtime in the test setup (e.g., create an RSA/EC key
with OpenSSL/Botan/Boost::asio utilities) and assign that PEM to server_key_pem
or a local variable before tests run, or (b) loading the PEM from a test-only
fixture file that is excluded from the repository and referenced via an
environment variable or test helper. Update any tests referencing server_key_pem
to use the runtime-generated PEM or the external fixture and ensure the
committed code no longer contains the private key literal.
🧹 Nitpick comments (9)
include/boost/corosio/io_stream.hpp (1)
220-231: Preserve const-correctness in get_impl().Returning a non-const pointer from a
constmethod permits mutation through const references. Consider returningconst*in the const overload and providing a non-const overload for mutable access.♻️ Suggested change
- io_stream_impl* - get_impl() const noexcept - { - return static_cast<io_stream_impl*>(impl_); - } + io_stream_impl const* + get_impl() const noexcept + { + return static_cast<io_stream_impl const*>(impl_); + } + + io_stream_impl* + get_impl() noexcept + { + return static_cast<io_stream_impl*>(impl_); + }test/unit/CMakeLists.txt (1)
32-35: Use TARGET check for OpenSSL test configuration.Using
if (TARGET boost_corosio_openssl)instead ofif (OPENSSL_FOUND)is more explicit and robust—it directly verifies the target exists before attempting to link. This aligns with modern CMake best practices and protects against potential edge cases.Suggested fix
-if (OPENSSL_FOUND) +if (TARGET boost_corosio_openssl) target_link_libraries(boost_corosio_tests PRIVATE boost_corosio_openssl) target_compile_definitions(boost_corosio_tests PRIVATE BOOST_COROSIO_HAS_OPENSSL=1) endif()test/unit/socket.cpp (1)
601-647: Consider polling instead of fixed sleeps to reduce test flakiness.
A short polling loop (like the acceptor tests) is more reliable under load.♻️ Suggested polling pattern (apply to both tests)
- // Wait for read to complete - timer t2(a.context()); - t2.expires_after(std::chrono::milliseconds(50)); - co_await t2.wait(); + // Poll for completion to avoid fixed sleeps + timer poll(a.context()); + for (int i = 0; i < 1000 && !read_done; ++i) + { + poll.expires_after(std::chrono::milliseconds(1)); + co_await poll.wait(); + }Also applies to: 649-693
src/corosio/src/tls/context.cpp (4)
46-61: Consider extracting file reading into a helper function.The file reading pattern (open file, check success, read via
rdbuf(), store result) is repeated verbatim inuse_certificate_file,use_certificate_chain_file,use_private_key_file,load_verify_file, andadd_crl_file. Extracting this into a private helper would reduce duplication and centralize error handling.♻️ Suggested helper function
// Add as a static helper or private method static system::result<std::string> read_file_contents(std::string_view filename) { std::ifstream file(std::string(filename), std::ios::binary); if (!file) return system::error_code(ENOENT, system::generic_category()); std::ostringstream ss; ss << file.rdbuf(); if (file.bad()) return system::error_code(EIO, system::generic_category()); return ss.str(); }Then simplify each file-loading method:
system::result<void> context::use_certificate_file(std::string_view filename, file_format format) { auto contents = read_file_contents(filename); if (!contents) return contents.error(); impl_->entity_certificate = std::move(*contents); impl_->entity_cert_format = format; return {}; }
52-58: Read errors after file open are not detected.The current implementation only checks if the file was opened successfully, but doesn't verify that the read completed without errors. If
file.rdbuf()encounters an I/O error mid-read, the error goes undetected.🔧 Add read error check
std::ostringstream ss; ss << file.rdbuf(); + if (file.bad()) + return system::error_code(EIO, system::generic_category()); impl_->entity_certificate = ss.str();
113-131: PKCS#12 stubs are correctly marked as unimplemented.The TODO comments and
ENOTSUPreturn values clearly indicate these are placeholders awaiting implementation.Would you like me to open an issue to track the PKCS#12 implementation, or help generate the implementation using OpenSSL's
d2i_PKCS12/PKCS12_parseAPIs?
207-215: Consider usingstd::spanfor ALPN protocols.Using
std::initializer_list<std::string_view>works but limits callers to brace-initialized lists. If C++20 is available,std::span<const std::string_view>would be more flexible.test/unit/tls/wolfssl_stream.cpp (1)
53-78: Consider removing dead code or adding a tracking issue.The
testFailureCases()method is implemented but never called. While documented as disabled due to cancellation issues, having unreachable test code can cause maintenance burden. Consider either:
- Removing the method until cancellation is fixed
- Adding a
TODOwith issue reference for trackingWould you like me to open an issue to track enabling the failure tests once TLS-aware cancellation is implemented?
test/unit/tls/cross_ssl_stream.cpp (1)
145-153: Missingioc.restart()after the last failure test case.While this code is currently disabled, if enabled in the future, the missing
ioc.restart()after the last test case (lines 150-152) would leave the io_context in a non-restartable state. For consistency with other test blocks, consider adding it.Suggested fix
// WolfSSL client verifies, OpenSSL server has no cert { auto client_ctx = make_client_context(); auto server_ctx = make_anon_context(); server_ctx.set_ciphersuites( "" ); run_tls_test_fail( ioc, client_ctx, server_ctx, make_wolfssl, make_openssl ); + ioc.restart(); }
| find_package(OpenSSL) | ||
| if (OpenSSL_FOUND) | ||
| if (OPENSSL_FOUND) | ||
| file(GLOB_RECURSE BOOST_COROSIO_OPENSSL_HEADERS CONFIGURE_DEPENDS |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the CMakeLists.txt file and examine the context around lines 216-218
find . -name "CMakeLists.txt" -type f | head -20Repository: cppalliance/corosio
Length of output: 327
🏁 Script executed:
# Let's read the CMakeLists.txt file around the mentioned lines
cat -n CMakeLists.txt | sed -n '210,230p'Repository: cppalliance/corosio
Length of output: 1437
🏁 Script executed:
# Let's also search for all OpenSSL-related checks in CMakeLists.txt
grep -n "OPENSSL\|OpenSSL" CMakeLists.txtRepository: cppalliance/corosio
Length of output: 831
🏁 Script executed:
# Let's check if there are other OpenSSL find_package usages in the codebase
find . -name "CMakeLists.txt" -type f -exec grep -l "find_package(OpenSSL)" {} \;Repository: cppalliance/corosio
Length of output: 79
🏁 Script executed:
# Let's also check all uses of OPENSSL_FOUND vs OpenSSL_FOUND in the repo
rg "OpenSSL_FOUND|OPENSSL_FOUND" --type cmakeRepository: cppalliance/corosio
Length of output: 140
🌐 Web query:
CMake find_package(OpenSSL) module mode vs config mode OPENSSL_FOUND vs OpenSSL_FOUND
💡 Result:
Short answer
- Module-mode (FindOpenSSL.cmake) historically set variables named OPENSSL_FOUND and other OPENSSL_* variables. Newer CMake versions add a canonical, mixed-case variable OpenSSL_FOUND and imported targets (OpenSSL::Crypto, OpenSSL::SSL). (cmake.org)
- Config-mode (OpenSSLConfig.cmake provided by OpenSSL installs / packages) exposes the package’s config and usually defines OpenSSL_FOUND and the imported targets. (cmake.org)
- OPENSSL_FOUND (all-caps) is the older, module-mode compatibility variable; OpenSSL_FOUND (mixed-case) is the modern canonical name (OpenSSL_FOUND was added in CMake 3.3 and OPENSSL_FOUND was later deprecated in recent CMake docs). Prefer checking the imported targets (target_exists/OpenSSL::SSL) or OpenSSL_FOUND. (cmake.org)
Recommendation (minimal)
- Use targets when possible:
target_link_libraries(myTarget PRIVATE OpenSSL::SSL OpenSSL::Crypto) - For boolean checks, prefer:
if(OpenSSL_FOUND)
...
elseif(OPENSSL_FOUND) # for older CMake compatibility
...
endif()
(cmake.org)
Citations:
- 1: https://cmake.org/cmake/help/v3.13/module/FindOpenSSL.html?utm_source=openai
- 2: https://cmake.org/cmake/help/git-stage/module/FindOpenSSL.html?utm_source=openai
- 3: https://cmake.org/cmake/help/git-stage/module/FindOpenSSL.html?utm_source=openai
- 4: https://cmake.org/cmake/help/git-stage/module/FindOpenSSL.html?utm_source=openai
Use target checking instead of OPENSSL_FOUND variable.
find_package(OpenSSL) in config mode sets OpenSSL_FOUND (mixed-case), not OPENSSL_FOUND (all-caps). The current guard prevents OpenSSL support when using config-mode packages. Check targets instead, since the code already depends on them and target checks work with both module and config modes.
🛠️ Suggested fix
find_package(OpenSSL)
-if (OPENSSL_FOUND)
+if (TARGET OpenSSL::SSL AND TARGET OpenSSL::Crypto)
file(GLOB_RECURSE BOOST_COROSIO_OPENSSL_HEADERS CONFIGURE_DEPENDS
"${CMAKE_CURRENT_SOURCE_DIR}/include/boost/corosio/openssl/*.hpp")
@@
-endif ()
+endif ()📝 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.
| find_package(OpenSSL) | |
| if (OpenSSL_FOUND) | |
| if (OPENSSL_FOUND) | |
| file(GLOB_RECURSE BOOST_COROSIO_OPENSSL_HEADERS CONFIGURE_DEPENDS | |
| find_package(OpenSSL) | |
| if (TARGET OpenSSL::SSL AND TARGET OpenSSL::Crypto) | |
| file(GLOB_RECURSE BOOST_COROSIO_OPENSSL_HEADERS CONFIGURE_DEPENDS | |
| "${CMAKE_CURRENT_SOURCE_DIR}/include/boost/corosio/openssl/*.hpp") | |
| endif () |
🤖 Prompt for AI Agents
In `@CMakeLists.txt` around lines 216 - 218, The CMake guard uses the incorrect
all-caps variable OPENSSL_FOUND after calling find_package(OpenSSL), which
breaks config-mode usage; replace the variable check with a target-based check
(e.g., use if (TARGET OpenSSL::SSL) or if (TARGET OpenSSL::Crypto) or similar)
so the subsequent logic around find_package(OpenSSL) and the file(GLOB_RECURSE
BOOST_COROSIO_OPENSSL_HEADERS ...) block runs when the OpenSSL targets are
available regardless of module vs config mode; update any references that relied
on OPENSSL_FOUND to instead rely on the presence of the
OpenSSL::SSL/OpenSSL::Crypto targets.
| (See accompanying file [LICENSE_1_0.txt](LICENSE_1_0.txt) or copy at | ||
| https://www.boost.org/LICENSE_1_0.txt) |
There was a problem hiding this comment.
Mixed Markdown and AsciiDoc syntax.
The license file link uses Markdown syntax [LICENSE_1_0.txt](LICENSE_1_0.txt) within an AsciiDoc file. Use AsciiDoc link syntax for consistency.
Suggested fix
-(See accompanying file [LICENSE_1_0.txt](LICENSE_1_0.txt) or copy at
+(See accompanying file link:LICENSE_1_0.txt[LICENSE_1_0.txt] or copy at📝 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.
| (See accompanying file [LICENSE_1_0.txt](LICENSE_1_0.txt) or copy at | |
| https://www.boost.org/LICENSE_1_0.txt) | |
| (See accompanying file link:LICENSE_1_0.txt[LICENSE_1_0.txt] or copy at | |
| https://www.boost.org/LICENSE_1_0.txt) |
🤖 Prompt for AI Agents
In `@README.adoc` around lines 40 - 41, Replace the Markdown-style link
"[LICENSE_1_0.txt](LICENSE_1_0.txt)" in README.adoc with AsciiDoc link syntax
(for example use link:LICENSE_1_0.txt[LICENSE_1_0.txt] or
xref:LICENSE_1_0.txt[]), updating the line that currently references
LICENSE_1_0.txt so the file uses consistent AsciiDoc link formatting.
| constexpr std::uint16_t test_port_base = 49300; | ||
| constexpr std::uint16_t test_port_range = 100; | ||
| std::uint16_t next_test_port = 0; | ||
|
|
||
| std::uint16_t | ||
| get_test_port() noexcept | ||
| { | ||
| auto port = test_port_base + (next_test_port % test_port_range); | ||
| ++next_test_port; | ||
| return static_cast<std::uint16_t>(port); | ||
| } |
There was a problem hiding this comment.
Thread safety concern with static port counter.
The next_test_port counter is a mutable static variable accessed without synchronization. If tests run concurrently from multiple threads, this creates a data race.
Consider using std::atomic<std::uint16_t> for thread-safe access:
🔧 Proposed fix
constexpr std::uint16_t test_port_base = 49300;
constexpr std::uint16_t test_port_range = 100;
-std::uint16_t next_test_port = 0;
+std::atomic<std::uint16_t> next_test_port{0};
std::uint16_t
get_test_port() noexcept
{
- auto port = test_port_base + (next_test_port % test_port_range);
- ++next_test_port;
+ auto current = next_test_port.fetch_add(1, std::memory_order_relaxed);
+ auto port = test_port_base + (current % test_port_range);
return static_cast<std::uint16_t>(port);
}🤖 Prompt for AI Agents
In `@src/corosio/src/test/socket_pair.cpp` around lines 26 - 36, The mutable
global next_test_port poses a data race; change its type to
std::atomic<std::uint16_t> and update get_test_port() to perform an atomic
fetch-add (e.g., auto idx = next_test_port.fetch_add(1,
std::memory_order_relaxed)) then compute port = test_port_base + (idx %
test_port_range) and return that cast to std::uint16_t; keep test_port_base and
test_port_range unchanged and preserve noexcept on get_test_port().
| if (!accept_done || accept_ec) | ||
| { | ||
| acc.close(); | ||
| throw std::runtime_error("socket_pair accept failed"); | ||
| } | ||
|
|
||
| if (!connect_done || connect_ec) | ||
| { | ||
| acc.close(); | ||
| s1.close(); | ||
| throw std::runtime_error("socket_pair connect failed"); | ||
| } |
There was a problem hiding this comment.
Missing cleanup of s2 socket on accept failure.
When accept fails (lines 80-84), s2 was already opened at line 56 but is not closed before throwing. This could leak the socket resource.
🔧 Proposed fix
if (!accept_done || accept_ec)
{
acc.close();
+ s2.close();
throw std::runtime_error("socket_pair accept failed");
}📝 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.
| if (!accept_done || accept_ec) | |
| { | |
| acc.close(); | |
| throw std::runtime_error("socket_pair accept failed"); | |
| } | |
| if (!connect_done || connect_ec) | |
| { | |
| acc.close(); | |
| s1.close(); | |
| throw std::runtime_error("socket_pair connect failed"); | |
| } | |
| if (!accept_done || accept_ec) | |
| { | |
| acc.close(); | |
| s2.close(); | |
| throw std::runtime_error("socket_pair accept failed"); | |
| } | |
| if (!connect_done || connect_ec) | |
| { | |
| acc.close(); | |
| s1.close(); | |
| throw std::runtime_error("socket_pair connect failed"); | |
| } |
🤖 Prompt for AI Agents
In `@src/corosio/src/test/socket_pair.cpp` around lines 80 - 91, The
accept-failure branch neglects to close the already-opened s2 socket, leaking
the resource; update the block that handles (!accept_done || accept_ec) to close
s2 (in addition to acc) before throwing, and also review the connect-failure
branch to ensure both s1 and s2 are closed there as well so all opened sockets
(s2, s1, acc) are cleaned up on error.
| // Apply cipher suites if provided | ||
| if( !cd.ciphersuites.empty() ) | ||
| { | ||
| SSL_CTX_set_security_level( ctx_, 0 ); | ||
| if( SSL_CTX_set_cipher_list( ctx_, cd.ciphersuites.c_str() ) == 0 ) | ||
| { | ||
| // Invalid cipher string - abort initialization | ||
| SSL_CTX_free( ctx_ ); | ||
| ctx_ = nullptr; | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenSSL SSL_CTX_set_security_level security restrictions
💡 Result:
Summary — what SSL_CTX_set_security_level(n) does
- It sets a global “security level” (0–5) that enforces minimum algorithm/key/parameter strengths and protocol/feature bans for that SSL_CTX (and SSL_set_security_level for per-connection). Call: SSL_CTX_set_security_level(ctx, n). [1]
- The security level filters/blocks: cipher suites, minimum RSA/DSA/DH/ECC key sizes, disallowed MAC/hash/signature algorithms, TLS/SSL versions, and some features (compression, RC4, session tickets, non‑PFS suites, etc.). Some violations remove ciphers from the advertised list (client); others abort the handshake or prevent setting weak certificates/params. [1][2]
Brief per-level restrictions (canonical behavior)
- 0 — no extra restrictions (legacy compatibility).
- 1 — ≈80 bits security: forbids <1024‑bit RSA/DSA/DH, ECC <160 bits; forbids MD5 MAC, CCM‑64, SHA1/MD5 signatures; disables SSLv3, TLS1.0, TLS1.1, DTLS1.0. (Compatibility level in many stacks.) [1]
- 2 — ≈112 bits: forbids <2048‑bit RSA/DSA/DH, ECC <224 bits; additionally forbids RC4; disables compression. [1]
- 3 — ≈128 bits: forbids <3072‑bit RSA/DSA/DH, ECC <256 bits; additionally requires forward secrecy (no non‑PFS suites); disables session tickets. [1]
- 4 — ≈192 bits: forbids <7680‑bit RSA/DSA/DH, ECC <384 bits; forbids SHA1 MAC. [1]
- 5 — ≈256 bits: forbids <15360‑bit RSA/DSA/DH, ECC <512 bits. [1]
Notes / practical points
- The security level is applied after any cipher-list or other settings: setting a very permissive cipher list does not override the security level — items inconsistent with the level are rejected. [1][3]
- You can see what ciphers survive a security level with the openssl ciphers tool, e.g. openssl ciphers -s -v 'ALL:
@SECLEVEL=2'. [2] - Default: the OpenSSL build-time default can be changed with -DOPENSSL_TLS_SECURITY_LEVEL; if not set the documentation notes a default of 2 (check your OpenSSL build/distribution for its configured default). [1]
If you want, I can:
- show the exact OpenSSL docs/man lines for each bullet, or
- help you choose a level for a given compatibility/performance goal, or
- show an example of detecting rejected certificates/handshakes caused by a security level.
🏁 Script executed:
# First, let's look at the full context of the cipher suite configuration and understand when/how ciphersuites are provided
cat -n src/openssl/src/openssl_stream.cpp | sed -n '180,210p'Repository: cppalliance/corosio
Length of output: 1023
🏁 Script executed:
# Let's also check the struct definition to understand what cd.ciphersuites represents
rg -A 5 "ciphersuites" src/openssl/src/openssl_stream.cpp | head -30Repository: cppalliance/corosio
Length of output: 414
🏁 Script executed:
# Look for any other security-related settings or comments in the file
rg -i "security\|cipher\|tls|ssl" src/openssl/src/openssl_stream.cpp | head -20Repository: cppalliance/corosio
Length of output: 1136
🏁 Script executed:
# Check if there's any documentation or configuration that explains the cipher suite handling
find . -name "*.md" -o -name "*.txt" -o -name "*.rst" | xargs grep -l "cipher\|security" 2>/dev/null | head -5Repository: cppalliance/corosio
Length of output: 94
🏁 Script executed:
# Check the tls-context-design.md mentioned in earlier results
cat ./context/tls-context-design.md | head -100Repository: cppalliance/corosio
Length of output: 4715
🏁 Script executed:
# Look for any documentation about the context_data and ciphersuites parameter
rg -B5 -A10 "struct context_data\|class context_data" src/openssl/Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Check public API headers for documentation on cipher suite parameter
find . -name "*.hpp" -o -name "*.h" | xargs grep -l "context\|cipher" 2>/dev/null | head -5Repository: cppalliance/corosio
Length of output: 246
🏁 Script executed:
# Look for any comments in the constructor about the cipher suite behavior
rg -B10 "openssl_native_context.*context_data" src/openssl/src/openssl_stream.cppRepository: cppalliance/corosio
Length of output: 351
🏁 Script executed:
# Find the context_data definition to understand the ciphersuites parameter
find . -name "*.hpp" -o -name "*.h" | xargs grep -l "context_data" 2>/dev/nullRepository: cppalliance/corosio
Length of output: 148
🏁 Script executed:
# Look at the header file that defines context_data
cat ./include/boost/corosio/tls/context_data.hpp 2>/dev/null || cat ./include/boost/corosio/context_data.hpp 2>/dev/null || rg "context_data" --type hpp -A 15 | head -60Repository: cppalliance/corosio
Length of output: 90
🏁 Script executed:
# Check if there's any documentation comment about the cipher suites behavior in the public API
find . -name "*.hpp" -path "*/tls/*" | xargs cat 2>/dev/null | grep -B5 -A10 "ciphersuites\|cipher" | head -80Repository: cppalliance/corosio
Length of output: 2483
Unconditional security level 0 for all custom cipher suites is overly permissive.
Setting SSL_CTX_set_security_level(ctx_, 0) whenever custom ciphersuites are provided disables OpenSSL's security checks unconditionally, even for strong cipher specifications like ECDHE-RSA-AES256-GCM-SHA384. This shifts the entire burden of secure selection to the user without documenting the behavior.
Instead:
- Document in the public API that
set_ciphersuites()defaults to security level 0 (or reconsider the design) - Consider allowing users to specify
@SECLEVEL=Nwithin their cipher string (as shown in themake_anon_context()helper) - Only lower security level when weak ciphers are actually specified, rather than unconditionally for all custom ciphers
🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 186 - 197, The current code
unconditionally calls SSL_CTX_set_security_level(ctx_, 0) whenever
cd.ciphersuites is non-empty, which is overly permissive; change the logic in
the initialization where cd.ciphersuites, SSL_CTX_set_cipher_list and
SSL_CTX_set_security_level are used so you only lower the security level when
the provided cipher string explicitly requests a lower sec level (e.g. contains
"@SECLEVEL=N") or when the cipher string contains known weak tokens (e.g.
"RC4","DES","NULL","EXPORT","LOW","MD5","aNULL") that require lowering the
level; alternatively prefer trying SSL_CTX_set_cipher_list first and only call
SSL_CTX_set_security_level(ctx_, 0) as a fallback when set_cipher_list fails due
to security policy, and document in the public API (e.g. set_ciphersuites())
that omitting `@SECLEVEL` defaults to preserving OpenSSL's security level unless
weak ciphers are detected or explicitly requested (see make_anon_context() for
handling `@SECLEVEL`).
| openssl_stream:: | ||
| openssl_stream( io_stream& stream, tls::context ctx ) | ||
| : tls_stream( stream ) | ||
| { | ||
| auto* impl = new openssl_stream_impl_( s_, std::move( ctx ) ); | ||
|
|
||
| auto ec = impl->init_ssl(); | ||
| if( ec ) | ||
| { | ||
| delete impl; | ||
| return; | ||
| } | ||
|
|
||
| impl_ = impl; | ||
| } |
There was a problem hiding this comment.
Silent failure on construction is problematic.
When init_ssl() fails, the constructor silently returns without setting impl_, leaving the object in an unusable state. The caller has no way to detect this failure.
Consider either:
- Throwing an exception on initialization failure
- Providing an
is_valid()or similar method and documenting that callers must check it
🔧 Example: throw on failure
openssl_stream::
openssl_stream( io_stream& stream, tls::context ctx )
: tls_stream( stream )
{
auto* impl = new openssl_stream_impl_( s_, std::move( ctx ) );
auto ec = impl->init_ssl();
if( ec )
{
delete impl;
- return;
+ throw system::system_error( ec, "openssl_stream initialization failed" );
}
impl_ = impl;
}📝 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.
| openssl_stream:: | |
| openssl_stream( io_stream& stream, tls::context ctx ) | |
| : tls_stream( stream ) | |
| { | |
| auto* impl = new openssl_stream_impl_( s_, std::move( ctx ) ); | |
| auto ec = impl->init_ssl(); | |
| if( ec ) | |
| { | |
| delete impl; | |
| return; | |
| } | |
| impl_ = impl; | |
| } | |
| openssl_stream:: | |
| openssl_stream( io_stream& stream, tls::context ctx ) | |
| : tls_stream( stream ) | |
| { | |
| auto* impl = new openssl_stream_impl_( s_, std::move( ctx ) ); | |
| auto ec = impl->init_ssl(); | |
| if( ec ) | |
| { | |
| delete impl; | |
| throw system::system_error( ec, "openssl_stream initialization failed" ); | |
| } | |
| impl_ = impl; | |
| } |
🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 756 - 770, The constructor
openssl_stream(...) currently returns silently when impl->init_ssl() returns an
error, leaving impl_ unset; change this to surface the failure by throwing an
exception instead of returning: after calling impl->init_ssl() on the newly
created openssl_stream_impl_ instance, if ec indicates failure, capture the
error details, delete impl and throw a std::runtime_error (or a specific
exception type) including ec.message() and context ("openssl_stream init_ssl
failed"); ensure impl_ is only assigned on success. Alternatively, if you prefer
non-throwing semantics, add a boolean member (e.g., is_valid_) initialized
false, set it true only after successful init_ssl(), and document and expose an
is_valid() accessor so callers can detect construction failure — update
openssl_stream constructor, openssl_stream_impl_, init_ssl() usage, and the
impl_ assignment accordingly.
| for local f in [ glob-tree-ex . : *.cpp ] | ||
| { | ||
| run $(f) ; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's see the Jamfile content and directory structure
echo "=== test/unit/Jamfile content ==="
cat test/unit/Jamfile 2>/dev/null || echo "File not found"
echo ""
echo "=== Directory structure of test/unit/ ==="
find test/unit -type f -name "*.cpp" 2>/dev/null | sort
echo ""
echo "=== Directory structure overview ==="
find test/unit -type d 2>/dev/null | head -20Repository: cppalliance/corosio
Length of output: 1353
🏁 Script executed:
#!/bin/bash
# Check each .cpp file for test indicators
echo "=== Checking .cpp files for test markers ==="
find test/unit -name "*.cpp" -type f 2>/dev/null | while read file; do
echo "--- $file ---"
# Check for test markers
if grep -q "TEST_SUITE\|int main\|BOOST_AUTO_TEST\|BOOST_TEST" "$file"; then
echo " ✓ Contains test markers"
else
echo " ✗ No test markers found"
fi
# Show first few lines to understand purpose
head -3 "$file"
echo ""
doneRepository: cppalliance/corosio
Length of output: 2312
🏁 Script executed:
#!/bin/bash
# Examine the two files that lack test markers
echo "=== test/unit/read.cpp - First 50 lines ==="
head -50 test/unit/read.cpp
echo ""
echo "=== test/unit/write.cpp - First 50 lines ==="
head -50 test/unit/write.cppRepository: cppalliance/corosio
Length of output: 963
🏁 Script executed:
#!/bin/bash
# Check for int main() function which is required for standalone executables
echo "=== Checking for main() function ==="
for file in test/unit/read.cpp test/unit/write.cpp; do
if grep -q "int main" "$file"; then
echo "$file: ✓ Has main()"
else
echo "$file: ✗ NO main() function found"
fi
doneRepository: cppalliance/corosio
Length of output: 194
🏁 Script executed:
#!/bin/bash
echo "=== Full content: test/unit/read.cpp ==="
cat test/unit/read.cpp
echo ""
echo "=== Full content: test/unit/write.cpp ==="
cat test/unit/write.cppRepository: cppalliance/corosio
Length of output: 957
🏁 Script executed:
#!/bin/bash
# Check if these files are meant to be header-only verification or if there's build configuration
echo "=== Checking total line counts ==="
wc -l test/unit/read.cpp test/unit/write.cpp
echo ""
echo "=== Check if these are mentioned in git history ==="
git log --oneline --all -- test/unit/read.cpp test/unit/write.cpp 2>/dev/null | head -20Repository: cppalliance/corosio
Length of output: 281
Glob-based test discovery will fail on header self-containment tests.
The glob-tree-ex . : *.cpp pattern picks up read.cpp and write.cpp, which are header self-containment verification files containing only includes and no main() function. These files cannot be compiled as standalone executables and will cause the build to fail.
Either exclude these files from the glob pattern:
for local f in [ glob-tree-ex . : *.cpp : read.cpp write.cpp ]
Or move these header verification files to a different location outside the glob scope, such as a compile_tests/ subdirectory, and handle them separately in the Jamfile.
🤖 Prompt for AI Agents
In `@test/unit/Jamfile` around lines 23 - 26, The glob-based test discovery using
"glob-tree-ex . : *.cpp" in the for local f loop picks up header
self-containment files (e.g., read.cpp, write.cpp) that have no main() and fail
compilation; update the Jamfile to exclude those files from the glob (e.g.,
modify the glob-tree-ex invocation to exclude read.cpp and write.cpp) or move
the header verification files into a separate directory (e.g., compile_tests/)
and handle them separately so the run $(f) loop only iterates over compilable
test sources.
| void | ||
| testBidirectional() | ||
| { | ||
| io_context ioc; | ||
|
|
||
| auto [s1, s2] = make_socket_pair(ioc); | ||
|
|
||
| capy::run_async(ioc.get_executor())( | ||
| [](socket& a, socket& b) -> capy::task<> | ||
| { | ||
| char buf[32] = {}; | ||
|
|
||
| // Write from s1, read from s2 | ||
| auto [ec1, n1] = co_await a.write_some( | ||
| capy::const_buffer("hello", 5)); | ||
| BOOST_TEST(!ec1); | ||
| BOOST_TEST_EQ(n1, 5u); | ||
|
|
||
| auto [ec2, n2] = co_await b.read_some( | ||
| capy::mutable_buffer(buf, sizeof(buf))); | ||
| BOOST_TEST(!ec2); | ||
| BOOST_TEST_EQ(n2, 5u); | ||
| BOOST_TEST_EQ(std::string_view(buf, n2), "hello"); | ||
|
|
||
| // Write from s2, read from s1 | ||
| auto [ec3, n3] = co_await b.write_some( | ||
| capy::const_buffer("world", 5)); | ||
| BOOST_TEST(!ec3); | ||
| BOOST_TEST_EQ(n3, 5u); | ||
|
|
||
| auto [ec4, n4] = co_await a.read_some( | ||
| capy::mutable_buffer(buf, sizeof(buf))); | ||
| BOOST_TEST(!ec4); | ||
| BOOST_TEST_EQ(n4, 5u); | ||
| BOOST_TEST_EQ(std::string_view(buf, n4), "world"); | ||
| }(s1, s2)); | ||
|
|
||
| ioc.run(); | ||
|
|
||
| s1.close(); | ||
| s2.close(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.hpp" -o -name "*.h" -o -name "*.cpp" | grep -i socket | head -20Repository: cppalliance/corosio
Length of output: 393
🏁 Script executed:
rg "class socket" -A 5 --type cpp --type hppRepository: cppalliance/corosio
Length of output: 90
🏁 Script executed:
fd "socket_pair" -type fRepository: cppalliance/corosio
Length of output: 233
🏁 Script executed:
wc -l ./include/boost/corosio/socket.hpp ./include/boost/corosio/test/socket_pair.hppRepository: cppalliance/corosio
Length of output: 163
🏁 Script executed:
cat -n ./include/boost/corosio/socket.hpp | head -100Repository: cppalliance/corosio
Length of output: 3615
🏁 Script executed:
cat -n ./include/boost/corosio/test/socket_pair.hppRepository: cppalliance/corosio
Length of output: 1366
🏁 Script executed:
cat -n ./include/boost/corosio/socket.hpp | tail -200Repository: cppalliance/corosio
Length of output: 6935
🏁 Script executed:
rg "read_some" ./include/boost/corosio/socket.hpp -A 10Repository: cppalliance/corosio
Length of output: 509
🏁 Script executed:
cat -n ./src/corosio/src/test/socket_pair.cppRepository: cppalliance/corosio
Length of output: 3325
🏁 Script executed:
rg "read_some" --type-list | headRepository: cppalliance/corosio
Length of output: 452
🏁 Script executed:
fd io_stream -type fRepository: cppalliance/corosio
Length of output: 233
🏁 Script executed:
rg "read_some" . -A 15 | head -80Repository: cppalliance/corosio
Length of output: 4843
🏁 Script executed:
find . -name "io_stream.hpp" -o -name "io_stream.cpp"Repository: cppalliance/corosio
Length of output: 100
🏁 Script executed:
rg "class io_stream" -A 50Repository: cppalliance/corosio
Length of output: 3284
🏁 Script executed:
cat -n ./test/unit/tls/test_utils.hpp | grep -A 20 "read_some"Repository: cppalliance/corosio
Length of output: 1734
🏁 Script executed:
cat -n ./include/boost/corosio/io_stream.hpp | head -150Repository: cppalliance/corosio
Length of output: 6255
🏁 Script executed:
rg "read_some" ./include/boost/corosio/io_stream.hpp -B 5 -A 20Repository: cppalliance/corosio
Length of output: 4091
Loop in read_some calls to handle partial reads.
The documentation explicitly states read_some may return fewer bytes than the buffer capacity. Use a while loop to accumulate all expected bytes to avoid test flakiness.
Suggested fix (loop until full message)
- auto [ec2, n2] = co_await b.read_some(
- capy::mutable_buffer(buf, sizeof(buf)));
- BOOST_TEST(!ec2);
- BOOST_TEST_EQ(n2, 5u);
- BOOST_TEST_EQ(std::string_view(buf, n2), "hello");
+ std::size_t total = 0;
+ while (total < 5)
+ {
+ auto [ec2, n2] = co_await b.read_some(
+ capy::mutable_buffer(buf + total, sizeof(buf) - total));
+ BOOST_TEST(!ec2);
+ BOOST_TEST(n2 > 0);
+ total += n2;
+ }
+ BOOST_TEST_EQ(std::string_view(buf, total), "hello");
@@
- auto [ec4, n4] = co_await a.read_some(
- capy::mutable_buffer(buf, sizeof(buf)));
- BOOST_TEST(!ec4);
- BOOST_TEST_EQ(n4, 5u);
- BOOST_TEST_EQ(std::string_view(buf, n4), "world");
+ total = 0;
+ while (total < 5)
+ {
+ auto [ec4, n4] = co_await a.read_some(
+ capy::mutable_buffer(buf + total, sizeof(buf) - total));
+ BOOST_TEST(!ec4);
+ BOOST_TEST(n4 > 0);
+ total += n4;
+ }
+ BOOST_TEST_EQ(std::string_view(buf, total), "world");🤖 Prompt for AI Agents
In `@test/unit/test/socket_pair.cpp` around lines 39 - 80, The testBidirectional
coroutine uses read_some (called on socket variables a and b) which can return
partial reads; change each read block to loop until the expected byte count is
accumulated: after writing (via a.write_some and b.write_some) repeatedly call
read_some into a buffer with the proper offset and remaining length, accumulate
n returned and break on error or when total == expected length, and assert the
final total and content; ensure you check error codes (ec) on each iteration and
use the same expected lengths (5 for "hello"/"world") when comparing the
accumulated result.
| // Server private key from Boost.Beast | ||
| inline constexpr char const* server_key_pem = | ||
| "-----BEGIN PRIVATE KEY-----\n" | ||
| "MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCz0GwgnxSBhygx\n" | ||
| "BdhTHGx5LDLIJSuIDJ6nMwZFvAjdhLnB/vOTLppr5MKxqQHEpYdyDYGD1noBoz4T\n" | ||
| "iIRj5JapChMgx58NLq5QyXkHV/ONT7yi8x05P41c2F9pBEnUwUxIUG1Cb6AN0cZW\n" | ||
| "F/wSMOZ0w3DoBhnl1sdQfQiS25MTK6x4tATmWm9SJc2lsjWptbyIN6hFXLYPXTwn\n" | ||
| "YzCLvv1EK6Ft7tMPc/FcJpd/wYHgl8shDmY7rV+AiGTxUU35V0AzpJlmvct5aJV/\n" | ||
| "5vSRRLwT9qLZSddE9zy/0rovC5GML6S7BUC4lIzJ8yxzOzSStBPxvdrOobSSNlRZ\n" | ||
| "IlE7gnyNAgMBAAECggEAY0RorQmldGx9D7M+XYOPjsWLs1px0cXFwGA20kCgVEp1\n" | ||
| "kleBeHt93JqJsTKwOzN2tswl9/ZrnIPWPUpcbBlB40ggjzQk5k4jBY50Nk2jsxuV\n" | ||
| "9A9qzrP7AoqhAYTQjZe42SMtbkPZhEeOyvCqxBAi6csLhcv4eB4+In0kQo7dfvLs\n" | ||
| "Xu/3WhSsuAWqdD9EGnhD3n+hVTtgiasRe9318/3R9DzP+IokoQGOtXm+1dsfP0mV\n" | ||
| "8XGzQHBpUtJNn0yi6SC4kGEQuKkX33zORlSnZgT5VBLofNgra0THd7x3atOx1lbr\n" | ||
| "V0QizvCdBa6j6FwhOQwW8UwgOCnUbWXl/Xn4OaofMQKBgQDdRXSMyys7qUMe4SYM\n" | ||
| "Mdawj+rjv0Hg98/xORuXKEISh2snJGKEwV7L0vCn468n+sM19z62Axz+lvOUH8Qr\n" | ||
| "hLkBNqJvtIP+b0ljRjem78K4a4qIqUlpejpRLw6a/+44L76pMJXrYg3zdBfwzfwu\n" | ||
| "b9NXdwHzWoNuj4v36teGP6xOUwKBgQDQCT52XX96NseNC6HeK5BgWYYjjxmhksHi\n" | ||
| "stjzPJKySWXZqJpHfXI8qpOd0Sd1FHB+q1s3hand9c+Rxs762OXlqA9Q4i+4qEYZ\n" | ||
| "qhyRkTsl+2BhgzxmoqGd5gsVT7KV8XqtuHWLmetNEi+7+mGSFf2iNFnonKlvT1JX\n" | ||
| "4OQZC7ntnwKBgH/ORFmmaFxXkfteFLnqd5UYK5ZMvGKTALrWP4d5q2BEc7HyJC2F\n" | ||
| "+5lDR9nRezRedS7QlppPBgpPanXeO1LfoHSA+CYJYEwwP3Vl83Mq/Y/EHgp9rXeN\n" | ||
| "L+4AfjEtLo2pljjnZVDGHETIg6OFdunjkXDtvmSvnUbZBwG11bMnSAEdAoGBAKFw\n" | ||
| "qwJb6FNFM3JnNoQctnuuvYPWxwM1yjRMqkOIHCczAlD4oFEeLoqZrNhpuP8Ij4wd\n" | ||
| "GjpqBbpzyVLNP043B6FC3C/edz4Lh+resjDczVPaUZ8aosLbLiREoxE0udfWf2dU\n" | ||
| "oBNnrMwwcs6jrRga7Kr1iVgUSwBQRAxiP2CYUv7tAoGBAKdPdekPNP/rCnHkKIkj\n" | ||
| "o13pr+LJ8t+15vVzZNHwPHUWiYXFhG8Ivx7rqLQSPGcuPhNss3bg1RJiZAUvF6fd\n" | ||
| "e6QS4EZM9dhhlO2FmPQCJMrRVDXaV+9TcJZXCbclQnzzBus9pwZZyw4Anxo0vmir\n" | ||
| "nOMOU6XI4lO9Xge/QDEN4Y2R\n" | ||
| "-----END PRIVATE KEY-----\n"; |
There was a problem hiding this comment.
Private key in repo will trigger secret‑scanning / compliance alarms.
Even for test fixtures, this typically fails CI or violates policy. Consider moving the key to a test fixture file with an explicit allowlist entry, or generating it at test runtime.
🛠️ Minimal mitigation (if policy allows test keys)
-// Server private key from Boost.Beast
+// Server private key from Boost.Beast (test‑only fixture)
+// gitleaks:allow
inline constexpr char const* server_key_pem =🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 67-94: 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 65 - 94, The inline constant
server_key_pem embeds a real private key and should be removed from source;
replace its usage by either (a) generating a test key at runtime in the test
setup (e.g., create an RSA/EC key with OpenSSL/Botan/Boost::asio utilities) and
assign that PEM to server_key_pem or a local variable before tests run, or (b)
loading the PEM from a test-only fixture file that is excluded from the
repository and referenced via an environment variable or test helper. Update any
tests referencing server_key_pem to use the runtime-generated PEM or the
external fixture and ensure the committed code no longer contains the private
key literal.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/corosio/src/detail/iocp/sockets.cpp (1)
176-226: Guard peer_wrapper on failure to avoid stale/null release.
Line 220 unconditionally callspeer_wrapper->release(). In Line 970-989, early setup failures post the op beforepeer_wrapperis assigned, soop.peer_wrappercan benullptror stale from a prior accept, leading to a crash or releasing the wrong wrapper.🛠️ Proposed fix
@@ void accept_op:: operator()() { @@ - // Release the peer wrapper on failure - peer_wrapper->release(); - peer_wrapper = nullptr; + // Release the peer wrapper on failure (if one was assigned) + if (peer_wrapper) + { + peer_wrapper->release(); + peer_wrapper = nullptr; + } @@ void win_acceptor_impl_internal:: accept( @@ - op.reset(); + op.reset(); + op.peer_wrapper = nullptr; + op.accepted_socket = INVALID_SOCKET;Also applies to: 947-1013
3 participants
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.