2009/9/22 Mark Edgar <medgar...@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
