Re: [PATCH] show-branch: fix crash with long ref name

From: Jeff King <peff@peff.net>
Date: Feb 17, 2017, 05:03 UTC
Message-ID: <20170217050339.i4s2esabk3uvi5yb@sigill.intra.peff.net>
In-Reply-To: <CAP8UFD20=zVy=1Tr4D1Rxf+a4yR_X2zmexNCTNKV5RSF9ueYrg@mail.gmail.com>

On Thu, Feb 16, 2017 at 01:40:00PM +0100, Christian Couder wrote:

Show 13 quoted lines

> > I started to add some tests, but I had second thoughts. It _is_ nice
> > to show off the fix, but as far as regressions go, this specific case is
> > unlikely to come up again. What would be more valuable, I think, is a
> > test script which set up a very long refname (not just 150 bytes or
> > whatever) and ran it through a series of git commands.
> 
> I agree that a test script running through a series of command with
> long refnames would be great.
> 
> But I think the refname should not necesarily be too long. As I wrote
> in the commit message of my patch, if the ref name had been much
> longer the crash would not have happened because the ref could not
> have been created in the first place.

Right, I think there's a tension there. Too short and it is not interesting, and too long and things start to fail for uninteresting reasons (e.g., your filesystem can't handle the name).

Show 5 quoted lines

> So the best would be to run through a series of commands with a
> refname ranging from let's say 80 chars to 300 chars.
> 
> That would have a chance to catch crashes due to legacy code using for
> example things like `char stuff[128]` or `char stuff[256]`.

But that doesn't catch `char stuff[512]`. I think you'd really want a range of sizes, and to test all of them. Worse, though, is it's not clear how you decide when a test has failed. Obviously smashing the stack is a bad outcome, but part of the goal would presumably be to flush out unnecessary length limitations elsewhere.

I got about that far in my thinking before saying "wow, this is getting to be complicated for not much gain".

Show 7 quoted lines

> > So I dunno. It seems like being thorough is a
> > lot of hassle for not much gain. Being not-thorough is easy, but is
> > mostly a token that is unlikely to find any real bugs.
> 
> Yeah, if we really care, it might be better to start using a fuzzer or
> a property based testing tool instead of bothering with these kind of
> tests by ourselves, which is also a different topic.

Yes, I'd agree that a fuzzer is probably a better choice. I played with AFL a bit back when it came out, but I never got it to turn up anything useful.

I am disappointed that this obvious memory problem survived for so long. I did quite a bit of auditing for such problems a year or two ago, but I focused on problematic functions like strcpy, sprintf, etc.

It's easy to use memcpy() wrong, too, but it's hard to audit. There are a lot of calls, and you have to match up the copied length with a value computed elsewhere. I traded a lot of them out back then for safer variants (like the FLEX_ALLOC stuff), but many calls just fundamentally need something unsafe like memcpy to get their job done.

-Peff