Two bugs in 2.3.10-rc2 that cause segfaults

[Ken Murchison]

Applied to CVS.  Thanks for hunting these down.


Rob Mueller wrote:
> 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
> 
> 


-- 
Kenneth Murchison
Systems Programmer
Project Cyrus Developer/Maintainer
Carnegie Mellon University