[Freeciv-Dev] networking bugs in the code (PR#280)

[hayden@xxxxxxxxxxxxxx]

Full_Name: Mark G Hayden
Version: 1.10
Distribution: Built from source
Client: Both (or N/A)
OS: RedHat Linux
Submission from: (NULL) (24.142.58.200)


I experienced problems playing free civ this weekend.  In between
games, I took a look at the TCP socket code to see if I could
spot any problems.  These are my comments.  I've not contributed
to the freeciv development effort before and am not on freeciv-dev
mailing list.  Any replies should be sent directly to me.

The problems in the 1.10 civserver this weekend might be explained
by the networking code.  The client TCP sockets should be placed
in non-blocking mode by doing something like:

  int flag ;
  flag = O_NONBLOCK ;
  ret = fcntl(sock, F_SETFL, flag) ;

I don't believe that is happening anywhere in the code.  Also,
the code in flush_connection_send_buffer assumes that a write()
to the socket will write all the data requested:

/********************************************************************
  flush'em
********************************************************************/
void flush_connection_send_buffer(struct connection *pc)
{
  if(pc) {
    if(pc->send_buffer.ndata) {
      if(write(pc->sock, (const char *)pc->send_buffer.data,
pc->send_buffer.ndata)!=
         pc->send_buffer.ndata) {
        freelog(LOG_NORMAL, "failed writing data to socket");
      }
      pc->send_buffer.ndata=0;
    }
  }
}

Potentially the write() call could block since
it is not in non-blocking mode.  If the call blocks, which could happen
when, for instance, the connection to a client is really slow or becomes
unreachable, then the server would hang there without being able to
handle
other client requests.  This could be one reason for the server hanging.

It is in spec for the write() call to not write out all the
data in the buffer, so writing a log message and dropping the unsent
data is definitely a *BAD* thing to do.  This allows arbitrary data to
be lost
in the client-server communication stream.  Either the connection should
be shutdown or more attempts should be made to send the remaining data.

The right way to do this code is to select() for both reading and
writing
on the socket (currently, the code only select()'s for reading).   Also,
the socket should be put in non-blocking mode to make sure that the
server
never blocks on any of the socket system calls.