Re: [PATCH] mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair()

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 13, 2023, 14:54 UTC
Message-ID: <xmqqy1dynofo.fsf@gitster.g>
In-Reply-To: <ZXlYIZ0Hb1kN84NU@tanuki>

Patrick Steinhardt <ps@pks.im> writes:

Show 17 quoted lines

>>  static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in)
>>  {
>> -	int c;
>>  	int take_next_literally = 0;
>>  
>> -	while ((c = *in++) != 0) {
>> +	while (*in) {
>> +		int c = *in++;
>>  		if (take_next_literally == 1) {
>>  			take_next_literally = 0;
>>  		} else {
>
> I was wondering whether we want to convert `unquote_quoted_pair()` in
> the same way. It's not subject to the same issue because it doesn't
> return `in` to its callers. But it would lower the cognitive overhead a
> bit by keeping the coding style consistent. But please feel free to
> ignore this remark.

Yeah, I was wondering about the value of establishing a pattern that can be followed safely and with clarity, too. I also briefly wondered how bad if we picked the "compensate for the increment done by the last iteration before leaving the loop by returning (in-1)" idiom (which Peff called "a hacky way") to be that universal pattern, but this bug was a clear enough evidence that it does not work very well in developers' minds.

I actually had trouble with the proposed update, and wondered if

- while ((c = *in++) != 0) { + while ((c = *in)) { + in++;

is easier to follow, but then was hit by the possibility that the same "we have incremented 'in' a bit too early" may exist if such a loop wants to use 'in' in its body. Wouldn't it mean that

- while ((c = *in++) != 0) { + for (; c = *in; in++) {

would be even a better rewrite?

Enough bikeshedding...

Show 8 quoted lines

> Another thing I was wondering about is the recursive nature of these
> functions, and whether it can lead to similar issues like we recently
> had when parsing revisions with stack exhaustion. I _think_ this should
> not be much of a problem in this case though, as we're talking about
> mail headers here. While the length of header values isn't restricted
> per se (the line length is restricted to 1000, but with Comment Folding
> Whitespace values can span multiple lines), but mail provdiers will sure
> clamp down on mails with a "From:" header that is megabytes in size.

Good thinking, and I think Peff's iterative rewrite would be a good way to deal with it if it ever becomes an issue.

> So taken together, this looks good to me.

Thanks, both, for writing and reviewing.