Re: [PATCH] show-branch: use strbuf instead of static buffer

From: Jeff King <peff@peff.net>
Date: Apr 6, 2013, 04:58 UTC
Message-ID: <20130406045833.GB26544@sigill.intra.peff.net>
In-Reply-To: <20130405234915.GU30308@google.com>

On Fri, Apr 05, 2013 at 04:49:15PM -0700, Jonathan Nieder wrote:

Show 8 quoted lines

> > Though this is a stack overflow, I don't know that it's exploitable for
> > anything interesting; an attacker does not get to write arbitrary data,
> > but rather only a sequence of "^%d" and "~%d" relative history markers.
> > Perhaps in theory one could devise a history such that the sequence
> > markers spelled out some malicious code, but it would be quite a
> > challenge
> 
> Overwrite the return address and return-to-libc?

Still hard, since you need to construct a usable address (and arguments) out of sequences of "^[0-9]+" and "~[0-9]+". But I'd love to see a working exploit if somebody thinks they can do it. :)

> Very clean and obviously correct.  Thanks.
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

> A test would be nice, though.

What should it be testing? That a giant chain of second-parent merges that exceeds 1000 bytes doesn't segfault? Tests like that are not all that interesting, because they do not catch real-world regressions. We have closed this barn door; it is not impossible that it will be re-opened, but it is not likely. A test that checks only for a very specific type of failure is only ever going to see that failure.

If you want to design a suite of tests that check that show-branch gives correct output for particular brands of large repo, that would be generic and potentially useful. But I don't think it's actually worth spending a lot of time on (reviewing the code for more static buffers and sprintfs would probably be a much more fruitful use of time).

-Peff