avoid sprintf and strcpy with flex arrays

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/archive.c b/archive.c
index 01b0899b3f9027c5a392bd58667ed0e216d082ed..4ac86c837384c45c33dae141c3ff53dd37eb0fd5 100644 (file)
--- 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);
        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);
        hashcpy(d->oid.hash, sha1);
 }
 

diff --git a/builtin/blame.c b/builtin/blame.c
index e253ac0dcb858c9911bf3dbf9dcb11dbcbfbfc84..e70fb6dac3bd24ce4ea8abf355c1c50c9a669329 100644 (file)
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -459,12 +459,13 @@ static void queue_blames(struct scoreboard *sb, struct origin *porigin,
 static struct origin *make_origin(struct commit *commit, const char *path)
 {
        struct origin *o;
-       o = xcalloc(1, sizeof(*o) + strlen(path) + 1);
+       size_t pathlen = strlen(path) + 1;
+       o = xcalloc(1, sizeof(*o) + pathlen);
        o->commit = commit;
        o->refcnt = 1;
        o->next = commit->util;
        commit->util = o;
-       strcpy(o->path, path);
+       memcpy(o->path, path, pathlen); /* includes NUL */
        return o;
 }
 

diff --git a/fast-import.c b/fast-import.c
index d0c25024cd4257f4eb8f74879ef744aee004fbeb..895c6b4a7ee6925fd2a852a612616cc8f5ece9c9 100644 (file)
--- 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;
+       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 c2709de2a0ac512e32eb18f70ca2f45d8f1d98c8..9937a40484503b068f52c940e8b62325b9edfae0 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -2695,7 +2695,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
                int namelen = strlen(entry->name) + 1;
                struct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);
                hashcpy(n->sha1, entry->u.value.oid.hash);
-               strcpy(n->name, entry->name);
+               memcpy(n->name, entry->name, namelen); /* includes NUL */
                n->next = cb->ref_to_prune;
                cb->ref_to_prune = n;
        }
@@ -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); /* includes NUL */
        ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
        transaction->updates[transaction->nr++] = update;
        return update;

diff --git a/sha1_file.c b/sha1_file.c
index 4211af1d89be432bf0a232557f0b6874a5f271f0..cc3de244ebfd5f12cde8180e17cb8ec320c981dd 100644 (file)
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1180,9 +1180,10 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local)
 struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)
 {
        const char *path = sha1_pack_name(sha1);
-       struct packed_git *p = alloc_packed_git(strlen(path) + 1);
+       int alloc = strlen(path) + 1;
+       struct packed_git *p = alloc_packed_git(alloc);
 
-       strcpy(p->pack_name, path);
+       memcpy(p->pack_name, path, alloc); /* includes NUL */
        hashcpy(p->sha1, sha1);
        if (check_packed_git_idx(idx_path, p)) {
                free(p);

diff --git a/submodule.c b/submodule.c
index 245ed4dfbb6a3d54adbd41d5de0ea64cf38dc8a3..c480ed53b446e17ec3a25624846b018e4ae7c9f2 100644 (file)
--- a/submodule.c
+++ b/submodule.c
@@ -122,6 +122,7 @@ static int add_submodule_odb(const char *path)
        struct strbuf objects_directory = STRBUF_INIT;
        struct alternate_object_database *alt_odb;
        int ret = 0;
+       int alloc;
        const char *git_dir;
 
        strbuf_addf(&objects_directory, "%s/.git", path);
@@ -142,9 +143,10 @@ static int add_submodule_odb(const char *path)
                                        objects_directory.len))
                        goto done;
 
-       alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
+       alloc = objects_directory.len + 42; /* for "12/345..." sha1 */
+       alt_odb = xmalloc(sizeof(*alt_odb) + alloc);
        alt_odb->next = alt_odb_list;
-       strcpy(alt_odb->base, objects_directory.buf);
+       xsnprintf(alt_odb->base, alloc, "%s", objects_directory.buf);
        alt_odb->name = alt_odb->base + objects_directory.len;
        alt_odb->name[2] = '/';
        alt_odb->name[40] = '\0';