Skip to content

bugfix(network): Close sockets that fail to bind - #3488

Merged
xezon merged 1 commit into
TheSuperHackers:mainfrom
tintinhamans:arctic/fix/udp-bind-socket-leak
Oct 12, 2026
Merged

xezon merged 1 commit into
TheSuperHackers:mainfrom
tintinhamans:arctic/fix/udp-bind-socket-leak

Conversation

@tintinhamans

Copy link
Copy Markdown

UDP::Bind opens a new socket on every call and never closed it when bind failed. Transport::init retries Bind for a full second, so every attempt on a busy port leaks a socket. With the LAN lobby port 8086 already taken, for example by a second game on the same PC, I measured about 100k sockets and 70 MB of kernel pool per second. Entering the lobby runs that loop twice.

FirewallHelperClass::openSpareSocket left a spare socket that failed to bind in its slot, and the next call overwrote it.

Both now close the socket on failure.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3f5f9745-7341-42ba-b7d5-4abe9245d9fe

📥 Commits

Reviewing files that changed from the base of the PR and between 21f6de1 and 95cbed3.


📒 Files selected for processing (2)
  • Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp
  • Core/GameEngine/Source/GameNetwork/udp.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.



Walkthrough

When UDP::Bind fails, it now closes the socket and resets fd to 0. The spare-socket creation path deletes the failed UDP object and sets its pointer to nullptr before returning failure.

Changes

UDP bind failure cleanup

Layer / File(s) Summary
Clean up failed UDP binds
Core/GameEngine/Source/GameNetwork/udp.cpp, Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp
On bind failure, UDP::Bind closes the socket and resets fd to 0. openSpareSocket deletes the UDP object and sets its pointer to nullptr before returning FALSE.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Suggested reviewers: xezon

Merge Risk: 🟡 Moderate · up to 95cbe

A POSIX bind failure may stop retries while leaving transport or spare-socket setup with a closed socket, disrupting UDP networking. Correct the bind failure status before merging.

Pre-merge checks · Passed 4
Passed Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: closing sockets when binding fails.
Description check Passed The description directly explains the socket leak, the affected retry path, and the two fixes in the changeset.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; failed sockets are closed without breaking retries or later cleanup.

Summary

Closes sockets when UDP::Bind fails and deletes failed spare-socket objects.

  • Failed UDP binds now close sockets and clear spare objects.

Reviews (2) · Last reviewed commit: "bugfix(network): Close sockets that fail..." · Reviewed by Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a6e9ab6b-2ae5-4874-b075-3ea8913b2f42
📥 Commits

Reviewing files that changed from the base of the PR and between 0e6a06c and 21f6de1.

📒 Files selected for processing (2)
  • Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp
  • Core/GameEngine/Source/GameNetwork/udp.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

Comment thread Core/GameEngine/Source/GameNetwork/udp.cpp

@Mauller Mauller left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

looks good

Comment thread Core/GameEngine/Source/GameNetwork/udp.cpp Outdated
Comment thread Core/GameEngine/Source/GameNetwork/FirewallHelper.cpp Outdated
@xezon xezon added Minor Severity: Minor < Major < Critical < Blocker Network Anything related to network, servers Memory Is memory related Fix Is fixing something, but is not user facing Gen Relates to Generals ZH Relates to Zero Hour labels Oct 12, 2026
@tintinhamans
tintinhamans force-pushed the arctic/fix/udp-bind-socket-leak branch from 21f6de1 to 95cbed3 Compare October 12, 2026 20:03

@xezon xezon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Very good

@xezon
xezon merged commit 2d6761c into TheSuperHackers:main Oct 12, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Fix Is fixing something, but is not user facing Gen Relates to Generals Memory Is memory related Minor Severity: Minor < Major < Critical < Blocker Network Anything related to network, servers ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants