On Wed, Sep 16, 2015 at 02:27:57PM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > - memcpy(buffer, filename, dirlen);
> > - strcpy(buffer + dirlen, "tmp_obj_XXXXXX");
> > - fd = git_mkstemp_mode(buffer, 0444);
> > + strbuf_reset(tmp);
> > + strbuf_add(tmp, filename, dirlen);
> > + strbuf_addstr(tmp, "tmp_obj_XXXXXX");
> > + fd = git_mkstemp_mode(tmp->buf, 0444);
> > if (fd < 0 && dirlen && errno == ENOENT) {
> > - /* Make sure the directory exists */
> > - memcpy(buffer, filename, dirlen);
> > - buffer[dirlen-1] = 0;
> > - if (mkdir(buffer, 0777) && errno != EEXIST)
> > + /*
> > + * Make sure the directory exists; note that mkstemp will have
> > + * put a NUL in our buffer, so we have to rewrite the path,
> > + * rather than just chomping the length.
> > + */
> > + strbuf_reset(tmp);
> > + strbuf_add(tmp, filename, dirlen - 1);
> > + if (mkdir(tmp->buf, 0777) && errno != EEXIST)
> > return -1;
>
> I had to read the patch three times before understanding what the
> business with NUL in this comment is about.
>
> The old code was doing the same thing, i.e. instead of attempting to
> reuse the early part of buffer[] it copied the early part of
> filename[] there again, exactly for the same reason, but it didn't
> even explain why the copy was necessary. Now the new code explains
> why strbuf_setlen() is not used here pretty nicely.
Exactly (I found this out the hard way by trying to clean that up, and
learned something new about mkstemp).
Mentioning the NUL is probably unnecessarily confusing. That is what our
gitmkstemp does, but mkstemp(3) says "undefined" on my system (POSIX
does not mention it at all, but the NUL seems like a reasonable safety
in case any callers ignore the return value).
I've updated this to:
/*
* Make sure the directory exists; note that the contents
* of the buffer are undefined after mkstemp returns an
* error, so we have to rewrite the whole buffer from
* scratch.
*/
-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