api and bug fixes #30
Conversation
|
An automated preview of the documentation is available at https://30.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-19 23:52:51 UTC |
vinniefalco
force-pushed
the
develop
branch
3 times, most recently
from
d43fa35 to
e2fbdd7
Compare
📝 WalkthroughWalkthroughAdds a backend-agnostic TLS context API, OpenSSL TLS backend and streams, migrates internal buffer type to Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant TLS as tls::context / tls_stream
participant SSL as OpenSSL (SSL*, SSL_CTX)
participant IO as io_stream (socket)
rect rgba(0,128,255,0.5)
App->>TLS: construct tls_stream with context
TLS->>TLS: obtain cached native SSL_CTX (get_openssl_context)
TLS->>SSL: create SSL* and BIO pair, apply session config (SNI/etc)
end
rect rgba(0,200,100,0.5)
App->>TLS: await handshake(client/server)
TLS->>IO: perform underlying read/write via BIO flush/read
IO->>TLS: network bytes / socket completions
TLS->>SSL: feed bytes to SSL/BIO and call SSL_connect/SSL_accept
SSL->>TLS: wants_read / wants_write or handshake complete
TLS-->>App: handshake result (success or ec)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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. ✨ Finishing touches
Comment |
![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
🤖 Fix all issues with AI agents
In `@example/tls_context_examples.cpp`:
- Around line 125-131: In the password callback passed to
ctx.set_password_callback, guard against std::getenv returning null and enforce
the max_len contract: retrieve the env var name "TLS_KEY_PASSWORD", check for
nullptr and use an empty string if null, and when constructing the returned
std::string truncate to at most max_len characters (e.g., by taking a substring
or copying only max_len bytes) before returning; keep the lambda signature and
use tls::password_purpose only for any conditional logic if needed.
In `@include/boost/corosio/tcp_server.hpp`:
- Around line 273-278: The try_pop() implementation dereferences idle_ without
checking for nullptr, causing undefined behavior when the idle list is empty;
update worker_base* try_pop() to first check if idle_ is nullptr and return
nullptr in that case, otherwise store idle_ in a local variable (e.g., w),
advance idle_ to w->next, and return w, preserving noexcept and using the
existing worker_base type and idle_ member.
In `@include/boost/corosio/tls/context.hpp`:
- Around line 696-699: The template declarations for set_verify_callback and
set_password_callback are missing definitions, causing link errors when
instantiated; add their implementations so they are available to all translation
units by either (a) defining template<typename Callback> system::result<void>
set_verify_callback(Callback callback) and template<typename Callback>
system::result<void> set_password_callback(Callback callback) inline in the
header near their declarations, or (b) move their implementations to a companion
.ipp and `#include` it at the end of context.hpp, or (c) replace them with
explicit non-template overloads if only specific callback signatures are needed;
ensure the implementations use the same return type (system::result<void>) and
reference the existing class members used by verify/password callbacks.
- Around line 16-18: The header is missing <initializer_list>, causing uses of
std::initializer_list in set_alpn to fail; add `#include` <initializer_list>
alongside the other includes at the top of the header so that set_alpn (which
takes std::initializer_list<std::string_view>) compiles correctly.
- Around line 604-605: The note referencing set_ciphersuites_tls13() is
misleading because that method isn't part of the public API; update the doc
comment in context.hpp to clarify that TLS 1.3 cipher configuration is
backend-dependent and that callers should use the public set_ciphersuites() or
backend-specific APIs instead of the non-existent set_ciphersuites_tls13();
mention the exact public symbol set_ciphersuites() and, if desired, add a short
remark that some backends may provide their own TLS1.3-specific APIs (name them
if you add backend-specific docs) rather than implying a universal
set_ciphersuites_tls13() function.
In `@include/boost/corosio/tls/tls_stream.hpp`:
- Around line 59-64: Move the stop-token pre-check from await_resume() into
await_suspend() for both handshake_awaitable and shutdown_awaitable: in
await_suspend() (for the coroutine awaiting the operation) immediately inspect
token_.stop_requested() and if true set ec_ =
make_error_code(system::errc::operation_canceled) and return the
coroutine_handle (h) so it resumes without starting the async operation; remove
the token_.stop_requested() check from await_resume() so it no longer overrides
real operation results; ensure you reference and update the members token_ and
ec_ used by handshake_awaitable and shutdown_awaitable and keep the rest of the
suspend/start logic intact.
In `@include/boost/corosio/tls/wolfssl_stream.hpp`:
- Around line 32-45: Update the example in wolfssl_stream.hpp to use the enum
constant defined on the base class: replace usage of wolfssl_stream::client with
tls_stream::client in the handshake call so it references the defining type
(tls_stream) instead of the derived class (wolfssl_stream); ensure any related
examples/comments reflect the same change to maintain clarity and consistency
with the tls_stream::handshake_type definition.
In `@src/openssl/src/openssl_stream.cpp`:
- Around line 99-105: The constructor openssl_native_context(context_data const&
cd) always creates a client-only SSL_CTX via SSL_CTX_new(TLS_client_method()),
which will break server-side handshakes (e.g., SSL_accept); change the logic to
choose the appropriate method based on the context role or create a generic
server/client-capable context: detect the intended role from context_data (or
add a role flag), and call SSL_CTX_new with TLS_server_method() for server
roles, TLS_client_method() for client roles, or TLS_method() for a generic
context; update the openssl_native_context constructor to branch on that role
and ensure subsequent uses of ctx_ (e.g., SSL_accept) match the created method.
- Around line 254-273: In flush_output(), BIO_read(ext_bio_) removes bytes but
the current code assumes s_.write_some writes them all; handle partial writes by
looping until the full n bytes read are written: after BIO_read into out_buf_
use a write-offset loop that repeatedly calls
s_.write_some(capy::mutable_buffer(out_buf_.data()+offset, remaining)) and
advances offset by the returned written count, returning any error immediately;
ensure the io_mutex_.scoped_lock covers the entire write-loop for that chunk
(use the existing io_mutex_.scoped_lock), and only call BIO_read again after the
whole chunk is successfully written. Reference symbols: flush_output(),
ext_bio_, out_buf_, io_mutex_.scoped_lock(), and s_.write_some().
- Around line 436-460: In do_write_some, add handling for SSL_ERROR_ZERO_RETURN
and SSL_ERROR_SYSCALL (with ERR_get_error()==0) the same way do_read_some does:
treat SSL_ERROR_ZERO_RETURN as EOF and treat SSL_ERROR_SYSCALL with no OpenSSL
error as EOF instead of converting ERR_get_error() into a generic system error;
otherwise keep the existing path for non-zero ERR_get_error() (use
ERR_get_error() to produce a system::error_code). Update the error branch after
SSL_get_error(ssl_, ret) in do_write_some to check for SSL_ERROR_ZERO_RETURN and
SSL_ERROR_SYSCALL first, map them to the EOF error_code, and only call
ERR_get_error() and create a system::error_code for other error cases. Ensure
references are to do_write_some, SSL_ERROR_ZERO_RETURN, SSL_ERROR_SYSCALL, and
ERR_get_error.
🧹 Nitpick comments (4)
src/corosio/src/tls/detail/context_impl.hpp (1)
100-116: Factory invoked under lock — consider implications for expensive operations.The
createfactory is called while holdingnative_contexts_mutex_. If backend context initialization (e.g., loading certificates, parsing keys) is expensive, this could cause contention. For typical use cases with few backends, this is acceptable, but consider documenting that the factory should be lightweight or refactoring to use double-checked locking if performance becomes a concern.test/unit/tls/wolfssl_stream.cpp (1)
18-24: Test scaffolding noted.The empty
run()method serves the stated purpose of verifying the header is self-contained (compiles independently). Consider adding actual unit tests forwolfssl_streamfunctionality in a follow-up, such as construction, handshake operations, and I/O operations, similar to the existing timer tests.Would you like me to help generate test cases for
wolfssl_streambased on the patterns in the TLS documentation?context/tls-context-design.md (1)
68-77: Add language identifiers to fenced code blocks.Per static analysis (MD040), fenced code blocks should have a language specified. For the bootstrap model diagram, use
textorplaintext; for the search terms list, usetext.📝 Suggested fix
-``` +```text Bootstrap connection: Trust store: system default-``` +```text portable TLS abstraction API SSL context wrapper libraryAlso applies to: 139-149
src/corosio/src/detail/win/sockets.hpp (1)
13-25: Add explicit include forcapy::any_bufref.The type
capy::any_bufref&is used directly in the method signatures at lines 133 and 141 (read_some()andwrite_some()). While the type is currently accessible through the transitive include from<boost/corosio/socket.hpp>, add an explicit#include <boost/capy/any_bufref.hpp>to make the dependency clear and avoid relying on transitive includes for directly-used types.
| // Set password callback before loading encrypted key | ||
| ctx.set_password_callback( | ||
| []( std::size_t max_len, tls::password_purpose purpose ) | ||
| { | ||
| // Read from environment or secret manager | ||
| return std::string( std::getenv( "TLS_KEY_PASSWORD" ) ); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "tls_context_examples.cpp" -type fRepository: cppalliance/corosio
Length of output: 97
🏁 Script executed:
sed -n '120,140p' ./example/tls_context_examples.cpp | cat -nRepository: cppalliance/corosio
Length of output: 896
🏁 Script executed:
rg -A 5 "password_callback" --type cppRepository: cppalliance/corosio
Length of output: 3866
🏁 Script executed:
rg -B 10 "set_password_callback" include/boost/corosio/tls/context.hpp | head -50Repository: cppalliance/corosio
Length of output: 1779
🏁 Script executed:
rg "set_password_callback" -A 20 include/boost/corosio/tls/context.hpp | tail -40Repository: cppalliance/corosio
Length of output: 1241
🏁 Script executed:
rg -A 30 "password_callback" src/corosio/src/tls/detail/context_impl.hpp | head -60Repository: cppalliance/corosio
Length of output: 1136
🏁 Script executed:
rg "password_callback" src/corosio/src/tls/ -A 5 | grep -A 10 "invoke\|call\|pem_password_callback"Repository: cppalliance/corosio
Length of output: 561
🏁 Script executed:
find src -name "*.cpp" -o -name "*.hpp" | xargs grep -l "password_callback" | head -5Repository: cppalliance/corosio
Length of output: 106
🏁 Script executed:
rg "password_callback\(" src/ -A 3 -B 3Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
rg "pem_password_cb|password_callback|PEM_CALLBACK" src/ --type cpp --type hppRepository: cppalliance/corosio
Length of output: 90
🏁 Script executed:
rg "pem_password_cb|password_callback" src/ -A 5 -B 2Repository: cppalliance/corosio
Length of output: 668
🏁 Script executed:
fd -e cpp -e c | xargs grep -l "PEM_read_PrivateKey\|pem_password" 2>/dev/null | head -5Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
rg "password_callback" . --type-add 'source:*.{cpp,c,h,hpp}' -t source -B 3 -A 8Repository: cppalliance/corosio
Length of output: 7641
🏁 Script executed:
find src -name "*.cpp" | head -20Repository: cppalliance/corosio
Length of output: 785
🏁 Script executed:
find src -name "*tls*" -type dRepository: cppalliance/corosio
Length of output: 82
🏁 Script executed:
ls -la src/corosio/src/tls/Repository: cppalliance/corosio
Length of output: 226
Guard against null getenv in the password callback.
std::getenv can return null, causing undefined behavior when constructing a std::string. Additionally, the max_len parameter must be respected per the API contract.
Proposed fix
- return std::string( std::getenv( "TLS_KEY_PASSWORD" ) );
+ if(const char* pw = std::getenv("TLS_KEY_PASSWORD"))
+ return std::string(pw).substr(0, max_len);
+ return std::string{};📝 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.
| // Set password callback before loading encrypted key | |
| ctx.set_password_callback( | |
| []( std::size_t max_len, tls::password_purpose purpose ) | |
| { | |
| // Read from environment or secret manager | |
| return std::string( std::getenv( "TLS_KEY_PASSWORD" ) ); | |
| }); | |
| // Set password callback before loading encrypted key | |
| ctx.set_password_callback( | |
| []( std::size_t max_len, tls::password_purpose purpose ) | |
| { | |
| // Read from environment or secret manager | |
| if(const char* pw = std::getenv("TLS_KEY_PASSWORD")) | |
| return std::string(pw).substr(0, max_len); | |
| return std::string{}; | |
| }); |
🤖 Prompt for AI Agents
In `@example/tls_context_examples.cpp` around lines 125 - 131, In the password
callback passed to ctx.set_password_callback, guard against std::getenv
returning null and enforce the max_len contract: retrieve the env var name
"TLS_KEY_PASSWORD", check for nullptr and use an empty string if null, and when
constructing the returned std::string truncate to at most max_len characters
(e.g., by taking a substring or copying only max_len bytes) before returning;
keep the lambda signature and use tls::password_purpose only for any conditional
logic if needed.
include/boost/corosio/tcp_server.hpp
Outdated
| worker_base* try_pop() noexcept | ||
| { | ||
| auto* w = idle_; | ||
| idle_ = w->next; | ||
| return w; | ||
| } |
There was a problem hiding this comment.
Null pointer dereference in try_pop() when no idle workers available.
The method dereferences idle_ unconditionally on line 276. If the idle list is empty (idle_ == nullptr), this causes undefined behavior. The method name try_pop suggests it may fail, so it should handle the empty case.
🐛 Proposed fix
worker_base* try_pop() noexcept
{
+ if (!idle_)
+ return nullptr;
auto* w = idle_;
idle_ = w->next;
return w;
}📝 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.
| worker_base* try_pop() noexcept | |
| { | |
| auto* w = idle_; | |
| idle_ = w->next; | |
| return w; | |
| } | |
| worker_base* try_pop() noexcept | |
| { | |
| if (!idle_) | |
| return nullptr; | |
| auto* w = idle_; | |
| idle_ = w->next; | |
| return w; | |
| } |
🤖 Prompt for AI Agents
In `@include/boost/corosio/tcp_server.hpp` around lines 273 - 278, The try_pop()
implementation dereferences idle_ without checking for nullptr, causing
undefined behavior when the idle list is empty; update worker_base* try_pop() to
first check if idle_ is nullptr and return nullptr in that case, otherwise store
idle_ in a local variable (e.g., w), advance idle_ to w->next, and return w,
preserving noexcept and using the existing worker_base type and idle_ member.
| #include <functional> | ||
| #include <memory> | ||
| #include <string_view> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "context.hpp" -o -name "*context.hpp" | grep -i tls | head -20Repository: cppalliance/corosio
Length of output: 102
🏁 Script executed:
git ls-files | grep -i "tls.*context"Repository: cppalliance/corosio
Length of output: 257
🏁 Script executed:
git ls-files | grep "context.hpp"Repository: cppalliance/corosio
Length of output: 137
🏁 Script executed:
wc -l include/boost/corosio/tls/context.hppRepository: cppalliance/corosio
Length of output: 104
🏁 Script executed:
cat -n include/boost/corosio/tls/context.hpp | head -50Repository: cppalliance/corosio
Length of output: 1701
🏁 Script executed:
grep -n "set_alpn\|initializer_list" include/boost/corosio/tls/context.hppRepository: cppalliance/corosio
Length of output: 192
🏁 Script executed:
rg "initializer_list" include/boost/corosio/tls/context.hpp -A 2 -B 2Repository: cppalliance/corosio
Length of output: 247
🏁 Script executed:
grep -n "^#include" include/boost/corosio/tls/context.hppRepository: cppalliance/corosio
Length of output: 222
Add <initializer_list> include for set_alpn.
set_alpn uses std::initializer_list<std::string_view> at line 630, but this header does not include <initializer_list>. Without it, code including this header will fail to compile.
🛠️ Suggested fix
`#include` <functional>
+#include <initializer_list>
`#include` <memory>
`#include` <string_view>📝 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.
| #include <functional> | |
| #include <memory> | |
| #include <string_view> | |
| `#include` <functional> | |
| `#include` <initializer_list> | |
| `#include` <memory> | |
| `#include` <string_view> |
🤖 Prompt for AI Agents
In `@include/boost/corosio/tls/context.hpp` around lines 16 - 18, The header is
missing <initializer_list>, causing uses of std::initializer_list in set_alpn to
fail; add `#include` <initializer_list> alongside the other includes at the top of
the header so that set_alpn (which takes
std::initializer_list<std::string_view>) compiles correctly.
| @note For TLS 1.3, use `set_ciphersuites_tls13()` on backends | ||
| that distinguish between TLS 1.2 and 1.3 cipher configuration. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Let's search more broadly for files with context.hpp
find . -name "context.hpp" -type f 2>/dev/null | head -20Repository: cppalliance/corosio
Length of output: 102
🏁 Script executed:
# If that doesn't work, let's search the entire directory structure
find . -name "*context*" -type f 2>/dev/null | head -20Repository: cppalliance/corosio
Length of output: 413
🏁 Script executed:
# Search for set_ciphersuites_tls13 in the entire repository
rg "set_ciphersuites_tls13"Repository: cppalliance/corosio
Length of output: 170
🏁 Script executed:
# Read the context.hpp file to understand the public API
wc -l ./include/boost/corosio/tls/context.hpp
# Read around lines 604-605
sed -n '600,615p' ./include/boost/corosio/tls/context.hpp
# Also search what's defined in this file for cipher methods
grep -n "cipher" ./include/boost/corosio/tls/context.hpp -iRepository: cppalliance/corosio
Length of output: 1401
Update misleading documentation note about non-existent TLS 1.3 API.
The note at lines 604-605 references set_ciphersuites_tls13(), but this method is not declared anywhere in the public API. The only public method for cipher configuration is set_ciphersuites() at line 608. Either document where set_ciphersuites_tls13() is available on specific backends, or clarify that TLS 1.3 cipher configuration is backend-dependent and may require using set_ciphersuites() or backend-specific APIs.
🤖 Prompt for AI Agents
In `@include/boost/corosio/tls/context.hpp` around lines 604 - 605, The note
referencing set_ciphersuites_tls13() is misleading because that method isn't
part of the public API; update the doc comment in context.hpp to clarify that
TLS 1.3 cipher configuration is backend-dependent and that callers should use
the public set_ciphersuites() or backend-specific APIs instead of the
non-existent set_ciphersuites_tls13(); mention the exact public symbol
set_ciphersuites() and, if desired, add a short remark that some backends may
provide their own TLS1.3-specific APIs (name them if you add backend-specific
docs) rather than implying a universal set_ciphersuites_tls13() function.
| template<typename Callback> | ||
| system::result<void> | ||
| set_verify_callback( Callback callback ); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's verify the file exists and understand its structure
if [ -f "include/boost/corosio/tls/context.hpp" ]; then
echo "=== File found. Searching for definitions and includes ==="
# Look for inline definitions
echo -e "\n=== Searching for set_verify_callback definitions ==="
rg -nP 'context::set_verify_callback\s*\(' -C2
echo -e "\n=== Searching for set_password_callback definitions ==="
rg -nP 'context::set_password_callback\s*\(' -C2
echo -e "\n=== Searching for .ipp includes in context.hpp ==="
rg -nP '#include\s+.*context.*ipp' -C2
echo -e "\n=== Searching for any .ipp includes in the file ==="
rg -nP '#include.*\.ipp' include/boost/corosio/tls/context.hpp
echo -e "\n=== Getting line range 696-705 from context.hpp ==="
sed -n '696,705p' include/boost/corosio/tls/context.hpp | cat -n
echo -e "\n=== Getting line range 858-865 from context.hpp ==="
sed -n '858,865p' include/boost/corosio/tls/context.hpp | cat -n
else
echo "File not found at include/boost/corosio/tls/context.hpp"
# Try to find it
find . -name "context.hpp" -path "*/tls/*" 2>/dev/null
fiRepository: cppalliance/corosio
Length of output: 1085
🏁 Script executed:
#!/bin/bash
# Search for any .ipp files in the tls directory
echo "=== Looking for .ipp files in tls directory ==="
find include/boost/corosio/tls -name "*.ipp" -o -name "*.hpp" | head -20
# Search for any definitions in the codebase (not just context)
echo -e "\n=== Searching for set_verify_callback implementations anywhere ==="
rg -nP 'set_verify_callback\s*\(' --type cpp --type hpp -A3 | head -50
echo -e "\n=== Searching for set_password_callback implementations anywhere ==="
rg -nP 'set_password_callback\s*\(' --type cpp --type hpp -A3 | head -50
# Check if there's a src directory with implementations
echo -e "\n=== Listing tls-related source files ==="
find . -path "*/tls/*" -name "*.cpp" -o -path "*/tls/*" -name "*.ipp" 2>/dev/null | head -20
# Check the end of the context.hpp file to see if something is included
echo -e "\n=== Last 30 lines of context.hpp to check for includes ==="
tail -n 30 include/boost/corosio/tls/context.hppRepository: cppalliance/corosio
Length of output: 1507
🏁 Script executed:
#!/bin/bash
# Look at the class definition and structure around these methods
echo "=== Lines 650-720 to see class context and set_verify_callback ==="
sed -n '650,720p' include/boost/corosio/tls/context.hpp | cat -n
echo -e "\n=== Lines 800-870 to see set_password_callback and class end ==="
sed -n '800,870p' include/boost/corosio/tls/context.hpp | cat -n
# Check if the class inherits from something
echo -e "\n=== Search for class definition ==="
rg -n 'class.*context' include/boost/corosio/tls/context.hpp
# Look for any template specialization or implementation elsewhere
echo -e "\n=== Checking if methods are defined inline with = delete or = default ==="
rg -n 'set_verify_callback|set_password_callback' include/boost/corosio/tls/context.hppRepository: cppalliance/corosio
Length of output: 6451
Provide definitions for template methods set_verify_callback and set_password_callback.
These template methods are declared at lines 698 and 860 but have no visible definitions. Templates require definitions available to all translation units. Without them, any instantiation with lambdas or specific callback types will fail to link. Either move the implementations inline in the header, provide an included .ipp file, or replace with non-template overloads.
🤖 Prompt for AI Agents
In `@include/boost/corosio/tls/context.hpp` around lines 696 - 699, The template
declarations for set_verify_callback and set_password_callback are missing
definitions, causing link errors when instantiated; add their implementations so
they are available to all translation units by either (a) defining
template<typename Callback> system::result<void> set_verify_callback(Callback
callback) and template<typename Callback> system::result<void>
set_password_callback(Callback callback) inline in the header near their
declarations, or (b) move their implementations to a companion .ipp and `#include`
it at the end of context.hpp, or (c) replace them with explicit non-template
overloads if only specific callback signatures are needed; ensure the
implementations use the same return type (system::result<void>) and reference
the existing class members used by verify/password callbacks.
| io_result<> await_resume() const noexcept | ||
| { | ||
| if(token_.stop_requested()) | ||
| return {make_error_code(system::errc::operation_canceled)}; | ||
| return {ec_}; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd include/boost/corosio/tls && wc -l tls_stream.hppRepository: cppalliance/corosio
Length of output: 81
🏁 Script executed:
head -n 140 include/boost/corosio/tls/tls_stream.hpp | tail -n 100Repository: cppalliance/corosio
Length of output: 2815
🏁 Script executed:
cat -n include/boost/corosio/tls/tls_stream.hpp | head -n 130 | tail -n 75Repository: cppalliance/corosio
Length of output: 2768
🏁 Script executed:
cat -n include/boost/corosio/tls/tls_stream.hpp | sed -n '40,140p'Repository: cppalliance/corosio
Length of output: 3529
🏁 Script executed:
cat -n include/boost/corosio/tls/tls_stream.hpp | sed -n '75,84p'Repository: cppalliance/corosio
Length of output: 492
🏁 Script executed:
cat -n include/boost/corosio/tls/tls_stream.hpp | sed -n '120,128p'Repository: cppalliance/corosio
Length of output: 466
Check pre-canceled stop tokens in await_suspend() to prevent masking operation results.
await_resume() checks token_.stop_requested() and returns operation_canceled if true. This can mask successful operation results if the token is canceled after the operation completes. Additionally, pre-canceled tokens are not short-circuited before starting the operation in await_suspend().
The fix moves the cancellation check to await_suspend() immediately after receiving the token. If pre-canceled, set ec_ to operation_canceled and return h to resume immediately without starting the operation. Remove the check from await_resume() to avoid overriding actual results.
Proposed fix for handshake_awaitable
io_result<> await_resume() const noexcept
{
- if(token_.stop_requested())
- return {make_error_code(system::errc::operation_canceled)};
return {ec_};
}
template<typename Ex>
auto await_suspend(
std::coroutine_handle<> h,
Ex const& ex,
std::stop_token token) -> std::coroutine_handle<>
{
token_ = std::move(token);
+ if(token_.stop_requested())
+ {
+ ec_ = make_error_code(system::errc::operation_canceled);
+ return h;
+ }
stream_.get().handshake(h, ex, type_, token_, &ec_);
return std::noop_coroutine();
}Proposed fix for shutdown_awaitable
io_result<> await_resume() const noexcept
{
- if(token_.stop_requested())
- return {make_error_code(system::errc::operation_canceled)};
return {ec_};
}
template<typename Ex>
auto await_suspend(
std::coroutine_handle<> h,
Ex const& ex,
std::stop_token token) -> std::coroutine_handle<>
{
token_ = std::move(token);
+ if(token_.stop_requested())
+ {
+ ec_ = make_error_code(system::errc::operation_canceled);
+ return h;
+ }
stream_.get().shutdown(h, ex, token_, &ec_);
return std::noop_coroutine();
}🤖 Prompt for AI Agents
In `@include/boost/corosio/tls/tls_stream.hpp` around lines 59 - 64, Move the
stop-token pre-check from await_resume() into await_suspend() for both
handshake_awaitable and shutdown_awaitable: in await_suspend() (for the
coroutine awaiting the operation) immediately inspect token_.stop_requested()
and if true set ec_ = make_error_code(system::errc::operation_canceled) and
return the coroutine_handle (h) so it resumes without starting the async
operation; remove the token_.stop_requested() check from await_resume() so it no
longer overrides real operation results; ensure you reference and update the
members token_ and ec_ used by handshake_awaitable and shutdown_awaitable and
keep the rest of the suspend/start logic intact.
| openssl_native_context( context_data const& cd ) | ||
| : ctx_( nullptr ) | ||
| { | ||
| // Create SSL_CTX for TLS client (auto-negotiate best version) | ||
| ctx_ = SSL_CTX_new( TLS_client_method() ); | ||
| if( !ctx_ ) | ||
| return; |
There was a problem hiding this comment.
SSL_CTX is client-only but server handshakes are supported.
TLS_client_method() is client-specific; server-side SSL_accept may fail unless the context is server-capable or role-specific. Consider using a generic method or selecting client/server based on context role. Please verify against the OpenSSL API expectations.
🔧 Possible fix (if generic context is desired)
- ctx_ = SSL_CTX_new( TLS_client_method() );
+ ctx_ = SSL_CTX_new( TLS_method() );📝 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_native_context( context_data const& cd ) | |
| : ctx_( nullptr ) | |
| { | |
| // Create SSL_CTX for TLS client (auto-negotiate best version) | |
| ctx_ = SSL_CTX_new( TLS_client_method() ); | |
| if( !ctx_ ) | |
| return; | |
| openssl_native_context( context_data const& cd ) | |
| : ctx_( nullptr ) | |
| { | |
| // Create SSL_CTX for TLS client (auto-negotiate best version) | |
| ctx_ = SSL_CTX_new( TLS_method() ); | |
| if( !ctx_ ) | |
| return; |
🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 99 - 105, The constructor
openssl_native_context(context_data const& cd) always creates a client-only
SSL_CTX via SSL_CTX_new(TLS_client_method()), which will break server-side
handshakes (e.g., SSL_accept); change the logic to choose the appropriate method
based on the context role or create a generic server/client-capable context:
detect the intended role from context_data (or add a role flag), and call
SSL_CTX_new with TLS_server_method() for server roles, TLS_client_method() for
client roles, or TLS_method() for a generic context; update the
openssl_native_context constructor to branch on that role and ensure subsequent
uses of ctx_ (e.g., SSL_accept) match the created method.
| capy::task<system::error_code> | ||
| flush_output() | ||
| { | ||
| while(BIO_ctrl_pending(ext_bio_) > 0) | ||
| { | ||
| int pending = static_cast<int>(BIO_ctrl_pending(ext_bio_)); | ||
| int to_read = (std::min)(pending, static_cast<int>(out_buf_.size())); | ||
| int n = BIO_read(ext_bio_, out_buf_.data(), to_read); | ||
| if(n <= 0) | ||
| break; | ||
|
|
||
| // Write to underlying stream | ||
| auto guard = co_await io_mutex_.scoped_lock(); | ||
| auto [ec, written] = co_await s_.write_some( | ||
| capy::mutable_buffer(out_buf_.data(), static_cast<std::size_t>(n))); | ||
| if(ec) | ||
| co_return ec; | ||
| } | ||
| co_return system::error_code{}; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the actual file to see the full context
wc -l src/openssl/src/openssl_stream.cppRepository: cppalliance/corosio
Length of output: 101
🏁 Script executed:
# Get the context around the specified lines
sed -n '240,280p' src/openssl/src/openssl_stream.cpp | cat -nRepository: cppalliance/corosio
Length of output: 1786
🏁 Script executed:
# Search for write_some usage patterns in the codebase to understand partial write handling
rg 'write_some' --context 3Repository: cppalliance/corosio
Length of output: 28806
🏁 Script executed:
# Check if there are other places where similar write patterns are handled
rg 'BIO_read' -A 5 -B 2Repository: cppalliance/corosio
Length of output: 1417
Handle partial writes when flushing BIO output.
The BIO_read() call removes bytes from the BIO immediately, but write_some() may write fewer bytes than requested. If fewer bytes are written than read from the BIO, the remainder is lost and cannot be recovered, corrupting the TLS record stream.
🔧 Proposed fix
- auto guard = co_await io_mutex_.scoped_lock();
- auto [ec, written] = co_await s_.write_some(
- capy::mutable_buffer(out_buf_.data(), static_cast<std::size_t>(n)));
- if(ec)
- co_return ec;
+ auto guard = co_await io_mutex_.scoped_lock();
+ std::size_t offset = 0;
+ while(offset < static_cast<std::size_t>(n))
+ {
+ auto [ec, written] = co_await s_.write_some(
+ capy::mutable_buffer(out_buf_.data() + offset,
+ static_cast<std::size_t>(n) - offset));
+ if(ec)
+ co_return ec;
+ offset += written;
+ }🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 254 - 273, In
flush_output(), BIO_read(ext_bio_) removes bytes but the current code assumes
s_.write_some writes them all; handle partial writes by looping until the full n
bytes read are written: after BIO_read into out_buf_ use a write-offset loop
that repeatedly calls s_.write_some(capy::mutable_buffer(out_buf_.data()+offset,
remaining)) and advances offset by the returned written count, returning any
error immediately; ensure the io_mutex_.scoped_lock covers the entire write-loop
for that chunk (use the existing io_mutex_.scoped_lock), and only call BIO_read
again after the whole chunk is successfully written. Reference symbols:
flush_output(), ext_bio_, out_buf_, io_mutex_.scoped_lock(), and
s_.write_some().
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/corosio/src/tcp_server.cpp (1)
67-73: Relies onawait_readyto guardtry_pop()null dereference.The
await_resumepath at line 72 callstry_pop()which dereferencesidle_unconditionally. This is safe only becauseawait_ready()(line 64) returns true whenidle_ != nullptr, meaning we skip suspension and hit this path. However, this coupling is fragile—ifawait_readylogic changes or iftry_popis called elsewhere, UB will occur.This is addressed by the fix suggested for
try_pop()in the header.include/boost/corosio/tcp_server.hpp (1)
268-272: Dispatcher required before modifying shared state in launcher destructor and guard_t.The destructor calls
srv_->push_sync(*w_)which unsynchronized modifies shared state (waiters_,wv_). Since the launcher destructor can run on any thread, this creates a data race when the executor thread concurrently accesses these members. Thepush_aw::await_suspendmethod demonstrates the correct pattern: dispatch toex_before touching shared state (see line 152:return self_.ex_.dispatch(h);with the comment "Dispatch to server's executor before touching shared state").The same issue also exists in the guard_t destructor (line 295) which similarly calls
push_syncsynchronously. Both destructors should post the operation to the executor instead of calling synchronously.src/wolfssl/src/wolfssl_stream.cpp (1)
943-949: Silent initialization failure can lead to null pointer dereference.As noted in the code comment,
construct()silently fails ifinit_ssl()returns an error, leavingimpl_null. Subsequent calls tohandshake()orshutdown()will dereference the null pointer throughtls_stream::get().Consider either:
- Throwing an exception on failure
- Storing the error for later retrieval
- Adding a validity check before operations
🤖 Fix all issues with AI agents
In `@include/boost/corosio/tls/tls_stream.hpp`:
- Around line 255-259: The get() helper currently does an unchecked static_cast
and dereferences impl_, which can be null if a derived ctor like
wolfssl_stream::construct returns early; change get() to validate impl_ before
casting (e.g., assert(impl_) or throw a clear exception / return an error) and
document the precondition that impl_ must be initialized before calling
tls_stream::handshake() or tls_stream::shutdown(); update callers or
constructors (e.g., wolfssl_stream::construct) to ensure they either initialize
impl_ on success or prevent handshake()/shutdown() calls on error.
- Around line 99-102: The await_ready() implementation in tls_stream's awaitable
incorrectly checks token_.stop_requested() before the token is initialized in
await_suspend(); update tls_stream::awaitable::await_ready() to unconditionally
return false (like handshake_awaitable) so the awaitable always defers readiness
to await_suspend(), ensuring the stop token check happens after the token is
set.
- Around line 54-57: await_ready() currently reads token_ before it's
initialized, so remove the token_ access from await_ready() (make it not depend
on token_ and simply return false/unconditional) and implement the
pre-cancellation short-circuit inside the await_suspend(std::coroutine_handle<>)
overload that receives the std::stop_token: store the provided token_ member,
then check token_.stop_requested() and handle immediate completion there (i.e.,
short-circuit/return true or resume as your await_suspend semantics require).
Update await_suspend(std::stop_token) (and any overload that stores token_) to
perform the cancellation check after assigning token_, and delete the
token_.stop_requested() call from await_ready().
In `@src/openssl/src/openssl_stream.cpp`:
- Around line 757-771: openssl_stream::construct currently discards errors from
openssl_stream_impl_::init_ssl and leaves impl_ null; change it to propagate the
error instead of silently returning: update openssl_stream::construct to return
a system::result<void> (or throw an exception) and, after creating
openssl_stream_impl_, check the result of init_ssl() and return/throw that error
if non-empty, deleting the impl on failure; only set impl_ = impl on success so
callers can detect construction failure via the returned error (or exception).
In `@src/wolfssl/src/wolfssl_stream.cpp`:
- Around line 139-146: The CA certificate loading loop using
wolfSSL_CTX_load_verify_buffer (iterating cd.ca_certificates and passing ctx_)
currently ignores return values; modify the loop to check each call's return and
handle failures (e.g., log the specific certificate index and error, and
return/propagate failure) similar to how entity certificate loading is handled:
after each wolfSSL_CTX_load_verify_buffer call, inspect its return value, call
wolfSSL_ERR_error_string or equivalent to capture details, and abort/propagate
so malformed CA certs don't silently disable peer verification.
- Around line 117-137: The calls to wolfSSL_CTX_use_certificate_buffer and
wolfSSL_CTX_use_PrivateKey_buffer in the certificate loading block ignore return
values; update the code to check each function's return (for the certificate
path using cd.entity_certificate and cd.entity_cert_format and for the key path
using cd.private_key and cd.private_key_format against ctx_), and on failure
either log the error with context (include which buffer failed and any available
wolfSSL error) and abort/fail context creation (e.g., return false or throw)
instead of proceeding so malformed certs/keys don't produce later handshake
failures.
♻️ Duplicate comments (11)
include/boost/corosio/tcp_server.hpp (1)
232-237: Null pointer dereference intry_pop()when idle list is empty.The method dereferences
idle_unconditionally on line 235. If the idle list is empty (idle_ == nullptr), this causes undefined behavior. While current call sites checkawait_ready()first, the method nametry_popimplies it should handle the empty case gracefully.🐛 Proposed fix
worker_base* try_pop() noexcept { + if (!idle_) + return nullptr; auto* w = idle_; idle_ = w->next; return w; }include/boost/corosio/tls/wolfssl_stream.hpp (1)
32-45: Consider usingtls_stream::clientin the example.The
handshake_typeenum is defined in the base classtls_stream. While the constant is inherited and accessible throughwolfssl_stream, referencing it via the defining class (tls_stream::client) would be clearer and more consistent with base class documentation.- co_await secure.handshake( wolfssl_stream::client ); + co_await secure.handshake( tls_stream::client );include/boost/corosio/tls/tls_stream.hpp (1)
59-64: Cancellation check inawait_resume()can override successful operation results.This issue was flagged in a past review. If
stop_requested()becomes true after the operation completes successfully,await_resume()returnsoperation_canceledinstead of the actual result. The cancellation check should be moved toawait_suspend()before starting the operation.src/openssl/src/openssl_stream.cpp (3)
99-105: SSL_CTX is client-only but server handshakes are supported.The context is created with
TLS_client_method(), butdo_handshake()at line 494 callsSSL_accept()for server-side handshakes. Server handshakes will fail because the SSL_CTX is client-specific.🔧 Suggested fix
- ctx_ = SSL_CTX_new( TLS_client_method() ); + ctx_ = SSL_CTX_new( TLS_method() );
254-273: Handle partial writes when flushing BIO output.
BIO_read()at line 261 removes bytes from the BIO, butwrite_some()at line 267-268 may write fewer bytes than requested. Any unwritten bytes are lost, corrupting the TLS record stream.🔧 Proposed fix
capy::task<system::error_code> flush_output() { while(BIO_ctrl_pending(ext_bio_) > 0) { int pending = static_cast<int>(BIO_ctrl_pending(ext_bio_)); int to_read = (std::min)(pending, static_cast<int>(out_buf_.size())); int n = BIO_read(ext_bio_, out_buf_.data(), to_read); if(n <= 0) break; // Write to underlying stream auto guard = co_await io_mutex_.scoped_lock(); - auto [ec, written] = co_await s_.write_some( - capy::mutable_buffer(out_buf_.data(), static_cast<std::size_t>(n))); - if(ec) - co_return ec; + std::size_t offset = 0; + while(offset < static_cast<std::size_t>(n)) + { + auto [ec, written] = co_await s_.write_some( + capy::mutable_buffer(out_buf_.data() + offset, + static_cast<std::size_t>(n) - offset)); + if(ec) + co_return ec; + offset += written; + } } co_return system::error_code{}; }
434-461: Add SSL_ERROR_ZERO_RETURN and SSL_ERROR_SYSCALL handling in write path.The
do_write_someerror handling is incomplete compared todo_read_some.SSL_ERROR_ZERO_RETURN(clean TLS shutdown) andSSL_ERROR_SYSCALLwithERR_get_error()==0(EOF) are not handled.🔧 Proposed fix
else { int err = SSL_get_error(ssl_, ret); if(err == SSL_ERROR_WANT_WRITE) { ec = co_await flush_output(); if(ec) goto done; } else if(err == SSL_ERROR_WANT_READ) { // Renegotiation - flush then read ec = co_await flush_output(); if(ec) goto done; ec = co_await read_input(); if(ec) goto done; } + else if(err == SSL_ERROR_ZERO_RETURN) + { + ec = make_error_code(capy::error::eof); + goto done; + } + else if(err == SSL_ERROR_SYSCALL) + { + unsigned long ssl_err = ERR_get_error(); + if(ssl_err == 0) + ec = make_error_code(capy::error::eof); + else + ec = system::error_code( + static_cast<int>(ssl_err), system::system_category()); + goto done; + } else { unsigned long ssl_err = ERR_get_error(); ec = system::error_code( static_cast<int>(ssl_err), system::system_category()); goto done; } }example/tls_context_examples.cpp (1)
125-131: Guard against nullstd::getenvin password callback.
std::getenv()can returnnullptr, causing undefined behavior when constructing astd::string. Themax_lenparameter should also be respected per the API contract.🔧 Proposed fix
ctx.set_password_callback( []( std::size_t max_len, tls::password_purpose purpose ) { // Read from environment or secret manager - return std::string( std::getenv( "TLS_KEY_PASSWORD" ) ); + if(const char* pw = std::getenv("TLS_KEY_PASSWORD")) + return std::string(pw).substr(0, max_len); + return std::string{}; });include/boost/corosio/tls/context.hpp (4)
16-18: Add<initializer_list>include forset_alpn.
set_alpn()at line 630 usesstd::initializer_list<std::string_view>, but<initializer_list>is not included. This will cause compilation failures for users including only this header.🔧 Suggested fix
`#include` <functional> +#include <initializer_list> `#include` <memory> `#include` <string_view>
604-606: Documentation references non-existentset_ciphersuites_tls13()API.The note mentions
set_ciphersuites_tls13()but this method doesn't exist in the public API. Either add the method or clarify that TLS 1.3 cipher configuration is backend-dependent.
696-699: Provide definitions for template methodset_verify_callback.This template method is declared but has no visible definition. Templates require definitions available to all translation units, or users will encounter linker errors when instantiating with lambdas.
858-860: Provide definitions for template methodset_password_callback.Same issue as
set_verify_callback- template declaration without definition will cause linker errors.
🧹 Nitpick comments (5)
include/boost/corosio/tcp_server.hpp (1)
138-159: Clarify the relationship betweenawait_suspendtemplate andawait_suspend_impl.The template
await_suspend(lines 147-153) dispatches directly toself_.ex_.dispatch(h)and ignores the passed executor parameter. Meanwhile,await_suspend_implis declared private but never referenced from the template. Ifawait_suspend_implis intended for the out-of-line implementation path, consider either documenting this split or removing the unused declaration to avoid confusion.src/corosio/src/tcp_server.cpp (2)
105-123: Silent continuation onpop()error may mask issues.When
pop()returns an error (line 112-113), the loop silently continues without logging or handling. Consider at minimum adding a debug log or counter to help diagnose issues if this path is hit unexpectedly.auto rv = co_await pop(); if(rv.has_error()) + { + // Consider logging: BOOST_COROSIO_LOG_DEBUG("pop() failed: ", rv.error()); continue; + }
125-132:bind()ignores actual error fromlisten().The TODO comment on line 129 notes this should return an error_code, but the function always returns an empty
error_code{}regardless of whetherlisten()succeeds. Iflisten()can fail, the error is silently discarded.Would you like me to help implement proper error propagation from
listen(), or open an issue to track this TODO?context/tls-context-design.md (2)
68-77: Add language specifier to fenced code block.Per markdownlint MD040, fenced code blocks should have a language specified. For this ASCII diagram, use
textorplaintext.📝 Suggested fix
-``` +```text Bootstrap connection: Trust store: system default
139-149: Add language specifier to fenced code block.Per markdownlint MD040, this search terms list should have a language specifier. Use
textfor plain text content.📝 Suggested fix
-``` +```text portable TLS abstraction API SSL context wrapper library
| bool await_ready() const noexcept | ||
| { | ||
| return token_.stop_requested(); | ||
| } |
There was a problem hiding this comment.
await_ready() accesses token_ before initialization.
await_ready() is called before await_suspend(), but token_ is only set in the await_suspend overload that receives a std::stop_token. At the time await_ready() executes, token_ is default-constructed (never-stopped), so this check will always return false and never short-circuit.
If the intent is to short-circuit on pre-canceled tokens, the check should be in await_suspend() after the token is received, as noted in the past review. Consider removing the check from await_ready() since it serves no purpose in the current design.
🔧 Suggested fix
bool await_ready() const noexcept
{
- return token_.stop_requested();
+ return false;
}🤖 Prompt for AI Agents
In `@include/boost/corosio/tls/tls_stream.hpp` around lines 54 - 57, await_ready()
currently reads token_ before it's initialized, so remove the token_ access from
await_ready() (make it not depend on token_ and simply return
false/unconditional) and implement the pre-cancellation short-circuit inside the
await_suspend(std::coroutine_handle<>) overload that receives the
std::stop_token: store the provided token_ member, then check
token_.stop_requested() and handle immediate completion there (i.e.,
short-circuit/return true or resume as your await_suspend semantics require).
Update await_suspend(std::stop_token) (and any overload that stores token_) to
perform the cancellation check after assigning token_, and delete the
token_.stop_requested() call from await_ready().
| bool await_ready() const noexcept | ||
| { | ||
| return token_.stop_requested(); | ||
| } |
There was a problem hiding this comment.
Same await_ready() issue as handshake_awaitable.
await_ready() checks token_.stop_requested() before the token is set in await_suspend(). This check is ineffective and should be replaced with return false;.
🔧 Suggested fix
bool await_ready() const noexcept
{
- return token_.stop_requested();
+ return false;
}📝 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.
| bool await_ready() const noexcept | |
| { | |
| return token_.stop_requested(); | |
| } | |
| bool await_ready() const noexcept | |
| { | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In `@include/boost/corosio/tls/tls_stream.hpp` around lines 99 - 102, The
await_ready() implementation in tls_stream's awaitable incorrectly checks
token_.stop_requested() before the token is initialized in await_suspend();
update tls_stream::awaitable::await_ready() to unconditionally return false
(like handshake_awaitable) so the awaitable always defers readiness to
await_suspend(), ensuring the stop token check happens after the token is set.
| private: | ||
| tls_stream_impl& get() const noexcept | ||
| { | ||
| return *static_cast<tls_stream_impl*>(impl_); | ||
| } |
There was a problem hiding this comment.
Unchecked cast assumes impl_ is always valid and correctly typed.
get() performs a static_cast to tls_stream_impl* without verifying that impl_ is non-null or actually points to a tls_stream_impl. If a derived class fails to initialize impl_ (as seen in wolfssl_stream::construct which can return early on error), calling handshake() or shutdown() would dereference a null pointer.
Consider adding an assertion or documenting the precondition.
🤖 Prompt for AI Agents
In `@include/boost/corosio/tls/tls_stream.hpp` around lines 255 - 259, The get()
helper currently does an unchecked static_cast and dereferences impl_, which can
be null if a derived ctor like wolfssl_stream::construct returns early; change
get() to validate impl_ before casting (e.g., assert(impl_) or throw a clear
exception / return an error) and document the precondition that impl_ must be
initialized before calling tls_stream::handshake() or tls_stream::shutdown();
update callers or constructors (e.g., wolfssl_stream::construct) to ensure they
either initialize impl_ on success or prevent handshake()/shutdown() calls on
error.
| void | ||
| openssl_stream:: | ||
| construct( tls::context ctx ) | ||
| { | ||
| 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.
Initialization errors are silently ignored.
If init_ssl() fails, impl_ is left as nullptr and the error is discarded. The caller has no way to detect that stream construction failed. Consider throwing or storing the error for later retrieval.
🔧 Possible approaches
- Throw on failure:
auto ec = impl->init_ssl();
if( ec )
{
delete impl;
- return;
+ throw system::system_error( ec );
}- Two-phase construction with a separate
construct()that returnssystem::result<void>:
system::result<void>
openssl_stream::
construct( tls::context ctx )
{
auto* impl = new openssl_stream_impl_( s_, std::move( ctx ) );
auto ec = impl->init_ssl();
if( ec )
{
delete impl;
return ec;
}
impl_ = impl;
return {};
}🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 757 - 771,
openssl_stream::construct currently discards errors from
openssl_stream_impl_::init_ssl and leaves impl_ null; change it to propagate the
error instead of silently returning: update openssl_stream::construct to return
a system::result<void> (or throw an exception) and, after creating
openssl_stream_impl_, check the result of init_ssl() and return/throw that error
if non-empty, deleting the impl on failure; only set impl_ = impl on success so
callers can detect construction failure via the returned error (or exception).
| // Apply certificates if provided | ||
| if( !cd.entity_certificate.empty() ) | ||
| { | ||
| int format = ( cd.entity_cert_format == file_format::pem ) | ||
| ? WOLFSSL_FILETYPE_PEM : WOLFSSL_FILETYPE_ASN1; | ||
| wolfSSL_CTX_use_certificate_buffer( ctx_, | ||
| reinterpret_cast<unsigned char const*>( cd.entity_certificate.data() ), | ||
| static_cast<long>( cd.entity_certificate.size() ), | ||
| format ); | ||
| } | ||
|
|
||
| // Apply private key if provided | ||
| if( !cd.private_key.empty() ) | ||
| { | ||
| int format = ( cd.private_key_format == file_format::pem ) | ||
| ? WOLFSSL_FILETYPE_PEM : WOLFSSL_FILETYPE_ASN1; | ||
| wolfSSL_CTX_use_PrivateKey_buffer( ctx_, | ||
| reinterpret_cast<unsigned char const*>( cd.private_key.data() ), | ||
| static_cast<long>( cd.private_key.size() ), | ||
| format ); | ||
| } |
There was a problem hiding this comment.
Certificate and private key loading errors are silently ignored.
wolfSSL_CTX_use_certificate_buffer and wolfSSL_CTX_use_PrivateKey_buffer return values are not checked. If the certificate or key data is malformed or incompatible, the context will be created but TLS handshakes will fail later with a less informative error.
Consider checking return values and either failing context creation or logging a warning.
🔧 Suggested fix
// Apply certificates if provided
if( !cd.entity_certificate.empty() )
{
int format = ( cd.entity_cert_format == file_format::pem )
? WOLFSSL_FILETYPE_PEM : WOLFSSL_FILETYPE_ASN1;
- wolfSSL_CTX_use_certificate_buffer( ctx_,
+ int ret = wolfSSL_CTX_use_certificate_buffer( ctx_,
reinterpret_cast<unsigned char const*>( cd.entity_certificate.data() ),
static_cast<long>( cd.entity_certificate.size() ),
format );
+ if( ret != WOLFSSL_SUCCESS )
+ {
+ wolfSSL_CTX_free( ctx_ );
+ ctx_ = nullptr;
+ return;
+ }
}
// Apply private key if provided
if( !cd.private_key.empty() )
{
int format = ( cd.private_key_format == file_format::pem )
? WOLFSSL_FILETYPE_PEM : WOLFSSL_FILETYPE_ASN1;
- wolfSSL_CTX_use_PrivateKey_buffer( ctx_,
+ int ret = wolfSSL_CTX_use_PrivateKey_buffer( ctx_,
reinterpret_cast<unsigned char const*>( cd.private_key.data() ),
static_cast<long>( cd.private_key.size() ),
format );
+ if( ret != WOLFSSL_SUCCESS )
+ {
+ wolfSSL_CTX_free( ctx_ );
+ ctx_ = nullptr;
+ return;
+ }
}📝 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 certificates if provided | |
| if( !cd.entity_certificate.empty() ) | |
| { | |
| int format = ( cd.entity_cert_format == file_format::pem ) | |
| ? WOLFSSL_FILETYPE_PEM : WOLFSSL_FILETYPE_ASN1; | |
| wolfSSL_CTX_use_certificate_buffer( ctx_, | |
| reinterpret_cast<unsigned char const*>( cd.entity_certificate.data() ), | |
| static_cast<long>( cd.entity_certificate.size() ), | |
| format ); | |
| } | |
| // Apply private key if provided | |
| if( !cd.private_key.empty() ) | |
| { | |
| int format = ( cd.private_key_format == file_format::pem ) | |
| ? WOLFSSL_FILETYPE_PEM : WOLFSSL_FILETYPE_ASN1; | |
| wolfSSL_CTX_use_PrivateKey_buffer( ctx_, | |
| reinterpret_cast<unsigned char const*>( cd.private_key.data() ), | |
| static_cast<long>( cd.private_key.size() ), | |
| format ); | |
| } | |
| // Apply certificates if provided | |
| if( !cd.entity_certificate.empty() ) | |
| { | |
| int format = ( cd.entity_cert_format == file_format::pem ) | |
| ? WOLFSSL_FILETYPE_PEM : WOLFSSL_FILETYPE_ASN1; | |
| int ret = wolfSSL_CTX_use_certificate_buffer( ctx_, | |
| reinterpret_cast<unsigned char const*>( cd.entity_certificate.data() ), | |
| static_cast<long>( cd.entity_certificate.size() ), | |
| format ); | |
| if( ret != WOLFSSL_SUCCESS ) | |
| { | |
| wolfSSL_CTX_free( ctx_ ); | |
| ctx_ = nullptr; | |
| return; | |
| } | |
| } | |
| // Apply private key if provided | |
| if( !cd.private_key.empty() ) | |
| { | |
| int format = ( cd.private_key_format == file_format::pem ) | |
| ? WOLFSSL_FILETYPE_PEM : WOLFSSL_FILETYPE_ASN1; | |
| int ret = wolfSSL_CTX_use_PrivateKey_buffer( ctx_, | |
| reinterpret_cast<unsigned char const*>( cd.private_key.data() ), | |
| static_cast<long>( cd.private_key.size() ), | |
| format ); | |
| if( ret != WOLFSSL_SUCCESS ) | |
| { | |
| wolfSSL_CTX_free( ctx_ ); | |
| ctx_ = nullptr; | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/wolfssl/src/wolfssl_stream.cpp` around lines 117 - 137, The calls to
wolfSSL_CTX_use_certificate_buffer and wolfSSL_CTX_use_PrivateKey_buffer in the
certificate loading block ignore return values; update the code to check each
function's return (for the certificate path using cd.entity_certificate and
cd.entity_cert_format and for the key path using cd.private_key and
cd.private_key_format against ctx_), and on failure either log the error with
context (include which buffer failed and any available wolfSSL error) and
abort/fail context creation (e.g., return false or throw) instead of proceeding
so malformed certs/keys don't produce later handshake failures.
| // Apply CA certificates for verification | ||
| for( auto const& ca : cd.ca_certificates ) | ||
| { | ||
| wolfSSL_CTX_load_verify_buffer( ctx_, | ||
| reinterpret_cast<unsigned char const*>( ca.data() ), | ||
| static_cast<long>( ca.size() ), | ||
| WOLFSSL_FILETYPE_PEM ); | ||
| } |
There was a problem hiding this comment.
CA certificate loading errors are silently ignored.
Similar to entity certificate loading, wolfSSL_CTX_load_verify_buffer return values are not checked. If a CA certificate is malformed, peer verification will fail unexpectedly.
🔧 Suggested fix
// Apply CA certificates for verification
for( auto const& ca : cd.ca_certificates )
{
- wolfSSL_CTX_load_verify_buffer( ctx_,
+ int ret = wolfSSL_CTX_load_verify_buffer( ctx_,
reinterpret_cast<unsigned char const*>( ca.data() ),
static_cast<long>( ca.size() ),
WOLFSSL_FILETYPE_PEM );
+ // Consider logging or failing on ret != WOLFSSL_SUCCESS
}📝 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 CA certificates for verification | |
| for( auto const& ca : cd.ca_certificates ) | |
| { | |
| wolfSSL_CTX_load_verify_buffer( ctx_, | |
| reinterpret_cast<unsigned char const*>( ca.data() ), | |
| static_cast<long>( ca.size() ), | |
| WOLFSSL_FILETYPE_PEM ); | |
| } | |
| // Apply CA certificates for verification | |
| for( auto const& ca : cd.ca_certificates ) | |
| { | |
| int ret = wolfSSL_CTX_load_verify_buffer( ctx_, | |
| reinterpret_cast<unsigned char const*>( ca.data() ), | |
| static_cast<long>( ca.size() ), | |
| WOLFSSL_FILETYPE_PEM ); | |
| // Consider logging or failing on ret != WOLFSSL_SUCCESS | |
| } |
🤖 Prompt for AI Agents
In `@src/wolfssl/src/wolfssl_stream.cpp` around lines 139 - 146, The CA
certificate loading loop using wolfSSL_CTX_load_verify_buffer (iterating
cd.ca_certificates and passing ctx_) currently ignores return values; modify the
loop to check each call's return and handle failures (e.g., log the specific
certificate index and error, and return/propagate failure) similar to how entity
certificate loading is handled: after each wolfSSL_CTX_load_verify_buffer call,
inspect its return value, call wolfSSL_ERR_error_string or equivalent to capture
details, and abort/propagate so malformed CA certs don't silently disable peer
verification.
There was a problem hiding this comment.
Actionable comments posted: 6
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)
943-952: Silent failure inconstruct()leaves stream in an unusable state.If
init_ssl()fails (e.g., due to certificate issues or WolfSSL initialization errors), the function returns without settingimpl_. The caller has no way to detect this failure, and any subsequent read/write/handshake operations on the stream will operate on a nullimpl_, likely causing undefined behavior or crashes.Consider one of:
- Throwing an exception with the error code
- Storing the error and providing an
is_open()orerror()accessor- Adding a factory function that returns
expected<wolfssl_stream, error_code>🔧 Suggested fix (throw on failure)
auto ec = impl->init_ssl(); if( ec ) { delete impl; - // For now, silently fail - could throw or store error - return; + throw system::system_error( ec, "wolfssl_stream initialization failed" ); } impl_ = impl; }
🤖 Fix all issues with AI agents
In `@context/tls-context-design.md`:
- Around line 68-77: The two fenced code blocks in tls-context-design.md (the
block with "Bootstrap connection..." and the later block listing TLS/SSL
phrases) lack language identifiers and trigger MD040; add a neutral language tag
(e.g., change ``` to ```text) to both fenced blocks so they become ```text ...
``` to satisfy markdownlint and keep formatting consistent.
In `@doc/modules/ROOT/pages/tutorials/tls-context.adoc`:
- Around line 396-400: The call to ctx.set_require_ocsp_staple() is inconsistent
with other setters that call .value() — update the usage to call
ctx.set_require_ocsp_staple().value() so failures are handled consistently;
locate the set_require_ocsp_staple method usage in the TLS context examples and
append .value() to the call to match the error-handling pattern used by other
setters.
- Around line 295-299: The call to ctx.set_hostname("api.example.com") is
inconsistent with other setters that call .value() to propagate errors; verify
whether tls::context::set_hostname returns system::result<void> and if so update
the example to call ctx.set_hostname("api.example.com").value(); otherwise, if
set_hostname is intentionally non-fallible, add a short comment explaining it is
noexcept/non-fallible to justify omitting .value(). Ensure you reference
set_hostname, .value(), and system::result<void> when making the change so the
pattern matches the rest of the doc.
In `@src/openssl/src/openssl_stream.cpp`:
- Around line 122-185: The code ignores return values from
SSL_CTX_use_certificate, SSL_CTX_use_PrivateKey, and X509_STORE_add_cert and
never validates the key/cert pairing; update the blocks that call
SSL_CTX_use_certificate, SSL_CTX_use_PrivateKey, and X509_STORE_add_cert to
check their return values, and on any failure free/null ctx_ (so init_ssl() can
surface the error) and return an error; after loading certificate and private
key call SSL_CTX_check_private_key and fail similarly if it returns false; apply
the same strict check pattern when adding each CA (use the return of
X509_STORE_add_cert) and ensure BIO/X509/EVP_PKEY are freed on failure to avoid
leaks.
- Around line 275-287: In read_input(), don't ignore BIO_write's return value:
check the int result from BIO_write(ext_bio_, in_buf_.data(),
static_cast<int>(n)) and loop to write remaining bytes until total bytes written
equals n; if BIO_write returns <= 0 treat it as a failure and return an
appropriate system::error_code (or map OpenSSL error) instead of dropping data,
and advance the buffer pointer by the bytes successfully written in repeated
calls.
In `@src/wolfssl/src/wolfssl_stream.cpp`:
- Around line 885-890: The current error path uses wolfSSL_get_error(nullptr, 0)
when native_ctx is null which may be meaningless; update the failure handling in
the cached context constructor (the branch where native_ctx == nullptr after
wolfSSL_CTX_new) to return a clear, dedicated error_code (e.g., a custom
context-creation error or a well-defined std::errc/system_category code) instead
of calling wolfSSL_get_error(nullptr, 0). Locate the native_ctx null-check in
wolfssl_stream.cpp and replace the wolfSSL_get_error call with a deterministic
error_code that signals "context creation failed" so callers can reliably detect
this condition.
♻️ Duplicate comments (4)
include/boost/corosio/tcp_server.hpp (1)
232-237: Null pointer dereference intry_pop()when idle list is empty.The method dereferences
idle_unconditionally on line 235. If the idle list is empty (idle_ == nullptr), this causes undefined behavior.🐛 Proposed fix
worker_base* try_pop() noexcept { + if (!idle_) + return nullptr; auto* w = idle_; idle_ = w->next; return w; }include/boost/corosio/tls/tls_stream.hpp (1)
54-64: Duplicate: stop-token handling still in await_ready/await_resume.Same concern as earlier review: pre-cancel handling should be in await_suspend to avoid masking completed results.
Also applies to: 99-109
src/wolfssl/src/wolfssl_stream.cpp (2)
117-137: Certificate and private key loading errors are silently ignored.This issue was already identified in a previous review. The return values of
wolfSSL_CTX_use_certificate_bufferandwolfSSL_CTX_use_PrivateKey_buffershould be checked to fail fast on malformed certificates or keys.
139-146: CA certificate loading errors are silently ignored.This issue was already identified in a previous review. The return value of
wolfSSL_CTX_load_verify_buffershould be checked in the loop.
🧹 Nitpick comments (2)
test/unit/tls/tls_stream.cpp (1)
18-26: Header self-containment test scaffold is appropriate.This ensures
tls_stream.hppcompiles independently. Consider adding actual test cases for the TLS stream functionality (handshake, shutdown, read/write through TLS) as the implementation matures.Would you like me to generate a TODO issue to track adding substantive TLS stream tests?
src/wolfssl/src/wolfssl_stream.cpp (1)
374-378: Redundant condition check.The check
if(total_read > 0)on line 376 is always true at this point since we're inside theif(ret > 0)block andtotal_readwas just incremented byret. The condition can be removed.♻️ Suggested simplification
total_read += static_cast<std::size_t>(ret); // For read_some semantics, return after first successful read - if(total_read > 0) - goto done; + goto done;
| ``` | ||
| Bootstrap connection: | ||
| Trust store: system default | ||
| Revocation: disabled or soft-fail | ||
| → Sufficient for OAuth flows, OCSP fetches, CRL downloads | ||
| Hardened connection (optional): | ||
| Trust store: system or custom | ||
| Revocation: enforced using pre-fetched data | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
markdownlint (MD040) flags the two fenced blocks without a language tag. Adding a neutral language (e.g., text) fixes the linting issue and improves consistency.
📝 Suggested fix
-```
+```text
Bootstrap connection:
Trust store: system default
Revocation: disabled or soft-fail
→ Sufficient for OAuth flows, OCSP fetches, CRL downloads
Hardened connection (optional):
Trust store: system or custom
Revocation: enforced using pre-fetched data
```diff
-```
+```text
portable TLS abstraction API
SSL context wrapper library
X.509 credential loading interface
cross-platform TLS configuration
backend-agnostic SSL library
TLS provider abstraction C++
certificate verification abstraction
OCSP stapling API design
CRL distribution points fetching
</details>
Also applies to: 139-149
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
68-68: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @context/tls-context-design.md around lines 68 - 77, The two fenced code
blocks in tls-context-design.md (the block with "Bootstrap connection..." and
the later block listing TLS/SSL phrases) lack language identifiers and trigger
MD040; add a neutral language tag (e.g., change totext) to both fenced
blocks so they become text ... to satisfy markdownlint and keep
formatting consistent.
</details>
<!-- fingerprinting:phantom:medusa:eagle -->
<!-- This is an auto-generated comment by CodeRabbit -->
| [source,cpp] | ||
| ---- | ||
| ctx.set_hostname( "api.example.com" ); | ||
| ---- | ||
|
|
There was a problem hiding this comment.
Inconsistent error handling pattern.
set_hostname() is called without .value() while other setters in the documentation use .value() to propagate errors. If set_hostname() returns system::result<void> like the others, add .value() for consistency. If it's intentionally non-fallible, consider adding a brief note explaining why.
🤖 Prompt for AI Agents
In `@doc/modules/ROOT/pages/tutorials/tls-context.adoc` around lines 295 - 299,
The call to ctx.set_hostname("api.example.com") is inconsistent with other
setters that call .value() to propagate errors; verify whether
tls::context::set_hostname returns system::result<void> and if so update the
example to call ctx.set_hostname("api.example.com").value(); otherwise, if
set_hostname is intentionally non-fallible, add a short comment explaining it is
noexcept/non-fallible to justify omitting .value(). Ensure you reference
set_hostname, .value(), and system::result<void> when making the change so the
pattern matches the rest of the doc.
| [source,cpp] | ||
| ---- | ||
| // Fail if server doesn't provide OCSP staple | ||
| ctx.set_require_ocsp_staple( true ); | ||
| ---- |
There was a problem hiding this comment.
Inconsistent error handling pattern.
set_require_ocsp_staple() is called without .value() while other setters use .value(). If this method can fail, add .value() for consistency with the rest of the tutorial.
🤖 Prompt for AI Agents
In `@doc/modules/ROOT/pages/tutorials/tls-context.adoc` around lines 396 - 400,
The call to ctx.set_require_ocsp_staple() is inconsistent with other setters
that call .value() — update the usage to call
ctx.set_require_ocsp_staple().value() so failures are handled consistently;
locate the set_require_ocsp_staple method usage in the TLS context examples and
append .value() to the call to match the error-handling pattern used by other
setters.
| // Apply certificates if provided | ||
| if( !cd.entity_certificate.empty() ) | ||
| { | ||
| BIO* bio = BIO_new_mem_buf( | ||
| cd.entity_certificate.data(), | ||
| static_cast<int>( cd.entity_certificate.size() ) ); | ||
| if( bio ) | ||
| { | ||
| X509* cert = nullptr; | ||
| if( cd.entity_cert_format == file_format::pem ) | ||
| cert = PEM_read_bio_X509( bio, nullptr, nullptr, nullptr ); | ||
| else | ||
| cert = d2i_X509_bio( bio, nullptr ); | ||
| if( cert ) | ||
| { | ||
| SSL_CTX_use_certificate( ctx_, cert ); | ||
| X509_free( cert ); | ||
| } | ||
| BIO_free( bio ); | ||
| } | ||
| } | ||
|
|
||
| // Apply private key if provided | ||
| if( !cd.private_key.empty() ) | ||
| { | ||
| BIO* bio = BIO_new_mem_buf( | ||
| cd.private_key.data(), | ||
| static_cast<int>( cd.private_key.size() ) ); | ||
| if( bio ) | ||
| { | ||
| EVP_PKEY* pkey = nullptr; | ||
| if( cd.private_key_format == file_format::pem ) | ||
| pkey = PEM_read_bio_PrivateKey( bio, nullptr, nullptr, nullptr ); | ||
| else | ||
| pkey = d2i_PrivateKey_bio( bio, nullptr ); | ||
| if( pkey ) | ||
| { | ||
| SSL_CTX_use_PrivateKey( ctx_, pkey ); | ||
| EVP_PKEY_free( pkey ); | ||
| } | ||
| BIO_free( bio ); | ||
| } | ||
| } | ||
|
|
||
| // Apply CA certificates for verification | ||
| X509_STORE* store = SSL_CTX_get_cert_store( ctx_ ); | ||
| for( auto const& ca : cd.ca_certificates ) | ||
| { | ||
| BIO* bio = BIO_new_mem_buf( ca.data(), static_cast<int>( ca.size() ) ); | ||
| if( bio ) | ||
| { | ||
| X509* cert = PEM_read_bio_X509( bio, nullptr, nullptr, nullptr ); | ||
| if( cert ) | ||
| { | ||
| X509_STORE_add_cert( store, cert ); | ||
| X509_free( cert ); | ||
| } | ||
| BIO_free( bio ); | ||
| } | ||
| } | ||
|
|
||
| // Apply verify depth | ||
| SSL_CTX_set_verify_depth( ctx_, cd.verify_depth ); | ||
| } |
There was a problem hiding this comment.
Handle OpenSSL configuration failures explicitly.
SSL_CTX_use_certificate, SSL_CTX_use_PrivateKey, and X509_STORE_add_cert return values are ignored. If any of these fail, the context is left partially configured and errors surface much later. Consider failing early by nulling/freeing ctx_ (so init_ssl() can surface the error) and also validating key/cert pairing via SSL_CTX_check_private_key.
🔧 Example pattern (apply similarly for key + CA loads)
- if( cert )
- {
- SSL_CTX_use_certificate( ctx_, cert );
- X509_free( cert );
- }
- BIO_free( bio );
+ if( cert )
+ {
+ if( SSL_CTX_use_certificate( ctx_, cert ) != 1 )
+ {
+ X509_free( cert );
+ BIO_free( bio );
+ SSL_CTX_free( ctx_ );
+ ctx_ = nullptr;
+ return;
+ }
+ X509_free( cert );
+ }
+ BIO_free( bio );🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 122 - 185, The code ignores
return values from SSL_CTX_use_certificate, SSL_CTX_use_PrivateKey, and
X509_STORE_add_cert and never validates the key/cert pairing; update the blocks
that call SSL_CTX_use_certificate, SSL_CTX_use_PrivateKey, and
X509_STORE_add_cert to check their return values, and on any failure free/null
ctx_ (so init_ssl() can surface the error) and return an error; after loading
certificate and private key call SSL_CTX_check_private_key and fail similarly if
it returns false; apply the same strict check pattern when adding each CA (use
the return of X509_STORE_add_cert) and ensure BIO/X509/EVP_PKEY are freed on
failure to avoid leaks.
| capy::task<system::error_code> | ||
| read_input() | ||
| { | ||
| auto guard = co_await io_mutex_.scoped_lock(); | ||
| auto [ec, n] = co_await s_.read_some( | ||
| capy::mutable_buffer(in_buf_.data(), in_buf_.size())); | ||
| if(ec) | ||
| co_return ec; | ||
|
|
||
| // Feed data into OpenSSL | ||
| int written = BIO_write(ext_bio_, in_buf_.data(), static_cast<int>(n)); | ||
| (void)written; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the file
find . -name "openssl_stream.cpp" -type fRepository: cppalliance/corosio
Length of output: 99
🏁 Script executed:
# Read the relevant section of the file
cat -n src/openssl/src/openssl_stream.cpp | sed -n '270,295p'Repository: cppalliance/corosio
Length of output: 1043
🏁 Script executed:
# Check the broader context to understand the function better
cat -n src/openssl/src/openssl_stream.cpp | sed -n '260,300p'Repository: cppalliance/corosio
Length of output: 1768
🏁 Script executed:
# Search for other BIO_write usages in the codebase
rg "BIO_write" -A 2 -B 2Repository: cppalliance/corosio
Length of output: 1258
🏁 Script executed:
# Check how ext_bio_ is initialized and configured
rg "ext_bio_" -B 3 -A 3 | head -60Repository: cppalliance/corosio
Length of output: 3898
🏁 Script executed:
# Find buffer initialization
rg "in_buf_" -B 2 -A 2 | head -40Repository: cppalliance/corosio
Length of output: 2592
🏁 Script executed:
# Check the constructor and initialization
cat -n src/openssl/src/openssl_stream.cpp | sed -n '1,150p' | grep -A 50 "openssl_stream_impl_"Repository: cppalliance/corosio
Length of output: 1916
🏁 Script executed:
# Search for any error handling patterns with BIO operations in general
rg "BIO_read|BIO_write|BIO_ctrl_pending" -B 1 -A 1Repository: cppalliance/corosio
Length of output: 1708
🏁 Script executed:
# Check if there are other places where write results are checked
rg "BIO_write" -B 3 -A 3 --context=5Repository: cppalliance/corosio
Length of output: 2526
🏁 Script executed:
# Find BIO pair creation and initialization
cat -n src/openssl/src/openssl_stream.cpp | sed -n '200,250p'Repository: cppalliance/corosio
Length of output: 1842
🏁 Script executed:
# Check if there are any test cases that might show expected behavior
find . -name "*.cpp" -o -name "*.hpp" | xargs grep -l "read_input\|openssl_stream" | head -5Repository: cppalliance/corosio
Length of output: 189
🏁 Script executed:
# Look for any comments or documentation about BIO buffer sizes
rg "BIO_new_bio_pair|default_buffer_size" -B 3 -A 3Repository: cppalliance/corosio
Length of output: 2574
🏁 Script executed:
# Check the OpenSSL documentation/behavior for BIO pairs with default sizes
# Also check if there's any conditional compilation or platform-specific code
cat -n src/openssl/src/openssl_stream.cpp | sed -n '175,215p'Repository: cppalliance/corosio
Length of output: 1291
🏁 Script executed:
# Look for error handling patterns with SSL/BIO operations throughout the file
rg "ERR_get_error|SSL_ERROR|if.*<=" src/openssl/src/openssl_stream.cpp | head -20Repository: cppalliance/corosio
Length of output: 1241
🏁 Script executed:
# Check the full read_input and adjacent functions to understand the broader pattern
cat -n src/openssl/src/openssl_stream.cpp | sed -n '250,290p'Repository: cppalliance/corosio
Length of output: 1795
🏁 Script executed:
# Check if there are other BIO operations in the codebase that handle errors
rg "BIO_read|BIO_write" -B 2 -A 2 src/openssl/Repository: cppalliance/corosio
Length of output: 1775
🏁 Script executed:
# Let's see the full context of how read_input is called to understand the flow
rg "read_input()" -B 5 -A 5Repository: cppalliance/corosio
Length of output: 4647
🏁 Script executed:
# Check if there's any documentation about the expected usage
head -100 src/openssl/src/openssl_stream.cppRepository: cppalliance/corosio
Length of output: 3249
🏁 Script executed:
# Let's research BIO_write semantics and behavior with BIO pairs
# Check OpenSSL documentation behavior
cat > /tmp/check_bio_semantics.py << 'EOF'
# According to OpenSSL documentation:
# BIO_write returns:
# - >0: number of bytes written (may be less than n)
# - <=0: error or no bytes written (0 means buffer full)
# This applies to all BIOs including memory BIOs in pairs
# The question is: can a BIO_pair's external BIO fill up?
# Answer: Yes! If the internal side doesn't drain it fast enough
print("OpenSSL BIO_write behavior:")
print("- Can return 0 to n-1 (short write)")
print("- Can return <=0 (error/full buffer)")
print("- BIO pairs CAN fill up if data isn't drained")
print("- Ignoring short write = data loss")
EOF
python /tmp/check_bio_semantics.pyRepository: cppalliance/corosio
Length of output: 149
🏁 Script executed:
# Check if there are any comments about this being intentional or safe
rg "BIO_write|written" -B 5 -A 5 src/openssl/src/openssl_stream.cpp | grep -A 10 "Feed data"Repository: cppalliance/corosio
Length of output: 418
🏁 Script executed:
# Look at the broader usage to see if there's any recovery mechanism
cat -n src/openssl/src/openssl_stream.cpp | sed -n '275,295p'Repository: cppalliance/corosio
Length of output: 916
Check BIO_write result in read_input to prevent silent data loss.
BIO_write can short-write or fail; ignoring its return value silently drops bytes and can desynchronize TLS state. Loop until all bytes are written or return an error if BIO_write fails.
🧩 Suggested fix
- int written = BIO_write(ext_bio_, in_buf_.data(), static_cast<int>(n));
- (void)written;
+ std::size_t off = 0;
+ while(off < n)
+ {
+ int written = BIO_write(
+ ext_bio_,
+ in_buf_.data() + off,
+ static_cast<int>(n - off));
+ if(written <= 0)
+ {
+ unsigned long ssl_err = ERR_get_error();
+ co_return ssl_err
+ ? system::error_code(
+ static_cast<int>(ssl_err), system::system_category())
+ : make_error_code(system::errc::io_error);
+ }
+ off += static_cast<std::size_t>(written);
+ }🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 275 - 287, In read_input(),
don't ignore BIO_write's return value: check the int result from
BIO_write(ext_bio_, in_buf_.data(), static_cast<int>(n)) and loop to write
remaining bytes until total bytes written equals n; if BIO_write returns <= 0
treat it as a failure and return an appropriate system::error_code (or map
OpenSSL error) instead of dropping data, and advance the buffer pointer by the
bytes successfully written in repeated calls.
| if( !native_ctx ) | ||
| { | ||
| return system::error_code( | ||
| wolfSSL_get_error(nullptr, 0), | ||
| system::system_category()); | ||
| wolfSSL_get_error( nullptr, 0 ), | ||
| system::system_category() ); | ||
| } |
There was a problem hiding this comment.
wolfSSL_get_error(nullptr, 0) may not return a meaningful error code.
When native_ctx is nullptr (due to wolfSSL_CTX_new failure in the cached context constructor), calling wolfSSL_get_error(nullptr, 0) may return 0 or an undefined value since the error isn't associated with an SSL object. Consider using a dedicated error code for context creation failure.
🔧 Suggested fix
WOLFSSL_CTX* native_ctx = tls::detail::get_wolfssl_context( impl );
if( !native_ctx )
{
- return system::error_code(
- wolfSSL_get_error( nullptr, 0 ),
- system::system_category() );
+ // Context creation failed - return a meaningful error
+ return system::error_code(
+ static_cast<int>(system::errc::operation_not_permitted),
+ system::generic_category() );
}📝 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( !native_ctx ) | |
| { | |
| return system::error_code( | |
| wolfSSL_get_error(nullptr, 0), | |
| system::system_category()); | |
| wolfSSL_get_error( nullptr, 0 ), | |
| system::system_category() ); | |
| } | |
| if( !native_ctx ) | |
| { | |
| // Context creation failed - return a meaningful error | |
| return system::error_code( | |
| static_cast<int>(system::errc::operation_not_permitted), | |
| system::generic_category() ); | |
| } |
🤖 Prompt for AI Agents
In `@src/wolfssl/src/wolfssl_stream.cpp` around lines 885 - 890, The current error
path uses wolfSSL_get_error(nullptr, 0) when native_ctx is null which may be
meaningless; update the failure handling in the cached context constructor (the
branch where native_ctx == nullptr after wolfSSL_CTX_new) to return a clear,
dedicated error_code (e.g., a custom context-creation error or a well-defined
std::errc/system_category code) instead of calling wolfSSL_get_error(nullptr,
0). Locate the native_ctx null-check in wolfssl_stream.cpp and replace the
wolfSSL_get_error call with a deterministic error_code that signals "context
creation failed" so callers can reliably detect this condition.
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 (3)
src/corosio/src/tcp_server.cpp (1)
59-73:pop_aw::await_resumemay dereference null fromtry_pop()whenawait_readywas false but no waiter was set.If
await_ready()returnsfalse(no idle workers) and the coroutine suspends,wait_.wwill be set bypush_aw::await_resumewhen a worker becomes available. However, ifawait_ready()returnstrue,try_pop()is called on line 72.Given the
try_pop()bug in the header (null dereference), this path could return a dereferenced null. Oncetry_pop()is fixed to returnnullptron empty, this code should also handle that case:🐛 Proposed fix (after try_pop is fixed)
system::result<tcp_server::worker_base&> tcp_server::pop_aw::await_resume() noexcept { if(wait_.w) return *wait_.w; - return *self_.wv_.try_pop(); + auto* w = self_.wv_.try_pop(); + if(!w) + return system::error_code{/* appropriate error */}; + return *w; }include/boost/corosio/tcp_server.hpp (1)
147-159: Remove unusedawait_suspend_impldeclaration frompush_awclass.The
await_suspend_implprivate method declared at line 158 has no implementation in the cpp file and is never called from anywhere in the codebase. The templateawait_suspendmethod (lines 147-153) dispatches directly viaself_.ex_.dispatch(h)without invoking it, making this dead code.src/wolfssl/src/wolfssl_stream.cpp (1)
348-354:current_op_is overwritten when read/write operations execute concurrently.
current_op_is a single shared pointer used by read, write, handshake, and shutdown operations. When these execute concurrently (as the API supports), one operation's localopstack variable gets overwritten in the shared pointer. Callbacks then access the wrong operation's buffer context, causing corrupted I/O or data races. The existingio_mutex_only guards underlying socket I/O, not TLS state.Separate
current_read_op_andcurrent_write_op_per-operation direction, or guard TLS operations with a mutex.
♻️ Duplicate comments (14)
include/boost/corosio/tcp_server.hpp (1)
232-237: Null pointer dereference intry_pop()when idle list is empty.This issue was previously flagged. The method dereferences
idle_on line 235 without checking if it's null. Whenidle_ == nullptr, this causes undefined behavior.🐛 Proposed fix
worker_base* try_pop() noexcept { + if (!idle_) + return nullptr; auto* w = idle_; idle_ = w->next; return w; }example/tls_context_examples.cpp (1)
125-131: Guard against nullstd::getenvreturn value.
std::getenv("TLS_KEY_PASSWORD")may returnnullptr, causing undefined behavior when constructingstd::string. Additionally, themax_lenparameter should be respected per the API contract.context/tls-context-design.md (1)
68-77: Add language identifiers to fenced code blocks.markdownlint (MD040) flags these fenced blocks without a language tag. Adding
textfixes the linting issue.src/wolfssl/src/wolfssl_stream.cpp (5)
942-948: Propagateinit_ssl()failures instead of silently returning.Construction failures are swallowed, leaving
impl_null with no signal to callers. Prefer returning an error or throwing.🔧 Example approach (throw on failure)
- if( ec ) - { - delete impl; - // For now, silently fail - could throw or store error - return; - } + if( ec ) + { + delete impl; + throw system::system_error( ec ); + }
104-105: Use a server-capable WolfSSL method (or select by role).Line 105 creates a client-only
WOLFSSL_CTX, but the stream supportswolfSSL_acceptfor server handshakes. Consider a generic method or choosing client/server by role.🔧 Possible direction (verify API availability)
- ctx_ = wolfSSL_CTX_new( wolfTLS_client_method() ); + ctx_ = wolfSSL_CTX_new( wolfTLS_method() );
117-137: Check return values when loading certificate and private key.The return values of
wolfSSL_CTX_use_certificate_bufferandwolfSSL_CTX_use_PrivateKey_bufferare ignored; failures will surface much later during handshakes.🔧 Suggested fix
- wolfSSL_CTX_use_certificate_buffer( ctx_, + int ret = wolfSSL_CTX_use_certificate_buffer( ctx_, reinterpret_cast<unsigned char const*>( cd.entity_certificate.data() ), static_cast<long>( cd.entity_certificate.size() ), format ); + if( ret != WOLFSSL_SUCCESS ) + { + wolfSSL_CTX_free( ctx_ ); + ctx_ = nullptr; + return; + }
139-146: Handle CA load failures explicitly.
wolfSSL_CTX_load_verify_bufferreturn values are ignored; malformed CA certs can silently disable verification.🔧 Suggested fix
- wolfSSL_CTX_load_verify_buffer( ctx_, + int ret = wolfSSL_CTX_load_verify_buffer( ctx_, reinterpret_cast<unsigned char const*>( ca.data() ), static_cast<long>( ca.size() ), WOLFSSL_FILETYPE_PEM ); + if( ret != WOLFSSL_SUCCESS ) + { + wolfSSL_CTX_free( ctx_ ); + ctx_ = nullptr; + return; + }
882-889: AvoidwolfSSL_get_error(nullptr, 0)when context creation fails.When
native_ctxis null,wolfSSL_get_error(nullptr, 0)isn’t meaningful. Return a deterministic error code for “context creation failed.”🔧 Suggested fix
- return system::error_code( - wolfSSL_get_error( nullptr, 0 ), - system::system_category() ); + return system::error_code( + static_cast<int>(system::errc::operation_not_permitted), + system::generic_category() );src/openssl/src/openssl_stream.cpp (6)
102-104: Use a server-capable TLS method (or select by role).Line 103 always creates a client-only
SSL_CTX, but the stream supportsSSL_acceptin server mode; this can break server handshakes. Use a generic method or select client/server by role.🔧 Proposed fix (generic context)
- ctx_ = SSL_CTX_new( TLS_client_method() ); + ctx_ = SSL_CTX_new( TLS_method() );
122-180: Fail early on certificate/private-key/CA load errors.Return values from
SSL_CTX_use_certificate,SSL_CTX_use_PrivateKey, andX509_STORE_add_certare ignored, and the key/cert pair isn’t validated. This can leave a partially configured context and defer failures to handshake time.🔧 Example pattern (apply to cert/key/CA blocks)
- if( cert ) - { - SSL_CTX_use_certificate( ctx_, cert ); - X509_free( cert ); - } + if( cert ) + { + if( SSL_CTX_use_certificate( ctx_, cert ) != 1 ) + { + X509_free( cert ); + BIO_free( bio ); + SSL_CTX_free( ctx_ ); + ctx_ = nullptr; + return; + } + X509_free( cert ); + }+ if( SSL_CTX_check_private_key( ctx_ ) != 1 ) + { + SSL_CTX_free( ctx_ ); + ctx_ = nullptr; + return; + }
254-271: Handle partial writes when flushing BIO output.
BIO_read()consumes bytes immediately, butwrite_some()may short-write; the remainder is currently dropped. Loop until all bytes are written.🔧 Proposed fix
- auto guard = co_await io_mutex_.scoped_lock(); - auto [ec, written] = co_await s_.write_some( - capy::mutable_buffer(out_buf_.data(), static_cast<std::size_t>(n))); - if(ec) - co_return ec; + auto guard = co_await io_mutex_.scoped_lock(); + std::size_t offset = 0; + while(offset < static_cast<std::size_t>(n)) + { + auto [ec, written] = co_await s_.write_some( + capy::mutable_buffer(out_buf_.data() + offset, + static_cast<std::size_t>(n) - offset)); + if(ec) + co_return ec; + offset += written; + }
275-286: CheckBIO_writeresult inread_input()to avoid data loss.
BIO_write()can short-write or fail. Ignoring its return value can drop bytes and corrupt the TLS stream.🔧 Proposed fix
- int written = BIO_write(ext_bio_, in_buf_.data(), static_cast<int>(n)); - (void)written; + std::size_t off = 0; + while(off < n) + { + int written = BIO_write( + ext_bio_, + in_buf_.data() + off, + static_cast<int>(n - off)); + if(written <= 0) + { + unsigned long ssl_err = ERR_get_error(); + co_return ssl_err + ? system::error_code( + static_cast<int>(ssl_err), system::system_category()) + : make_error_code(system::errc::io_error); + } + off += static_cast<std::size_t>(written); + }
436-460: Mirror read-side EOF/SYSCALL handling in the write path.
do_write_some()lacks theSSL_ERROR_ZERO_RETURNandSSL_ERROR_SYSCALLhandling present indo_read_some, so clean shutdowns can surface as generic errors.🔧 Proposed fix
+ else if(err == SSL_ERROR_ZERO_RETURN) + { + ec = make_error_code(capy::error::eof); + goto done; + } + else if(err == SSL_ERROR_SYSCALL) + { + unsigned long ssl_err = ERR_get_error(); + if(ssl_err == 0) + ec = make_error_code(capy::error::eof); + else + ec = system::error_code( + static_cast<int>(ssl_err), system::system_category()); + goto done; + } else { unsigned long ssl_err = ERR_get_error(); ec = system::error_code( static_cast<int>(ssl_err), system::system_category()); goto done; }
757-767: Propagateinit_ssl()failures fromconstruct().Errors are silently swallowed, leaving
impl_null with no signal to callers. Consider returning asystem::result<void>or throwing.🔧 Example approach (throw on failure)
- if( ec ) - { - delete impl; - return; - } + if( ec ) + { + delete impl; + throw system::system_error( ec ); + }
🧹 Nitpick comments (2)
src/corosio/src/tcp_server.cpp (1)
125-132:bind()ignores error fromlisten()- TODO comment acknowledges this.The comment on line 129 indicates this should return an error code, but currently it always returns an empty
error_code. This is a known limitation per the TODO.Consider tracking this as a follow-up issue to properly propagate the error.
src/corosio/src/tls/detail/context_impl.hpp (1)
40-128: Consider makingcontext_datanon-copyable/non-movable.
context_datacontains astd::mutexand raw pointers to cached native contexts. Default copy/move operations would be problematic:
std::mutexis non-copyable/non-movable- Copying the intrusive list pointers would lead to double-free in destructors
If
tls::contextuses shared ownership (as suggested bydemonstrate_sharing()in the examples), this is likely handled at thecontextlevel viashared_ptr<impl>. However, explicitly deleting copy/move operations oncontext_datawould make the intent clearer and prevent accidental misuse.♻️ Suggested improvement
struct context_data { + context_data() = default; + context_data(context_data const&) = delete; + context_data& operator=(context_data const&) = delete; + context_data(context_data&&) = delete; + context_data& operator=(context_data&&) = delete; + //-------------------------------------------- // Credentials
2 participants
Summary by CodeRabbit
New Features
Bug Fixes / Breaking Changes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.