Patches not taken up by upstream

Bron Gondwana brong at fastmail.fm
Wed Jun 16 23:09:39 EDT 2010


On Wed, Jun 16, 2010 at 11:49:22AM -0500, Patrick Goetz wrote:
> Wouldn't the len*10 here:
>        len = len*10 + c - '0';
>             if (len > MAXLITERAL || len < 0) {
> 
> imply that it's actually changing a 10.7MB max size to 21.4MB?

Talk about taking a break from working on Cyrus to bloody well
work on Cyrus :(

Here's the interesting bit as it is now:

+    while ((c = prot_getc(pin)) != EOF && cyrus_isdigit(c)) {
+       if (n > (INT_MAX/10 - 10))
+           fatal("num too big", EC_IOERR);
+       n = n * 10 + c - '0';
+       gotchar = 1;
+    }

This is in imapparse.c - it's called "getnum".  I'm replacing
all the spots where we pull a number off the wire character by
character with a call to getnum.

I'm also replacing everywhere that we advance a character
pointer through a string and do the same algorithm with
parsenum in lib/util.c.  It will have the same logic.  No
more MAXLITERAL.

There might be cause for adding a configurable maximum size
in the specific spot where we're parsing a literal off the
wire so we can't be forced to malloc an arbitrary amount of
memory, but that's a separate issue from reading the value,
and should be handled separately.  I might add one of those
too if I'm in a good mood after tracking down all the
copypasta instances of this dodgy little algorithm.
(half of them with /* xxx - overflow */ comments, mind you)

Bron.


More information about the Cyrus-devel mailing list