odb_mkstemp: write filename into strbuf

The odb_mkstemp() function expects the caller to provide a
fixed buffer to write the resulting tempfile name into. But
it creates the template using snprintf without checking the
return value. This means we could silently truncate the
filename.

In practice, it's unlikely that the truncation would end in
the template-pattern that mkstemp needs to open the file. So
we'd probably end up failing either way, unless the path was
specially crafted.

The simplest fix would be to notice the truncation and die.
However, we can observe that most callers immediately
xstrdup() the result anyway. So instead, let's switch to
using a strbuf, which is easier for them (and isn't a big
deal for the other 2 callers, who can just strbuf_release
when they're done with it).

Note that many of the callers used static buffers, but this
was purely to avoid putting a large buffer on the stack. We
never passed the static buffers out of the function, so
there's no complicated memory handling we need to change.

Signed-off-by: Jeff King <peff@peff.net>

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 49e7175d9aebf5a1964fd6d61d9509a022476df1..f4af2ab37ab66a118c4cdd72354b53e921a10763 100644 (file)
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -307,10 +307,10 @@ static const char *open_pack_file(const char *pack_name)
        if (from_stdin) {
                input_fd = 0;
                if (!pack_name) {
-                       static char tmp_file[PATH_MAX];
-                       output_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),
+                       struct strbuf tmp_file = STRBUF_INIT;
+                       output_fd = odb_mkstemp(&tmp_file,
                                                "pack/tmp_pack_XXXXXX");
-                       pack_name = xstrdup(tmp_file);
+                       pack_name = strbuf_detach(&tmp_file, NULL);
                } else {
                        output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);
                        if (output_fd < 0)

diff --git a/cache.h b/cache.h
index 6c01e2746833afb395c2ac4d42716289b08b95fd..4f3bdd0b5d4f29cb3d263519a7303acd2b99cc2a 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -1679,7 +1679,7 @@ extern void pack_report(void);
  * usual "XXXXXX" trailer, and the resulting filename is written into the
  * "template" buffer. Returns the open descriptor.
  */
-extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
+extern int odb_mkstemp(struct strbuf *template, const char *pattern);
 
 /*
  * Generate the filename to be used for a pack file with checksum "sha1" and

diff --git a/environment.c b/environment.c
index 2fdba76222b8f5744f1ae84fd11e72a7a3c115cf..88276790db4175f710597b5c69cc1757e302eecd 100644 (file)
--- a/environment.c
+++ b/environment.c
@@ -274,7 +274,7 @@ char *get_object_directory(void)
        return git_object_dir;
 }
 
-int odb_mkstemp(char *template, size_t limit, const char *pattern)
+int odb_mkstemp(struct strbuf *template, const char *pattern)
 {
        int fd;
        /*
@@ -282,18 +282,18 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern)
         * restrictive except to remove write permission.
         */
        int mode = 0444;
-       snprintf(template, limit, "%s/%s",
-                get_object_directory(), pattern);
-       fd = git_mkstemp_mode(template, mode);
+       strbuf_reset(template);
+       strbuf_addf(template, "%s/%s", get_object_directory(), pattern);
+       fd = git_mkstemp_mode(template->buf, mode);
        if (0 <= fd)
                return fd;
 
        /* slow path */
        /* some mkstemp implementations erase template on failure */
-       snprintf(template, limit, "%s/%s",
-                get_object_directory(), pattern);
-       safe_create_leading_directories(template);
-       return xmkstemp_mode(template, mode);
+       strbuf_reset(template);
+       strbuf_addf(template, "%s/%s", get_object_directory(), pattern);
+       safe_create_leading_directories(template->buf);
+       return xmkstemp_mode(template->buf, mode);
 }
 
 int odb_pack_keep(const char *name)

