work #108
work #108
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (38)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the
📝 WalkthroughWalkthroughThis pull request updates copyright notices across the codebase from an obfuscated email format to standard format, and introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
🤖 Fix all issues with AI agents
In `@example/echo-server/echo_server.cpp`:
- Around line 56-57: The loop that reads into buf_ must resume checking for
zero-length reads to avoid a busy spin; restore the original guard so that after
calling read_some (the call that yields ec and n) you break when either ec is
set or n == 0. Update the conditional around the read result in the function
handling the socket read (the read_some return handling that currently only
tests ec) to include "|| n == 0" so zero-byte reads cause the loop to exit and
prevent repeated zero-length write/read cycles.
In `@include/boost/corosio/endpoint.hpp`:
- Line 2: The file header in include/boost/corosio/endpoint.hpp has the
copyright year 2026 which is inconsistent with other files in this PR that use
2025; update the copyright line in endpoint.hpp to use 2025 (or confirm and
leave as 2026 if the change was intentional) so it matches the rest of the
project; locate the top-of-file comment in endpoint.hpp (the file/identifier:
endpoint.hpp, namespace corosio is present) and change the year token from
"2026" to "2025".
In `@include/boost/corosio/openssl_stream.hpp`:
- Around line 86-92: The openssl_stream constructor currently assigns impl_ =
make_impl(...) without checking for nullptr, which can happen when init_ssl()
fails; update the constructor (the template overload of openssl_stream taking S
stream and tls_context ctx) to validate the returned pointer from
make_impl(stream_, ctx) and throw a suitable exception (e.g., std::runtime_error
or a domain-specific error) if make_impl returns nullptr so subsequent methods
(handshake, read, write, reset) cannot dereference a null impl_; reference
make_impl, impl_, and init_ssl() when implementing the check.
In `@src/corosio/src/timer.cpp`:
- Around line 10-13: Add a high-level /* */ overview comment immediately after
the includes in src/corosio/src/timer.cpp that describes how the timer
implementation works (e.g., interaction between boost::corosio::timer, its
scheduling/expiry mechanism, threading/strand assumptions, how callbacks are
enqueued/executed, and any subtle lifetime or cancellation behavior). Place this
paragraph right after the existing `#include` lines and reference the primary
symbols in the file (such as the timer class/constructors, start/stop/expiry
handlers, and any helper functions) so future readers can quickly understand
non-obvious implementation details.
In `@src/openssl/src/openssl_stream.cpp`:
- Around line 305-328: The reset() method currently ignores SSL_clear()'s return
value; change impl::reset() to return a std::error_code (or similar error
indicator), call SSL_clear(ssl_) and if it returns 0 convert the OpenSSL error
(ERR_get_error / ERR_reason_error_string) into a meaningful std::error_code and
return it immediately (before draining ext_bio_ or reapplying hostname),
otherwise continue and return success; then update callers (notably
do_handshake() and the public wrapper that invokes impl::reset()) to check the
returned std::error_code and abort/propagate the error instead of proceeding
with SSL operations when reset fails.
🧹 Nitpick comments (4)
include/boost/corosio/tls_stream.hpp (1)
127-151: Well-documentedreset()addition to the TLS stream interface.The docstring covers purpose, preconditions, thread safety, and behavioral notes — nicely done.
One minor gap: consider documenting exception behavior (
@throwsor markingnoexcept). If the concrete implementations never throw during reset,noexcepton the virtual would enforce that contract across all backends.virtual void - reset() = 0; + reset() noexcept = 0;As per coding guidelines: "Document thread-safety guarantees and exception specifications where applicable."
include/boost/corosio/wolfssl_stream.hpp (1)
127-128: Consider adding a brief docstring forreset().The other overrides in this class follow a no-doc pattern (relying on the base class), but
reset()is a newly introduced operation. A one-line/// Reset the TLS session state for reuse.comment would help readers understand the semantics without navigating totls_stream.hpp. As per coding guidelines, docstrings are required for all functions in public headers in non-detail namespaces: "Docstrings should include a brief description of purpose, document all parameters, return values, and any preconditions, postconditions, or exceptions."include/boost/corosio/openssl_stream.hpp (1)
127-128: Missing documentation forreset().This is a new public API method on a public header. Per the coding guidelines, it needs at minimum a brief description,
@parnotes on preconditions (e.g., must not be called while an async operation is in flight), and what state the stream is left in after reset. Thetls_streambase class declaresvirtual void reset() = 0so ideally the base should carry the contract docs, but each override should at least have a/**@copydoctls_stream::reset */or its own brief.src/wolfssl/src/wolfssl_stream.cpp (1)
345-363: WolfSSLreset()frees the SSL object; OpenSSLreset()preserves it — asymmetric behavior worth noting.The WolfSSL
reset()callswolfSSL_free(ssl_)and nulls the pointer, while the OpenSSL variant callsSSL_clear()to preserve theSSL*/BIO pair. Both are valid approaches given the different initialization strategies (deferred vs. eager), but the behavioral asymmetry may confuse maintainers.Consider adding a brief comment in each
reset()explaining why the strategy differs (e.g., "WolfSSL defers SSL creation to handshake, so reset frees it entirely" vs. "OpenSSL eagerly creates SSL/BIO in make_impl, so reset preserves them").
| if (ec) | ||
| break; |
There was a problem hiding this comment.
Removing the n == 0 break guard may cause a busy loop on zero-byte reads.
The old condition was if (ec || n == 0). With only if (ec), a read_some that returns 0 bytes without an error will spin the loop: buf_.resize(0) → zero-length write → zero-length read_some → repeat. Even if the socket contract guarantees EOF via ec on peer shutdown, defensive coding against a zero-byte return (e.g., from a spurious wakeup or platform quirk) is cheap and prevents a potential tight loop.
Based on learnings: in boost::corosio, socket::shutdown(shutdown_send) has a contract that requires subsequent read operations to return an explicit EOF error code, not just zero bytes — but this contract may not cover all edge cases or platforms.
Proposed fix
- if (ec)
+ if (ec || n == 0)
break;🤖 Prompt for AI Agents
In `@example/echo-server/echo_server.cpp` around lines 56 - 57, The loop that
reads into buf_ must resume checking for zero-length reads to avoid a busy spin;
restore the original guard so that after calling read_some (the call that yields
ec and n) you break when either ec is set or n == 0. Update the conditional
around the read result in the function handling the socket read (the read_some
return handling that currently only tests ec) to include "|| n == 0" so
zero-byte reads cause the loop to exit and prevent repeated zero-length
write/read cycles.
| @@ -1,5 +1,5 @@ | |||
| // | |||
| // Copyright (c) 2026 Vinnie Falco (vinnie dot falco at gmail dot com) | |||
| // Copyright (c) 2026 Vinnie Falco (vinnie.falco@gmail.com) | |||
There was a problem hiding this comment.
Inconsistent copyright year: this file says 2026 while other files in this PR use 2025.
Other updated files (e.g., echo_server.cpp, timers.hpp, resolver.hpp) use Copyright (c) 2025. This file has Copyright (c) 2026. If this is intentional (e.g., file was created/modified in 2026), ignore; otherwise, align for consistency.
🤖 Prompt for AI Agents
In `@include/boost/corosio/endpoint.hpp` at line 2, The file header in
include/boost/corosio/endpoint.hpp has the copyright year 2026 which is
inconsistent with other files in this PR that use 2025; update the copyright
line in endpoint.hpp to use 2025 (or confirm and leave as 2026 if the change was
intentional) so it matches the rest of the project; locate the top-of-file
comment in endpoint.hpp (the file/identifier: endpoint.hpp, namespace corosio is
present) and change the year token from "2026" to "2025".
| template<capy::Stream S> | ||
| requires (!std::same_as<std::decay_t<S>, openssl_stream>) | ||
| openssl_stream(S stream, tls_context ctx) | ||
| : stream_(std::move(stream)) | ||
| , impl_(make_impl(stream_, ctx)) | ||
| { | ||
| } |
There was a problem hiding this comment.
make_impl can return nullptr, but the constructor doesn't check.
Looking at openssl_stream.cpp line 700–710, make_impl returns nullptr when init_ssl() fails. The constructor stores the result directly into impl_ without validation, so any subsequent member call (handshake, read, write, reset) will dereference a null pointer.
The wolfssl_stream constructor has the same pattern but its make_impl never returns nullptr (it defers SSL creation to handshake time), so it's safe there. The openssl variant is not.
Proposed fix: throw on init failure
template<capy::Stream S>
requires (!std::same_as<std::decay_t<S>, openssl_stream>)
openssl_stream(S stream, tls_context ctx)
: stream_(std::move(stream))
, impl_(make_impl(stream_, ctx))
{
+ if(!impl_)
+ throw std::runtime_error("OpenSSL initialization failed");
}Apply the same check to the pointer-based constructor.
🤖 Prompt for AI Agents
In `@include/boost/corosio/openssl_stream.hpp` around lines 86 - 92, The
openssl_stream constructor currently assigns impl_ = make_impl(...) without
checking for nullptr, which can happen when init_ssl() fails; update the
constructor (the template overload of openssl_stream taking S stream and
tls_context ctx) to validate the returned pointer from make_impl(stream_, ctx)
and throw a suitable exception (e.g., std::runtime_error or a domain-specific
error) if make_impl returns nullptr so subsequent methods (handshake, read,
write, reset) cannot dereference a null impl_; reference make_impl, impl_, and
init_ssl() when implementing the check.
| #include <boost/corosio/timer.hpp> | ||
|
|
||
| #include <boost/corosio/detail/except.hpp> | ||
|
|
There was a problem hiding this comment.
Add the required implementation overview comment after the includes.
This file contains non-trivial implementation logic but lacks the mandated /* */ high-level overview comment immediately after the includes. Please add a brief paragraph describing how the timer implementation works and any non-obvious details. As per coding guidelines “Files containing non-trivial implementation logic should include a /* */ block comment after the includes that provides a high-level overview of how the implementation works.”
🤖 Prompt for AI Agents
In `@src/corosio/src/timer.cpp` around lines 10 - 13, Add a high-level /* */
overview comment immediately after the includes in src/corosio/src/timer.cpp
that describes how the timer implementation works (e.g., interaction between
boost::corosio::timer, its scheduling/expiry mechanism, threading/strand
assumptions, how callbacks are enqueued/executed, and any subtle lifetime or
cancellation behavior). Place this paragraph right after the existing `#include`
lines and reference the primary symbols in the file (such as the timer
class/constructors, start/stop/expiry handlers, and any helper functions) so
future readers can quickly understand non-obvious implementation details.
| void | ||
| reset() | ||
| { | ||
| if(!ssl_) | ||
| return; | ||
|
|
||
| // Preserves SSL* and BIO pair, releases session state | ||
| SSL_clear(ssl_); | ||
|
|
||
| // Drain stale data from the external BIO | ||
| char drain[1024]; | ||
| while(BIO_ctrl_pending(ext_bio_) > 0) | ||
| BIO_read(ext_bio_, drain, sizeof(drain)); | ||
|
|
||
| // SSL_clear clears per-session settings; reapply hostname | ||
| auto& cd = detail::get_tls_context_data(ctx_); | ||
| if(!cd.hostname.empty()) | ||
| { | ||
| SSL_set_tlsext_host_name(ssl_, cd.hostname.c_str()); | ||
| SSL_set1_host(ssl_, cd.hostname.c_str()); | ||
| } | ||
|
|
||
| used_ = false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenSSL SSL_clear return value documentation
💡 Result:
SSL_clear(SSL *ssl) returns an int:
1: success — theSSLobject was reset and can be used for another connection. [1]0: failure — the reset could not be performed; check the OpenSSL error stack to determine why. [1]
Source: OpenSSL SSL_clear manual page (see RETURN VALUES). [1]
References
- [1] OpenSSL docs:
SSL_clear— RETURN VALUES (docs.openssl.org)
Citations:
🏁 Script executed:
# Search for all calls to reset() in the codebase to understand usage context
rg "reset\(" --type cpp --type hpp | head -20Repository: cppalliance/corosio
Length of output: 90
🏁 Script executed:
# Check the do_handshake function context where reset is called
sed -n '520,535p' src/openssl/src/openssl_stream.cppRepository: cppalliance/corosio
Length of output: 317
🏁 Script executed:
# Check the public reset() method mentioned in observations
sed -n '768,773p' src/openssl/src/openssl_stream.cppRepository: cppalliance/corosio
Length of output: 116
🏁 Script executed:
# Find all call sites of reset() in the codebase
rg "\.reset\(\)" --type-add 'cpp:*.{cpp,cc,cxx,h,hpp,hxx}' -A 2 -B 2Repository: cppalliance/corosio
Length of output: 19285
🏁 Script executed:
# Check the full do_handshake function to see error handling context
sed -n '526,600p' src/openssl/src/openssl_stream.cppRepository: cppalliance/corosio
Length of output: 1964
🏁 Script executed:
# Check if there's any error checking pattern for SSL operations in the file
rg "SSL_" src/openssl/src/openssl_stream.cpp | grep -E "(if|return|error)" | head -15Repository: cppalliance/corosio
Length of output: 874
Check SSL_clear() return value in reset().
SSL_clear() returns 0 on failure and 1 on success. Ignoring the return value means the stream silently proceeds with a potentially corrupted SSL session on the next handshake, leading to undefined behavior or security issues.
Change impl::reset() to check the return value and propagate the error. Since this is called from do_handshake(), consider returning std::error_code from reset() and handling the failure before proceeding with SSL operations.
Proposed approach
- void
- reset()
+ std::error_code
+ reset()
{
if(!ssl_)
- return;
+ return {};
- // Preserves SSL* and BIO pair, releases session state
- SSL_clear(ssl_);
+ if(!SSL_clear(ssl_))
+ {
+ unsigned long err = ERR_get_error();
+ return std::error_code(static_cast<int>(err), std::system_category());
+ }
// ... hostname reapplication ...
used_ = false;
+ return {};
}Update the public wrapper and do_handshake() to check the returned error code.
🤖 Prompt for AI Agents
In `@src/openssl/src/openssl_stream.cpp` around lines 305 - 328, The reset()
method currently ignores SSL_clear()'s return value; change impl::reset() to
return a std::error_code (or similar error indicator), call SSL_clear(ssl_) and
if it returns 0 convert the OpenSSL error (ERR_get_error /
ERR_reason_error_string) into a meaningful std::error_code and return it
immediately (before draining ext_bio_ or reapplying hostname), otherwise
continue and return success; then update callers (notably do_handshake() and the
public wrapper that invokes impl::reset()) to check the returned std::error_code
and abort/propagate the error instead of proceeding with SSL operations when
reset fails.
|
An automated preview of the documentation is available at https://108.corosio.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-02-09 00:08:13 UTC |
|
GCOVR code coverage report https://108.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-09 00:11:55 UTC |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores