fix(udp): RAII thread ownership, FD_SETSIZE guards, and safe stop()#1213
Open
ruck314 wants to merge 6 commits into
Open
fix(udp): RAII thread ownership, FD_SETSIZE guards, and safe stop()#1213ruck314 wants to merge 6 commits into
ruck314 wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the UDP transport layer by adding descriptor-range checks, moving worker ownership to RAII, and adding regression tests around those fixes. This fits the networking/protocols area by trying to make UDP client/server startup and shutdown safer under resource pressure.
Changes:
- Added
FD_SETSIZEvalidation in UDP client/server constructors and aroundFD_SET(...)usage. - Replaced raw worker-thread ownership with
std::unique_ptr<std::thread>in UDP core/client/server. - Added doctest-based UDP regression tests and wired them into the CMake test tree.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/cpp/protocols/udp/test_udp_raw_thread_audit_repro.cpp |
Adds source-audit regression tests for thread ownership changes. |
tests/cpp/protocols/udp/test_udp_fd_setsize_audit_repro.cpp |
Adds source-audit regression tests for FD_SETSIZE guards. |
tests/cpp/protocols/udp/CMakeLists.txt |
Registers the new UDP C++ tests. |
tests/cpp/protocols/CMakeLists.txt |
Includes the new UDP test subdirectory. |
src/rogue/protocols/udp/Server.cpp |
Adds fd validation, RAII thread creation, and bad-fd worker handling for UDP server. |
src/rogue/protocols/udp/Client.cpp |
Adds fd validation, RAII thread creation, and bad-fd worker handling for UDP client. |
include/rogue/protocols/udp/Core.h |
Changes shared UDP thread ownership to std::unique_ptr<std::thread>. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ruck314
changed the title
- Replace raw new/delete std::thread with std::unique_ptr<std::thread> in udp::Client and udp::Server - Add FD_SETSIZE bounds check in constructors to reject fd >= FD_SETSIZE at construction instead of crashing asynchronously in the worker thread - Add defensive FD_SETSIZE guards before every FD_SET(fd_, ...) call in acceptFrame() and runThread(); runThread() logs and exits cleanly instead of calling std::terminate - Guard stop() on thread_->joinable() to prevent std::terminate on double-stop or stop-before-start - Add doctest regression tests for RAII thread and FD_SETSIZE invariants
Condense multi-line explanatory comments in Client.cpp and Server.cpp to single-line summaries. Remove unnecessary doxygen comments on self-documenting test helper functions.
Resolve CMakeLists.txt conflict: keep both hardware and udp subdirs.
Per review feedback, source-text audit-style C++ tests are too brittle to be useful as CI tests. The two doctest files (test_udp_fd_setsize_audit_repro.cpp, test_udp_raw_thread_audit_repro.cpp) were grep checks against Client.cpp/Server.cpp, not functional behaviour. Drop them and the udp/ test subdirectory; the production hardening (FD_SETSIZE guards, RAII thread ownership, safe stop) is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
new/delete std::threadwithstd::unique_ptr<std::thread>inudp::Clientandudp::Server(RAII ownership eliminates leak risk on future throw paths)FD_SETSIZEbounds check in constructors — reject sockets withfd >= FD_SETSIZEat construction rather than crashing asynchronously in the worker threadFD_SETSIZEguards before everyFD_SET(fd_, ...)call inacceptFrame()andrunThread()—runThread()now logs and exits cleanly instead of callingstd::terminatestop()onthread_->joinable()to preventstd::terminateon double-stop or stop-before-startFiles changed
include/rogue/protocols/udp/Core.h—thread_type: raw ptr →unique_ptrsrc/rogue/protocols/udp/Client.cpp— ctor guard, RAII thread, safe stop, runThread exitsrc/rogue/protocols/udp/Server.cpp— ctor guard, RAII thread, safe stop, runThread exitTests
No new tests in this PR. The original C++ audit-style tests
(
test_udp_fd_setsize_audit_repro.cpp,test_udp_raw_thread_audit_repro.cpp) were source-text greps againstClient.cpp/Server.cppand have been removed per review feedback —they are too brittle to be useful as CI checks. Functional UDP
coverage at this level (oversized fd injection, double-stop)
requires kernel-level scaffolding that is out of scope here; the
audit-style intent should be formalised as an AI agent directive at
PR-review time instead.
Verification
./scripts/run_linters.shcmake -DROGUE_BUILD_TESTS=ON -B build && cmake --build build && ctest --test-dir buildpytest tests/