Re: [dev] [sic]: patches for string handling

2009/9/22 Mark Edgar <medgar123_AT_gmail.com>:
> Here's a summary of the changes in this patch:
>
> * Fix buffer overrun when using strncpy()

Applied.

> * Use startswith() macro instead strncmp()

Not applied.

> * Use strcmp() instead of strncmp() where appropriate

Applied.

> * Use const char * where appropriate

Not applied.

> * Use size_t instead of unsigned int in readl().

Not applied.

> 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.  ;)

Well, I prefer strncmp since that doesn't hides what startswith does hide.

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

That's only true for PING/PONG assumed the server doesn't sends any
crap after these command.
For PRIVMSG cmd is not NUL-terminated after PRIVMSG and contains
arguments, so the strncmp is on purpose in all cases of that function.

> 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.

Yes I prefer your assumption and solution.

> 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.

Yes looking forward to it. sic needs some love after all ;)

Thanks so far.
Anselm
Received on Wed Sep 23 2009 - 13:27:47 UTC