Re: [PATCH 1/1] untracked cache: fix off-by-one

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 12, 2019, 01:48 UTC
Message-ID: <xmqq7ec00z9t.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190410162029.GA30818@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Yeah, every struct on a platform where FLEX_ARRAY is 1 ends up
> over-allocated by 1 byte. We could account for that in all of our flex
> allocations, but I don't it affects enough platforms to be worth the
> bother.

It was an explicit design decision to declare that the overallocation was a non-problem back when we invented FLEX_ARRAY macro. We could probably have instead decided to pass number of bytes to be stored in the flex-array (including NUL if that is a character array) then subtract the value of FLEX_ARRAY if FLEX_ARRAY were limited to only 0 or 1, but that was not workable as it is the most natural to define FLEX_ARRAY to an empty string, i.e. making an array field decl to "type field[];", for the more recent compilers.

Show 7 quoted lines

>> As there is no obvious way to trigger this bug reliably, on all
>> platforms supported by Git, and as the bug is obvious enough, this patch
>> comes without a regression test.
>
> Makes sense. This code path should be well-covered by the existing tests
> anyway, so even if we could get those tools to trigger, I don't think
> there would be much point in adding a new test.

Yeah, it is already super that this obscure bug was spotted in the first place. It is true that this may regress without a test, but I do not think it is reasonable to expect that it is possible to write a reliable crash-detecting test for this one.

Show 10 quoted lines

> The right thing is probably something like:
>
>   eos = memchr(data, '\0', end - data);
>   if (!eos)
> 	return error("malformed untracked cache extension");
>   len = eos - data;
>
> I wouldn't be at all surprised if other bits of the index code have the
> same issue, though. And at any rate, thinking about that should
> definitely not hold up your fix.

True, true. I wonder if folks intereseted in libFuzzer can chime in with something useful here, but that is totally independent.