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

On Mon, Sep 21, 2015 at 01:11:09PM -0400, Eric Sunshine wrote:

quoted

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 think it was more that I looked at other instances of packed_git, and
realized they could not be safely converted. I think "struct
alternate_object_database" has similar problems.

-Peff

PS As I mentioned earlier, I did end up adding a FLEX_ALLOC() macro in
   another series that builds on top of this. I haven't posted it yet,
   but check out:

     https://github.com/peff/git/commit/ba491c527572c763286b4b9519aef3c30482c2d1

   and

     https://github.com/peff/git/commit/d88444d5ba00bd875ef5291dca3b71dd046186dc

   if you are curious.