On Mon, Feb 15, 2016 at 09:17:05PM -0500, Eric Sunshine wrote:
> > ---
> > diff --git a/builtin/reflog.c b/builtin/reflog.c
> > @@ -412,8 +410,7 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len)
> > !memcmp(ent->pattern, pattern, len))
> > return ent;
> >
> > - 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.
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)
So this is in fact fixing a bug. I can't say I'm terribly surprised
nobody noticed it, as per-ref reflog expiration is pretty obscure.
I hope this increases confidence in my patch series. Even though I
didn't _know_ there was a bug here, I did know that malloc computations
are a potential source of errors. And the FLEX_ALLOC helpers are
designed to remove that work and have a simple interface. We don't know
whether the caller will want a NUL afterwards or not, but we err on the
side of over-allocating by a byte (just as we over-allocate
read_sha1_file() output by a byte), because safety is better than
squeezing out a single byte.
> > diff --git a/hashmap.c b/hashmap.c
> > @@ -256,10 +256,9 @@ const void *memintern(const void *data, size_t len)
> > e = hashmap_get(&map, &key, data);
> > if (!e) {
> > /* not found: create it */
> > - e = xmallocz(sizeof(struct pool_entry) + len);
> > + FLEX_ALLOC_MEM(e, data, data, len);
>
> Ditto. I guess the new code is overallocating (which should be okay).
It is, but so was the original (it used xmallocz to get an extra NUL).
-Peff
--
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