On Thu, Feb 16, 2017 at 01:40:00PM +0100, Christian Couder wrote: > > 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). > 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". > > 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