On Mon, Feb 15, 2016 at 10:15 PM, Jeff King <peff@xxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html