On Mon, Feb 15, 2016 at 10:36 PM, Jeff King <peff@xxxxxxxx> 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@xxxxxxxx> > --- > 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 -- 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