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>

Jeff King committed Mar 28, 2017 at 15:45 UTC 594fa9998c41277c579a94657100fa303160aa7e
6 files changed +30 -27
builtin/index-pack.c
+3 -3
@@ -307,10 +307,10 @@ static const char *open_pack_file(const char *pack_name)
307 if (from_stdin) {
308 input_fd = 0;
309 if (!pack_name) {
310 - static char tmp_file[PATH_MAX];
311 - output_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),
310 + struct strbuf tmp_file = STRBUF_INIT;
311 + output_fd = odb_mkstemp(&tmp_file,
312 "pack/tmp_pack_XXXXXX");
313 - pack_name = xstrdup(tmp_file);
313 + pack_name = strbuf_detach(&tmp_file, NULL);
314 } else {
315 output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);
316 if (output_fd < 0)
cache.h
+1 -1
@@ -1679,7 +1679,7 @@ extern void pack_report(void);
1679 * usual "XXXXXX" trailer, and the resulting filename is written into the
1680 * "template" buffer. Returns the open descriptor.
1681 */
1682 -extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
1682 +extern int odb_mkstemp(struct strbuf *template, const char *pattern);
1683
1684 /*
1685 * Generate the filename to be used for a pack file with checksum "sha1" and
environment.c
+8 -8
@@ -274,7 +274,7 @@ char *get_object_directory(void)
274 return git_object_dir;
275 }
276
277 -int odb_mkstemp(char *template, size_t limit, const char *pattern)
277 +int odb_mkstemp(struct strbuf *template, const char *pattern)
278 {
279 int fd;
280 /*
@@ -282,18 +282,18 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern)
282 * restrictive except to remove write permission.
283 */
284 int mode = 0444;
285 - snprintf(template, limit, "%s/%s",
286 - get_object_directory(), pattern);
287 - fd = git_mkstemp_mode(template, mode);
285 + strbuf_reset(template);
286 + strbuf_addf(template, "%s/%s", get_object_directory(), pattern);
287 + fd = git_mkstemp_mode(template->buf, mode);
288 if (0 <= fd)
289 return fd;
290
291 /* slow path */
292 /* some mkstemp implementations erase template on failure */
293 - snprintf(template, limit, "%s/%s",
294 - get_object_directory(), pattern);
295 - safe_create_leading_directories(template);
296 - return xmkstemp_mode(template, mode);
293 + strbuf_reset(template);
294 + strbuf_addf(template, "%s/%s", get_object_directory(), pattern);
295 + safe_create_leading_directories(template->buf);
296 + return xmkstemp_mode(template->buf, mode);
297 }
298
299 int odb_pack_keep(const char *name)
fast-import.c
+5 -4
@@ -890,14 +890,15 @@ static struct tree_content *dup_tree_content(struct tree_content *s)
890
891 static void start_packfile(void)
892 {
893 - static char tmp_file[PATH_MAX];
893 + struct strbuf tmp_file = STRBUF_INIT;
894 struct packed_git *p;
895 struct pack_header hdr;
896 int pack_fd;
897
898 - pack_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),
899 - "pack/tmp_pack_XXXXXX");
900 - FLEX_ALLOC_STR(p, pack_name, tmp_file);
898 + pack_fd = odb_mkstemp(&tmp_file, "pack/tmp_pack_XXXXXX");
899 + FLEX_ALLOC_STR(p, pack_name, tmp_file.buf);
900 + strbuf_release(&tmp_file);
901 +
902 p->pack_fd = pack_fd;
903 p->do_not_close = 1;
904 pack_file = sha1fd(pack_fd, p->pack_name);
pack-bitmap-write.c
+7 -5
@@ -508,16 +508,16 @@ void bitmap_writer_finish(struct pack_idx_entry **index,
508 const char *filename,
509 uint16_t options)
510 {
511 - static char tmp_file[PATH_MAX];
511 static uint16_t default_version = 1;
512 static uint16_t flags = BITMAP_OPT_FULL_DAG;
513 + struct strbuf tmp_file = STRBUF_INIT;
514 struct sha1file *f;
515
516 struct bitmap_disk_header header;
517
518 - int fd = odb_mkstemp(tmp_file, sizeof(tmp_file), "pack/tmp_bitmap_XXXXXX");
518 + int fd = odb_mkstemp(&tmp_file, "pack/tmp_bitmap_XXXXXX");
519
520 - f = sha1fd(fd, tmp_file);
520 + f = sha1fd(fd, tmp_file.buf);
521
522 memcpy(header.magic, BITMAP_IDX_SIGNATURE, sizeof(BITMAP_IDX_SIGNATURE));
523 header.version = htons(default_version);
@@ -537,9 +537,11 @@ void bitmap_writer_finish(struct pack_idx_entry **index,
537
538 sha1close(f, NULL, CSUM_FSYNC);
539
540 - if (adjust_shared_perm(tmp_file))
540 + if (adjust_shared_perm(tmp_file.buf))
541 die_errno("unable to make temporary bitmap file readable");
542
543 - if (rename(tmp_file, filename))
543 + if (rename(tmp_file.buf, filename))
544 die_errno("unable to rename temporary bitmap file to '%s'", filename);
545 +
546 + strbuf_release(&tmp_file);
547 }
pack-write.c
+6 -6
@@ -71,9 +71,9 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
71 f = sha1fd_check(index_name);
72 } else {
73 if (!index_name) {
74 - static char tmp_file[PATH_MAX];
75 - fd = odb_mkstemp(tmp_file, sizeof(tmp_file), "pack/tmp_idx_XXXXXX");
76 - index_name = xstrdup(tmp_file);
74 + struct strbuf tmp_file = STRBUF_INIT;
75 + fd = odb_mkstemp(&tmp_file, "pack/tmp_idx_XXXXXX");
76 + index_name = strbuf_detach(&tmp_file, NULL);
77 } else {
78 unlink(index_name);
79 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,
329
330 struct sha1file *create_tmp_packfile(char **pack_tmp_name)
331 {
332 - char tmpname[PATH_MAX];
332 + struct strbuf tmpname = STRBUF_INIT;
333 int fd;
334
335 - fd = odb_mkstemp(tmpname, sizeof(tmpname), "pack/tmp_pack_XXXXXX");
336 - *pack_tmp_name = xstrdup(tmpname);
335 + fd = odb_mkstemp(&tmpname, "pack/tmp_pack_XXXXXX");
336 + *pack_tmp_name = strbuf_detach(&tmpname, NULL);
337 return sha1fd(fd, *pack_tmp_name);
338 }
339