[0/2] avoiding recursion in mailinfo

Message

Jeff King Dec. 14, 2023, 9:44 p.m. UTC

On Thu, Dec 14, 2023 at 08:41:20AM +0100, Patrick Steinhardt wrote:

> > @@ -72,11 +73,14 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in)
> >  				take_next_literally = 1;
> >  				continue;
> >  			case '(':
> > -				in = unquote_comment(outbuf, in);
> > +				strbuf_addch(outbuf, '(');
> > +				depth++;
> >  				continue;
> >  			case ')':
> >  				strbuf_addch(outbuf, ')');
> > -				return in;
> > +				if (!--depth)
> > +					return in;
> > +				continue;
> >  			}
> >  		}
> >  
> > I doubt it's a big deal either way, but if it's that easy to do it might
> > be worth it.
> 
> Isn't this only protecting against unbalanced braces? That might be a
> sensible check to do regardless, but does it protect against recursion
> blowing the stack if you just happen to have many opening braces?
> 
> Might be I'm missing something.

It protects against recrusion blowing the stack because it removes the
recursive call to unquote_comment(). ;)

The "depth" stuff is there because without recursion, we have to know
when we've hit the correct closing paren (as opposed to an embedded
one).

Here it is as a patch (actually two patches). I don't think it's high
priority, but I'd sunk enough brain cells into thinking about it that I
wanted to capture the work. ;)

I did it on top of the earlier mailinfo out-of-bounds read fix, but it
could be applied separately.

  [1/2]: t5100: make rfc822 comment test more careful
  [2/2]: mailinfo: avoid recursion when unquoting From headers

 mailinfo.c             | 8 ++++++--
 t/t5100/comment.expect | 2 +-
 t/t5100/comment.in     | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

-Peff

Comments

Patrick Steinhardt Dec. 15, 2023, 7:58 a.m. UTC | #1

On Thu, Dec 14, 2023 at 04:44:44PM -0500, Jeff King wrote:
> On Thu, Dec 14, 2023 at 08:41:20AM +0100, Patrick Steinhardt wrote:
> 
> > > @@ -72,11 +73,14 @@ static const char *unquote_comment(struct strbuf *outbuf, const char *in)
> > >  				take_next_literally = 1;
> > >  				continue;
> > >  			case '(':
> > > -				in = unquote_comment(outbuf, in);
> > > +				strbuf_addch(outbuf, '(');
> > > +				depth++;
> > >  				continue;
> > >  			case ')':
> > >  				strbuf_addch(outbuf, ')');
> > > -				return in;
> > > +				if (!--depth)
> > > +					return in;
> > > +				continue;
> > >  			}
> > >  		}
> > >  
> > > I doubt it's a big deal either way, but if it's that easy to do it might
> > > be worth it.
> > 
> > Isn't this only protecting against unbalanced braces? That might be a
> > sensible check to do regardless, but does it protect against recursion
> > blowing the stack if you just happen to have many opening braces?
> > 
> > Might be I'm missing something.
> 
> It protects against recrusion blowing the stack because it removes the
> recursive call to unquote_comment(). ;)

Doh. Of course it does, I completely missed the removals.

> The "depth" stuff is there because without recursion, we have to know
> when we've hit the correct closing paren (as opposed to an embedded
> one).

Yes, makes sense now. The patches look good to me, thanks!

Patrick