Two bugs in 2.3.10-rc2 that cause segfaults

[Rob Mueller]

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