"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
Re: gotwebd: memleak in (and small refactoring of) fcgi_parse_record
To:
Omar Polo <op@omarpolo.com>, gameoftrees@openbsd.org, Tracey Emery <tracey@traceyemery.net>
Date:
Thu, 1 Sep 2022 11:36:47 +0200

Download raw body.

Thread
On Thu, Sep 01, 2022 at 11:09:17AM +0200, Stefan Sperling wrote:
> On Thu, Sep 01, 2022 at 10:52:01AM +0200, Omar Polo wrote:
> > Using a memcpy + explicit NUL terminator (as the original code was
> > doing) is probably for the better.  (but change bcopy for memcpy.)
> 
> Fine, ok.
> 
> Unrelated to your diff, but I'm wondering why 'n' is a uint16_t
> instead of uint32_t.
> 
> The name_len and val_len values are uint32_t and their combined
> max value parsed from client-provided data is 0x7fffffff +
> 0x7fffffff == 4294967294 which is just below UINT_MAX.
> Which could result in 16-bit 'n' wrapping when we subtract at
> the end of the loop?
> 
> 		buf += name_len + val_len;
> 		n -= name_len - val_len;
  
I missed that we only reach this code if n >= name_len + val_len,
as Omar pointed out on IRC. So this should be fine as it is.