public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Patrick Steinhardt <ps@pks•im>
Cc: "Jeff King" <peff@peff•net>,
	git@vger•kernel.org, "Taylor Blau" <me@ttaylorr•com>,
	"Carlos Andrés Ramírez Cataño" <antaigroupltda@gmail•com>
Subject: Re: [PATCH] mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair()
Date: Wed, 13 Dec 2023 06:54:03 -0800	[thread overview]
Message-ID: <xmqqy1dynofo.fsf@gitster.g> (raw)
In-Reply-To: <ZXlYIZ0Hb1kN84NU@tanuki> (Patrick Steinhardt's message of "Wed, 13 Dec 2023 08:07:21 +0100")

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.

next prev parent reply	other threads:[~2023-12-13 14:54 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-12 22:12 [PATCH] mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair() Jeff King
2023-12-13  7:07 ` Patrick Steinhardt
2023-12-13  8:20   ` Jeff King
2023-12-14  7:41     ` Patrick Steinhardt
2023-12-14 21:44       ` [PATCH 0/2] avoiding recursion in mailinfo Jeff King
2023-12-14 21:47         ` [PATCH 1/2] t5100: make rfc822 comment test more careful Jeff King
2023-12-14 21:48         ` [PATCH 2/2] mailinfo: avoid recursion when unquoting From headers Jeff King
2023-12-15  7:58         ` [PATCH 0/2] avoiding recursion in mailinfo Patrick Steinhardt
2023-12-13 14:54   ` Junio C Hamano [this message]
2023-12-14 21:40     ` [PATCH] mailinfo: fix out-of-bounds memory reads in unquote_quoted_pair() Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqy1dynofo.fsf@gitster.g \
    --to=gitster@pobox$(echo .)com \
    --cc=antaigroupltda@gmail$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=me@ttaylorr$(echo .)com \
    --cc=peff@peff$(echo .)net \
    --cc=ps@pks$(echo .)im \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox