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.
We do lose the setting of the final byte to '\0' that xmallocz does, but
it doesn't matter here because xsnprintf will add the NUL itself.
> > 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?
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.
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.
> > - strcpy((char *)update->refname, refname);
> > + memcpy((char *)update->refname, refname, len); /* includese NUL */
>
> s/includese/includes/
Thanks, fixed.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html