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

> On Sun, Sep 20, 2015 at 06:48:32PM -0400, Eric Sunshine wrote:
>> > 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?
>
> 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.

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