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