Re: [PATCH 07/18] convert trivial cases to FLEX_ARRAY macros
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:08:15
On Mon, Feb 15, 2016 at 10:36 PM, Jeff King [off-list ref] wrote:
On Mon, Feb 15, 2016 at 10:26:26PM -0500, Jeff King wrote:quoted
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" parameterHere 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.
quoted hunk ↗ jump to hunk
-- >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 <redacted> ---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