[dev] [sic]: patches for string handling

From: Mark Edgar <medgar123_AT_gmail.com>
Date: Wed, 23 Sep 2009 00:53:35 +0200

Here's a summary of the changes in this patch:

* Fix buffer overrun when using strncpy()
* Use startswith() macro instead strncmp()
* Use strcmp() instead of strncmp() where appropriate
* Use const char * where appropriate
* Use size_t instead of unsigned int in readl().

The startswith() macro is debatable -- it won't hurt my feelings if no
one likes it. It also adds 2 lines to sic.c, bringing the total to
more than 250. ;)

Also, parsesrv() should be using strcmp() on cmd, not strncmp(), but I
decided not to try to fix that in this patch.

The only real problem is the buffer overrun after using strncpy().
Since it happens only when reading 'nick' or 'channel', I could have
fixed it in another way, by continuing to treat the contents of these
buffers as non-null-terminated char arrays instead of strings, so for
example, this call:

snprintf(bufout, sizeof bufout,
         "PASS %s\r\nNICK %s\r\nUSER %s localhost %s :%s\r\n",
          password, nick, nick, host, nick);

would be rewritten as:

snprintf(bufout, sizeof bufout,
         "PASS %s\r\nNICK %.*s\r\nUSER %.*s localhost %s :%.*s\r\n",
         password, sizeof nick, nick, sizeof nick, nick, host, sizeof
nick, nick);

or maybe just this:

snprintf(bufout, sizeof bufout,
         "PASS %s\r\nNICK %.32s\r\nUSER %.32s localhost %s :%.32s\r\n",
         password, nick, nick, host, nick);

and similarly for other places where 'nick' and 'channel' are read. I
decided against this. The way it is fixed in this patch relies on the
fact that these arrays are default-initialized to all '\0' bytes and
also that the last byte in each (nick[31] and channel[255]) is never
written to, thus the contents are always null-terminated. There's
also the data truncation issue (what if the server actually issues a
NICK change longer than 31 characters to you?) which is probably OK to
continue to ignore in this case.

I have more changes, including getting rid of calls to strlen(),
fixing the blocking I/O issues, and fixing the IRC protocol parsing in
parsesrv(). The cumulative line count delta for these changes is
actually negative (down to 245 lines). I should probably just attach
these now too, but I want to start with this one.


Received on Tue Sep 22 2009 - 22:53:35 UTC

This archive was generated by hypermail 2.2.0 : Tue Sep 22 2009 - 23:00:04 UTC