OpenTTD icon indicating copy to clipboard operation
OpenTTD copied to clipboard

Codechange: also store the sending socket in Packets

Open rubidium42 opened this issue 2 years ago • 0 comments

Motivation / Problem

When adding encryption to our network packets, we need to add a "message authentication code" (MAC) data to the packet. This will take up some space if/when the packet gets encrypted. Since some packets are filled up to the maximum size, before continuing to the next packet, the allocation for the MAC needs to be made when the packet is created. For this it needs to know whether the encryption is enabled on the socket.

There are multiple ways of achieving this:

  • adding a boolean to the constructor, and then add checks to the socket handler at each of the call sites.
  • adding the socket handler to the constructor.

When adding the boolean to the constructor you still need to pass the socket handler in PrepareToSend and you need to add some state whether the encryption has been added to the packet as well. When passing the socket handler, it can be put in the already existing field for the socket handler and it can be set to nullptr in case encryption is not needed any more, saving a bit of space.

Note that adding encryption will be a post-14-branch endeavour. However, having this infrastructure available in the 14 branch makes it easier to backport any network changes that are not related with the encryption/key exchange.

Description

TransferToNetworkQueue has a socket as parameter, but that is always equals to this->cs except when the server already closed the connection. Though then you would not be copying packets to the network queue.

The PacketWriter, that writes the network "map" packets during saving the game, reads and writes some instance variables that are accessed in other places within a lock. Either it is for allocating memory based on the variable being nullptr, or it is for checking the connection got closed. In case of the former that could technically be a problem when doing multiple simultaneous calls. In the latter case you could check for nullptr, then wait on the lock that Destroy has done and still do the expensive work. Moving it here just avoids that problem, for a tiny penalty when the socket got closed during download, though arguably that should be happening that often.

The last commit finally adds the socket to the packet's constructor. Except for some error messages that are sent before we even create the socket handler.

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 touches english.txt or translations? Check the guidelines
  • 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, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

rubidium42 avatar Feb 06 '24 18:02 rubidium42