Re: [PATCH 46/67] write_loose_object: convert to strbuf

[Jeff King]

On Wed, Sep 16, 2015 at 02:27:57PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > -   memcpy(buffer, filename, dirlen);
> > -   strcpy(buffer + dirlen, "tmp_obj_XX");
> > -   fd = git_mkstemp_mode(buffer, 0444);
> > +   strbuf_reset(tmp);
> > +   strbuf_add(tmp, filename, dirlen);
> > +   strbuf_addstr(tmp, "tmp_obj_XX");
> > +   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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 46/67] write_loose_object: convert to strbuf

[Junio C Hamano]

Jeff King  writes:

> - memcpy(buffer, filename, dirlen);
> - strcpy(buffer + dirlen, "tmp_obj_XX");
> - fd = git_mkstemp_mode(buffer, 0444);
> + strbuf_reset(tmp);
> + strbuf_add(tmp, filename, dirlen);
> + strbuf_addstr(tmp, "tmp_obj_XX");
> + 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.

An unsuccessful call to git_mkstemp_mode() would destroy the
template buffer by placing a NUL at the beginning of it (and I was
confused because I did not read the unwritten "at the beginning" in
"put a NUL in our buffer" above).

The patch looks good.  Thanks.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 46/67] write_loose_object: convert to strbuf

[Jeff King]

When creating a loose object tempfile, we use a fixed
PATH_MAX-sized buffer, and strcpy directly into it. This
isn't buggy, because we do a rough check of the size, but
there's no verification that our guesstimate of the required
space is enough (in fact, it's several bytes too big for the
current naming scheme).

Let's switch to a strbuf, which makes this much easier to
verify. The allocation overhead should be negligible, since
we are replacing a static buffer with a static strbuf, and
we'll only need to allocate on the first call.

While we're here, we can also document a subtle interaction
with mkstemp that would be easy to overlook.

Signed-off-by: Jeff King 
---
 sha1_file.c | 41 +
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 9d87b69..374a996 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3007,29 +3007,30 @@ static inline int directory_size(const char *filename)
  * We want to avoid cross-directory filename renames, because those
  * can have problems on various filesystems (FAT, NFS, Coda).
  */
-static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)
+static int create_tmpfile(struct strbuf *tmp, const char *filename)
 {
int fd, dirlen = directory_size(filename);
 
-   if (dirlen + 20 > bufsiz) {
-   errno = ENAMETOOLONG;
-   return -1;
-   }
-   memcpy(buffer, filename, dirlen);
-   strcpy(buffer + dirlen, "tmp_obj_XX");
-   fd = git_mkstemp_mode(buffer, 0444);
+   strbuf_reset(tmp);
+   strbuf_add(tmp, filename, dirlen);
+   strbuf_addstr(tmp, "tmp_obj_XX");
+   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;
-   if (adjust_shared_perm(buffer))
+   if (adjust_shared_perm(tmp->buf))
return -1;
 
/* Try again */
-   strcpy(buffer + dirlen - 1, "/tmp_obj_XX");
-   fd = git_mkstemp_mode(buffer, 0444);
+   strbuf_addstr(tmp, "/tmp_obj_XX");
+   fd = git_mkstemp_mode(tmp->buf, 0444);
}
return fd;
 }
@@ -3042,10 +3043,10 @@ static int write_loose_object(const unsigned char 
*sha1, char *hdr, int hdrlen,
git_zstream stream;
git_SHA_CTX c;
unsigned char parano_sha1[20];
-   static char tmp_file[PATH_MAX];
+   static struct strbuf tmp_file = STRBUF_INIT;
const char *filename = sha1_file_name(sha1);
 
-   fd = create_tmpfile(tmp_file, sizeof(tmp_file), filename);
+   fd = create_tmpfile(_file, filename);
if (fd < 0) {
if (errno == EACCES)
return error("insufficient permission for adding an 
object to repository database %s", get_object_directory());
@@ -3094,12 +3095,12 @@ static int write_loose_object(const unsigned char 
*sha1, char *hdr, int hdrlen,
struct utimbuf utb;
utb.actime = mtime;
utb.modtime = mtime;
-   if (utime(tmp_file, ) < 0)
+   if (utime(tmp_file.buf, ) < 0)
warning("failed utime() on %s: %s",
-   tmp_file, strerror(errno));
+   tmp_file.buf, strerror(errno));
}
 
-   return finalize_object_file(tmp_file, filename);
+   return finalize_object_file(tmp_file.buf, filename);
 }
 
 static int freshen_loose_object(const unsigned char *sha1)
-- 
2.6.0.rc2.408.ga2926b9

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html