diff --git a/fast-import.c b/fast-import.c
index f6f416f20a9f4c05f90690fcd7bfe049a8c91467..1ea3a08609bd8ece7a22ce33c78a0a798304375f 100644 (file)
--- a/fast-import.c
+++ b/fast-import.c
@@ -890,14 +890,15 @@ static struct tree_content *dup_tree_content(struct tree_content *s)
 
 static void start_packfile(void)
 {
-       static char tmp_file[PATH_MAX];
+       struct strbuf tmp_file = STRBUF_INIT;
        struct packed_git *p;
        struct pack_header hdr;
        int pack_fd;
 
-       pack_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),
-                             "pack/tmp_pack_XXXXXX");
-       FLEX_ALLOC_STR(p, pack_name, tmp_file);
+       pack_fd = odb_mkstemp(&tmp_file, "pack/tmp_pack_XXXXXX");
+       FLEX_ALLOC_STR(p, pack_name, tmp_file.buf);
+       strbuf_release(&tmp_file);
+
        p->pack_fd = pack_fd;
        p->do_not_close = 1;
        pack_file = sha1fd(pack_fd, p->pack_name);

diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 44492c346ce530ab156c128c5377245a724671d1..e313f4f2bc69f967ec4354e80cd62b329c7bcb21 100644 (file)
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -508,16 +508,16 @@ void bitmap_writer_finish(struct pack_idx_entry **index,
                          const char *filename,
                          uint16_t options)
 {
-       static char tmp_file[PATH_MAX];
        static uint16_t default_version = 1;
        static uint16_t flags = BITMAP_OPT_FULL_DAG;
+       struct strbuf tmp_file = STRBUF_INIT;
        struct sha1file *f;
 
        struct bitmap_disk_header header;
 
-       int fd = odb_mkstemp(tmp_file, sizeof(tmp_file), "pack/tmp_bitmap_XXXXXX");
+       int fd = odb_mkstemp(&tmp_file, "pack/tmp_bitmap_XXXXXX");
 
-       f = sha1fd(fd, tmp_file);
+       f = sha1fd(fd, tmp_file.buf);
 
        memcpy(header.magic, BITMAP_IDX_SIGNATURE, sizeof(BITMAP_IDX_SIGNATURE));
        header.version = htons(default_version);
@@ -537,9 +537,11 @@ void bitmap_writer_finish(struct pack_idx_entry **index,
 
        sha1close(f, NULL, CSUM_FSYNC);
 
-       if (adjust_shared_perm(tmp_file))
+       if (adjust_shared_perm(tmp_file.buf))
                die_errno("unable to make temporary bitmap file readable");
 
-       if (rename(tmp_file, filename))
+       if (rename(tmp_file.buf, filename))
                die_errno("unable to rename temporary bitmap file to '%s'", filename);
+
+       strbuf_release(&tmp_file);
 }

diff --git a/pack-write.c b/pack-write.c
index 485080a31a512dea7fff58a925b5db45973fe3f1..fa97b72559883a38be147fab737f6b7adae5975b 100644 (file)
--- a/pack-write.c
+++ b/pack-write.c
@@ -71,9 +71,9 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
                f = sha1fd_check(index_name);
        } else {
                if (!index_name) {
-                       static char tmp_file[PATH_MAX];
-                       fd = odb_mkstemp(tmp_file, sizeof(tmp_file), "pack/tmp_idx_XXXXXX");
-                       index_name = xstrdup(tmp_file);
+                       struct strbuf tmp_file = STRBUF_INIT;
+                       fd = odb_mkstemp(&tmp_file, "pack/tmp_idx_XXXXXX");
+                       index_name = strbuf_detach(&tmp_file, NULL);
                } else {
                        unlink(index_name);
                        fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
@@ -329,11 +329,11 @@ int encode_in_pack_object_header(unsigned char *hdr, int hdr_len,
 
 struct sha1file *create_tmp_packfile(char **pack_tmp_name)
 {
-       char tmpname[PATH_MAX];
+       struct strbuf tmpname = STRBUF_INIT;
        int fd;
 
-       fd = odb_mkstemp(tmpname, sizeof(tmpname), "pack/tmp_pack_XXXXXX");
-       *pack_tmp_name = xstrdup(tmpname);
+       fd = odb_mkstemp(&tmpname, "pack/tmp_pack_XXXXXX");
+       *pack_tmp_name = strbuf_detach(&tmpname, NULL);
        return sha1fd(fd, *pack_tmp_name);
 }