Re: [PATCH 56/67] avoid sprintf and strcpy with flex arrays

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 20, 2015, 22:48 UTC
Message-ID: <CAPig+cSL3W58TSYEAFz3twvxt_brB=kY=LEwX6m5RhBsg6VV6g@mail.gmail.com>
In-Reply-To: <20150915160956.GD29753@sigill.intra.peff.net>

On Tue, Sep 15, 2015 at 12:09 PM, Jeff King <peff@peff.net> wrote:

Show 47 quoted lines

> 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@peff.net>
> ---
> 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...

Show 7 quoted lines

>         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?

Show 20 quoted lines

>         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?

Show 20 quoted lines

> +       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;