Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: crash when joining a server again after a TCP disconnect #9453

Merged
merged 1 commit into from Jul 21, 2021

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jul 21, 2021

Motivation / Problem

  • Start a server
  • Join the server with another client
  • Kill the server
  • Restart the server
  • Try joining the server again, without closing the client
  • BOOM

This PR tries to fix this. But it isn't simple :P

Description

"my_client" wasn't always free'd when a game ended. "my_client"
keeps a reference inside the PT_NCLIENT pool. The rest of the
code assumes that when you are not in a game, it can freely
reset this pool.
In result: several ways to trigger a use-after-free.

A while back we untangled CloseConnection and CloseSocket in the NetworkTCPSocketHandler. Sadly, it seems that we didn't look close enough to how clients handle disconnects. But honestly, I suspect this has been broken for many years already; it was just hiding, as it "mostly" will be fine for release builds (lack of asserts etc).

The problem really is: when the TCP connection is terminated, SendPackets notices this, calls the CloseConnection(bool), which no longer called the CloseConnection(status). So ClientNetworkGameSocketHandler::CloseConnection was never called anymore, which is the only place that can free my_client.

The comment in ClientNetworkGameSocketHandler::CloseConnection is outdated: sock can never become INVALID_SOCKET anymore, unless the object is destroyed. Instead, we have a flag to indicate the socket is closed. But either way, when called, it should destroy my_client.

This creates another use-after-free, as ClientNetworkGameSocketHandler::Send calls SendPackets, which can cause the free to happen, but uses my_client after that again. So this is now guarded too.

Still with me? Yeah, it took me a while too to untangle this mess. I hope this explanation helped you a bit :)

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
"my_client" wasn't always free'd when a game ended. "my_client"
keeps a reference inside the PT_NCLIENT pool. The rest of the
code assumes that when you are not in a game, it can freely
reset this pool.
In result: several ways to trigger a use-after-free.
@TrueBrain TrueBrain merged commit 9cc7068 into OpenTTD:master Jul 21, 2021
14 checks passed
Loading
@TrueBrain TrueBrain deleted the fix-client-disconnect branch Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants