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')
  • newgrf_debug_data.h may need updating.
  • PR must be added to API tracker