From: Eric Sunshine <sunshine@sunshineco.com>
Date: Tue, 16 Feb 2016 04:10:41 GMT
Subject: Re: [PATCH 07/18] convert trivial cases to FLEX_ARRAY macros
Message-ID: <CAPig+cSyz7uYeHu=nP_V+6LaaWJZ-Bv6CR=9hmF8_cf74mnuHA@mail.gmail.com>
In-Reply-To: <20160216031554.GB13606@sigill.intra.peff.net>

On Mon, Feb 15, 2016 at 10:15 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 15, 2016 at 09:17:05PM -0500, Eric Sunshine wrote:
>> > -       ent = xcalloc(1, (sizeof(*ent) + len));
>> > -       memcpy(ent->pattern, pattern, len);
>> > +       FLEX_ALLOC_MEM(ent, pattern, pattern, len);
>>
>> Does the incoming 'len' already account for the NUL terminator, or was
>> the original code underallocating?
>>
>> Answering my own question: Looking at reflog_expire_config() and
>> parse_config_key(), I gather that 'len' already accounts for the NUL,
>> thus the new code is overallocating (which should not be a problem).
>
> Actually, I think the original underallocates. If we have
> gc.foobar.reflogExpire, then "pattern" will poitn to "foobar" and "len"
> will be 6. Meaning we allocate without a trailing NUL.

Ugh, yeah, I misread the code.

> That _should_ be OK, because the struct has a "len" field, and readers
> can be careful not to go past it. And indeed, in the loop above, we
> check the length and use memcmp().
>
> But later, in set_reflog_expiry_param(), we walk through the list and
> hand ent->pattern directly to wildmatch, which assumes a NUL-terminated
> string. In practice, it probably works out 7 out of 8 times, because
> malloc will align the struct, and we're on a zeroed page, so unless the
> string is exactly 8 characters, we'll get some extra NULs afterwards.
> But I could demonstrate it by doing:
>
>   gdb --args git -c gc.foobar12.reflogexpire=never reflog expire --all
>
> and breaking on wildmatch, which yields:
>
>   Breakpoint 1, wildmatch (pattern=0x85eb70 "foobar12Q", text=0x85e4d4
>         "refs/heads/master", flags=0, wo=0x0)

Nice confirmation.