On Tue, Sep 15, 2015 at 12:09 PM, Jeff King <peff@xxxxxxxx> wrote:
> When we are allocating a struct with a FLEX_ARRAY member, we
> generally compute the size of the array and then sprintf or
> strcpy into it. Normally we could improve a dynamic allocation
> like this by using xstrfmt, but it doesn't work here; we
> have to account for the size of the rest of the struct.
>
> But we can improve things a bit by storing the length that
> we use for the allocation, and then feeding it to xsnprintf
> or memcpy, which makes it more obvious that we are not
> writing more than the allocated number of bytes.
>
> It would be nice if we had some kind of helper for
> allocating generic flex arrays, but it doesn't work that
> well:
>
> - the call signature is a little bit unwieldy:
>
> d = flex_struct(sizeof(*d), offsetof(d, path), fmt, ...);
>
> You need offsetof here instead of just writing to the
> end of the base size, because we don't know how the
> struct is packed (partially this is because FLEX_ARRAY
> might not be zero, though we can account for that; but
> the size of the struct may actually be rounded up for
> alignment, and we can't know that).
>
> - some sites do clever things, like over-allocating because
> they know they will write larger things into the buffer
> later (e.g., struct packed_git here).
>
> So we're better off to just write out each allocation (or
> add type-specific helpers, though many of these are one-off
> allocations anyway).
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> diff --git a/archive.c b/archive.c
> index 01b0899..4ac86c8 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -171,13 +171,14 @@ static void queue_directory(const unsigned char *sha1,
> unsigned mode, int stage, struct archiver_context *c)
> {
> struct directory *d;
> - d = xmallocz(sizeof(*d) + base->len + 1 + strlen(filename));
> + size_t len = base->len + 1 + strlen(filename) + 1;
> + d = xmalloc(sizeof(*d) + len);
Mental note: The new code makes this one longer than the original code...
> d->up = c->bottom;
> d->baselen = base->len;
> d->mode = mode;
> d->stage = stage;
> c->bottom = d;
> - d->len = sprintf(d->path, "%.*s%s/", (int)base->len, base->buf, filename);
> + d->len = xsnprintf(d->path, len, "%.*s%s/", (int)base->len, base->buf, filename);
Considering that we need space for the '/' and the NUL, the new code
seems to be correct, and the old code was under-allocating, right?
> hashcpy(d->oid.hash, sha1);
> }
>
> diff --git a/fast-import.c b/fast-import.c
> index d0c2502..895c6b4 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -863,13 +863,15 @@ static void start_packfile(void)
> {
> static char tmp_file[PATH_MAX];
> struct packed_git *p;
> + int namelen;
> struct pack_header hdr;
> int pack_fd;
>
> pack_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),
> "pack/tmp_pack_XXXXXX");
> - p = xcalloc(1, sizeof(*p) + strlen(tmp_file) + 2);
> - strcpy(p->pack_name, tmp_file);
> + namelen = strlen(tmp_file) + 2;
You mentioned this specially in the commit message, but from a brief
read of the code, it's still not obvious (to me) why this is +2 rather
than +1. Since you're touching the code anyhow, perhaps add an in-code
comment explaining it?
> + p = xcalloc(1, sizeof(*p) + namelen);
> + xsnprintf(p->pack_name, namelen, "%s", tmp_file);
> p->pack_fd = pack_fd;
> p->do_not_close = 1;
> pack_file = sha1fd(pack_fd, p->pack_name);
> diff --git a/refs.c b/refs.c
> index c2709de..df6c41a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3984,10 +3984,10 @@ void ref_transaction_free(struct ref_transaction *transaction)
> static struct ref_update *add_update(struct ref_transaction *transaction,
> const char *refname)
> {
> - size_t len = strlen(refname);
> - struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
> + size_t len = strlen(refname) + 1;
> + struct ref_update *update = xcalloc(1, sizeof(*update) + len);
>
> - strcpy((char *)update->refname, refname);
> + memcpy((char *)update->refname, refname, len); /* includese NUL */
s/includese/includes/
> ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
> transaction->updates[transaction->nr++] = update;
> return update;
--
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