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

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

>>  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...

> 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.