Re: [PATCH 07/18] convert trivial cases to FLEX_ARRAY macros

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Feb 15, 2016 at 10:36 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 15, 2016 at 10:26:26PM -0500, Jeff King wrote:
>> We could do this on top of my series (I can also factor out the fix
>> separately to go at the beginning if we don't want to hold the bugfix
>> hostage).
>>
>> -- >8 --
>> Subject: [PATCH] reflog_expire_cfg: drop misleading "len" parameter
>
> Here it is as a separate fix. Applying my series on top would need a
> minor and obvious tweak. I'll hold a re-roll for more comments, but will
> otherwise plan to stick this at the front of the series.

Yep, I prefer this version of the patch too, as it makes it explicit
that a bug is being fixed rather than it happening "by accident" via
the FLEX_ALLOC_MEM conversion, which is easily overlooked.

> -- >8 --
> Subject: [PATCH] reflog_expire_cfg: NUL-terminate pattern field
>
> You can tweak the reflog expiration for a particular subset
> of refs by configuring gc.foo.reflogexpire. We keep a linked
> list of reflog_expire_cfg structs, each of which holds the
> pattern and a "len" field for the length of the pattern. The
> pattern itself is _not_ NUL-terminated.
>
> However, we feed the pattern directly to wildmatch(), which
> expects a NUL-terminated string, meaning it may keep reading
> random junk after our struct.
>
> We can fix this by allocating an extra byte for the NUL
> (which is already zero because we use xcalloc). Let's also
> drop the misleading "len" field, which is no longer
> necessary. The existing use of "len" can be converted to use
> strncmp().
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> @@ -408,13 +407,12 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len)
>                 reflog_expire_cfg_tail = &reflog_expire_cfg;
>
>         for (ent = reflog_expire_cfg; ent; ent = ent->next)
> -               if (ent->len == len &&
> -                   !memcmp(ent->pattern, pattern, len))
> +               if (!strncmp(ent->pattern, pattern, len) &&
> +                   ent->pattern[len] == '\0')

If 'ent->pattern' is shorter than 'pattern' then the strncmp() will
fail, thus it will short-circuit before ent->pattern[len] has a chance
to access beyond the end of memory allocated for 'ent->pattern'. Okay,
makes sense.

>                         return ent;
>
> -       ent = xcalloc(1, (sizeof(*ent) + len));
> +       ent = xcalloc(1, sizeof(*ent) + len + 1);
>         memcpy(ent->pattern, pattern, len);
> -       ent->len = len;
>         *reflog_expire_cfg_tail = ent;
>         reflog_expire_cfg_tail = &(ent->next);
>         return ent;
> --
> 2.7.1.574.gccd43a9