Hi Ken
After some testing on our test server, we rolled out 2.3.10-rc2 onto one of
our production IMAP stores and almost immediately starting noticing
segfaults for some users.
Bron and I spent quite a bit of time this afternoon debugging, and we're
pretty sure we found both the causes.
1. In index_parse_sequence(), your realloc() doesn't multiply by
sizeof(struct seq_range), so if you have a sequence list with more than
about 8 items, it will start corrupting memory
2. The signed -> unsigned changes in the prot stream stuff weren't as
innocent as they looked and created a subtle and nasty bug. You might want
to audit the other code where you changed signed -> unsigned as well...
Here's an example of the problem if you use IDLE and close the connection on
it. In that case, what happens is:
In cmd_idle(), we wait for data from the client by calling:
c = getword(imapd_in, &arg);
getword() calls prot_getc(), which looks like this:
#define prot_getc(s) ((s)->cnt-- > 0 ? (int)*(s)->ptr++ : prot_fill(s))
Say there's no data in the prot buffer, then cnt == 0. However when we call
the above macro, it does cnt--, which makes cnt wrap to 4294967295, and then
calls prot_fill. prot_fill() calls read(), and if read() returns <= 0 (eg
other end closed the connection) it does.
if (n <= 0) {
if (n) s->error = xstrdup(strerror(errno));
else s->eof = 1;
return EOF;
}
So it sets the eof flag and returns EOF.
Back in cmd_idle, we get the EOF, and exit out of cmd_idle and back into the
main loop, which does.
/* Parse tag */
c = getword(imapd_in, &tag);
Now getword calls the prot_getc() macro again... but uh, oh. cnt is
definitely > 0 now (it's 4294967295) and so we reading through unitialised
memory trying to interpret it as IMAP commands until we hit a segfault
somewhere.
Our fix is to change prot_getc() to only decrement cnt if it's non-zero.
Because it's an expression macro, it uses the C comma operator which I've
found often even experienced C people don't know about
(http://www.eskimo.com/~scs/cclass/int/sx4db.html).
#define prot_getc(s) ((s)->cnt > 0 ? (--(s)->cnt, (int)*(s)->ptr++) :
prot_fill(s))
Both patches are here:
http://cyrus.brong.fastmail.fm/patches/cyrus-fix-segfaults-2.3.10.diff
Rob
