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

On Mon, Sep 21, 2015 at 11:15 AM, Jeff King [off-list ref] wrote:

On Sun, Sep 20, 2015 at 06:48:32PM -0400, Eric Sunshine wrote:

quoted

quoted

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

quoted

        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?

Not quite. The original uses xmallocz, which handles the NUL itself. But
the purpose of this patch is to pull the total length into a variable
that we can use both for the malloc and for the xsnprintf, so we have
to account for it ourselves.

Makes sense. I missed the "z" when reading the old code. Thanks for
the explanation.

quoted

quoted

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

To be honest, I'm not sure what's going on with the "+ 2" here.

In many cases with packed_git we allocate with "foo.idx" and want to be
able to later write "foo.pack" into the same buffer. But here we are
putting in a tmpfile name. This comes from 8455e48, but I don't see any
clue there. I wonder if the "+2" was simply cargo-culted from other
instances.

Ah, ok. I guess I misunderstood the commit message to mean or imply
that the +2 was correct and sensible and well-understood.

I'm loath to change it in the middle of this patch, because it would be
hard to see amidst the other changes. I'd rather make this a straight
conversion, and worry about it in a separate patch.