[Freeciv-Dev] two obscure get_packet_from_connection bugs

[Jeff Mallatt <jjm@xxxxxxxxxxxx>]

While perusing get_packet_from_connection(), I noticed what I believe to be
two small bugs near the top of the routine.  Here's the code:

void *get_packet_from_connection(struct connection *pc, int *ptype)
{
  int len, type;

  if(pc->buffer.ndata<4)
    return NULL;           /* length and type not read */

  get_uint16(pc->buffer.data, &len);
  get_uint8(pc->buffer.data+2, &type);

  if(pc->first_packet) {
*** first packet stuff removed ***
  }

  if(pc->byte_swap) {
    len = swab_uint16(len);
    /* so the packet gets processed (removed etc) properly: */
    put_uint16(pc->buffer.data, len);
  }

  if(len > pc->buffer.ndata)
    return NULL;           /* not all data has been read */

*** rest of routine skipped ***

First, the test for whether the length and type fields have been received is:
  if(pc->buffer.ndata<4)
but, the length and type fields only occupy 3 bytes.  So, if we have a
packet that is only a signal and contains no payload (we do:
PACKET_BEFORE_NEW_YEAR), then the processing of that packet will be held
off until at least one byte of the following packet is received.  The
attached patch changes the test to:
  if(pc->buffer.ndata<3)

Second, if we happen to receive a partial packet in the old byte order
(where pc->byte_swap is true), the code puts the swapped length back into
the packet buffer before returning to fetch the rest of the packet.  The
next time through, the length will be swapped a second time and put back
into the packet, leading to potential problems.  The attached patch does
not put the correct length into the packet buffer until after it determines
that the entire packet is present for processing.

get_packet_two_fixes-0.diff
Description: Text document

jjm