Skip to content

Refactor localhost.cpp for socket relay management - #41520

Open
Eamon (Eamon2009) wants to merge 2 commits into
microsoft:masterfrom
Eamon2009:patch-9
Open

Refactor localhost.cpp for socket relay management#41520
Eamon (Eamon2009) wants to merge 2 commits into
microsoft:masterfrom
Eamon2009:patch-9

Conversation

@Eamon2009

@Eamon2009 Eamon (Eamon2009) commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

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, so POLLHUP and POLLERR from a closed connection were ignored, causing an infinite spin at 100% CPU. Added explicit handling for POLLHUP and POLLERR so the thread detects disconnects and shuts down cleanly. Also added POLLNVAL handling 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-1 bytes, the code now uses strnlen to copy only up to the first NUL (bounded) and explicitly null-terminates, ensuring the remainder stays zeroed.

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

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 returned POLLHUP/POLLERR, but since those were not handled, the thread spun indefinitely at 100% CPU instead of exiting. Added explicit handling for POLLHUP and POLLERR so the thread detects disconnects and shuts down cleanly.

Additionally, POLLNVAL was missing from the handled event mask. If a descriptor ever became invalid (e.g., closed unexpectedly), poll() could continuously report POLLNVAL and the loop would spin again without making progress. Added POLLNVAL to 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 strnlen to determine the actual string length (bounded by sizeof(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.

@Eamon2009
Eamon (Eamon2009) requested a review from a team as a code owner September 4, 2026 04:14
Copilot AI lite review requested due to automatic review settings September 4, 2026 04:14
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread src/linux/init/localhost.cpp
Comment thread src/linux/init/localhost.cpp Outdated
Copilot AI review requested due to automatic review settings September 4, 2026 05:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants