Refactor localhost.cpp for socket relay management - #41520
Refactor localhost.cpp for socket relay management#41520Eamon (Eamon2009) wants to merge 2 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The updated poll loop and ioctl string copy still leave edge cases (POLLNVAL busy-loop risk, and copying padding bytes past NUL from traced process memory) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the Linux-side localhost relay and port tracker interception logic in src/linux/init/localhost.cpp to avoid relay worker hangs on disconnect, preserve buffered relay data by waiting for both directions to close, and make interface-name handling in the ioctl path more robust.
Changes:
- Adjust relay worker termination logic to wait for both relay directions to close.
- Handle additional
poll()result events so disconnect conditions are detected instead of spinning. - Make interface name copying explicitly NUL-terminated for the ioctl-based interface-state message.
File summaries
| File | Description |
|---|---|
| src/linux/init/localhost.cpp | Fixes relay worker disconnect handling/exit conditions and tightens ioctl interface-name handling for the GNS tunnel bridge request. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are small, localized, and align with established relay patterns in the codebase to prevent disconnect-related spins and improve shutdown behavior.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary of the Pull Request
Fixed the relay worker thread hanging or spinning when a peer disconnects or a descriptor becomes invalid. The loop only checked for
POLLIN, soPOLLHUPandPOLLERRfrom a closed connection were ignored, causing an infinite spin at 100% CPU. Added explicit handling forPOLLHUPandPOLLERRso the thread detects disconnects and shuts down cleanly. Also addedPOLLNVALhandling so the worker exits instead of busy-looping when a descriptor becomes invalid unexpectedly.Changed the relay exit condition from
||to&&so the thread waits for both directions to close before exiting. This prevents dropping buffered data when one side shuts down early.Fixed the ioctl interface name copy to avoid leaking uninitialized bytes from traced process memory. Instead of copying a fixed
sizeof-1bytes, the code now usesstrnlento copy only up to the first NUL (bounded) and explicitly null-terminates, ensuring the remainder stays zeroed.PR Checklist
Detailed Description of the Pull Request / Additional comments
Relay worker thread hang and spin
The relay loop was polling only for
POLLIN. When a peer disconnected, the socket returnedPOLLHUP/POLLERR, but since those were not handled, the thread spun indefinitely at 100% CPU instead of exiting. Added explicit handling forPOLLHUPandPOLLERRso the thread detects disconnects and shuts down cleanly.Additionally,
POLLNVALwas missing from the handled event mask. If a descriptor ever became invalid (e.g., closed unexpectedly),poll()could continuously reportPOLLNVALand the loop would spin again without making progress. AddedPOLLNVALto the mask so the worker exits immediately instead of busy-looping.Relay exit condition
Previously the relay thread exited when either direction closed (
||). This caused buffered data in the still-open direction to be dropped. The condition is now&&so the thread stays alive until both directions have closed, ensuring all buffered data is flushed.Interface name null-termination and bounded copy
The ioctl handler copied the interface name assuming the destination struct was zero-initialized. This is fragile and could leak stack data or produce an unterminated string if initialization assumptions change. The fix now uses
strnlento determine the actual string length (bounded bysizeof(request.InterfaceName) - 1), copies only that many bytes, and sets the final byte to'\0'directly. This prevents transmitting uninitialized/non-NUL padding bytes from the traced process memory across the channel.