Fix use-after-free in IOCP socket service shutdown#131
Fix use-after-free in IOCP socket service shutdown#131vinniefalco wants to merge 6 commits intocppalliance:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates Windows IOCP socket wrapper lifecycle management, completion handling patterns, and TLS stream lock acquisition. Changes defer wrapper cleanup to destructor, isolate operation queue processing, add explicit null checks, and implement error handling for lock failures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #131 +/- ##
===========================================
- Coverage 80.98% 80.97% -0.01%
===========================================
Files 64 64
Lines 5721 5724 +3
===========================================
+ Hits 4633 4635 +2
- Misses 1088 1089 +1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
An automated preview of the documentation is available at https://131.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-14 03:00:40 UTC |
|
GCOVR code coverage report https://131.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-14 03:09:13 UTC |
vinniefalco
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/iocp/sockets.cpp`:
- Around line 620-633: The destructor currently deletes nodes returned by
socket_wrapper_list_.pop_front() and acceptor_wrapper_list_.pop_front(), but
those popped nodes keep stale next_/prev_ pointers that later cause list
corruption when their destructor calls unregister_impl() -> remove(); fix by
ensuring pop_front() clears the popped node's linkage (set node->next_ =
node->prev_ = nullptr) so deleted nodes are clearly unlinked, or alternatively
harden remove() in intrusive.hpp to detect and ignore already-unlinked nodes
(e.g., verify a node is linked to this list before dereferencing prev_/next_);
target the pop_front implementation and/or remove/unregister_impl functions to
implement this defensive reset/check.
16f8d83 to
fcf1067
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/wolfssl/src/wolfssl_stream.cpp`:
- Around line 874-876: In do_shutdown, the SHUTDOWN_NOT_DONE paths that call
auto [lec, guard] = co_await io_cm_.scoped_lock() currently break without
propagating the lock error (lec) into ec, so callers can observe success despite
a lock failure; update both occurrences (the scoped_lock() branches around the
SHUTDOWN_NOT_DONE logic) to assign ec = lec before the break to match the
WOLFSSL_SUCCESS path, or if the discard is intentional, add a clear comment in
do_shutdown explaining why lock errors are suppressed there; reference the
scoped_lock call, the lec variable, and the ec return code when making the
change.
During execution_context destruction, win_sockets::shutdown() was eagerly deleting socket wrapper objects while the scheduler still had pending IOCP operations referencing them. When win_scheduler:: shutdown() later drained those operations and destroyed coroutine frames, the tcp_socket destructors inside those frames would call release() on already-freed wrappers, corrupting the heap. The fix follows the same pattern Asio uses in win_iocp_socket_service_base::base_shutdown(): shutdown() now only closes socket handles to force pending I/O to complete; wrapper deletion is deferred to ~win_sockets(), which runs after the scheduler has finished draining all outstanding operations. Also fixes accept_op's destroy path to properly release its pre-allocated accepted_socket and peer_wrapper before destroying the coroutine frame.
post_deferred_completions() was called with completed_ops_ by reference. When PostQueuedCompletionStatus failed, the recovery path did completed_ops_.splice(ops) where ops aliased completed_ops_, corrupting the intrusive queue and permanently losing operations. Leaked ops never decrement outstanding_work_ so run() hangs. Drain completed_ops_ into a local queue before calling post_deferred_completions, matching the epoll/kqueue backends.
Reset stop_event_posted_ flag in restart() so a stopped IOCP scheduler can run again. Set linger(true, 0) on server sockets in accept churn benchmarks to avoid TIME_WAIT accumulation.
…tch_coro Remove the duplicate outstanding_work_ zero-check from do_one() which could race with run()'s own shutdown logic and cause premature exits. Add explicit nullptr comparison in dispatch_coro to silence compiler warnings. Improve GQCS timeout comment.
Adapt openssl_stream and wolfssl_stream to the new scoped_lock() signature that returns (error_code, guard) via structured bindings, propagating lock-acquisition errors instead of ignoring them. Update tcp_server to use the renamed get_current_frame_allocator().
2 participants
Summary by CodeRabbit
Bug Fixes
Performance