Jeff King <peff@xxxxxxxx> 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.
>> 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.
> 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.
