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

On Mon, Feb 15, 2016 at 10:15 PM, Jeff King [off-list ref] wrote:

On Mon, Feb 15, 2016 at 09:17:05PM -0500, Eric Sunshine wrote:

quoted

quoted

-       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.