From 892e723afd2b5696e4d75280e730bf9f1ea92329 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 28 Mar 2017 15:45:25 -0400 Subject: do not check odb_mkstemp return value for errors The odb_mkstemp function does not return an error; it dies on failure instead. But many of its callers compare the resulting descriptor against -1 and die themselves. Mostly this is just pointless, but it does raise a question when looking at the callers: if they show the results of the "template" buffer after a failure, what's in it? The answer is: it doesn't matter, because it cannot happen. So let's make that clear by removing the bogus error checks. In bitmap_writer_finish(), we can drop the error-handling code entirely. In the other two cases, it's shared with the open() in another code path; we can just move the error-check next to that open() call. And while we're at it, let's flesh out the function's docstring a bit to make the error behavior clear. Signed-off-by: Jeff King --- builtin/index-pack.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'builtin/index-pack.c') diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 88d205f858..49e7175d9a 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -311,10 +311,11 @@ static const char *open_pack_file(const char *pack_name) output_fd = odb_mkstemp(tmp_file, sizeof(tmp_file), "pack/tmp_pack_XXXXXX"); pack_name = xstrdup(tmp_file); - } else + } else { output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600); - if (output_fd < 0) - die_errno(_("unable to create '%s'"), pack_name); + if (output_fd < 0) + die_errno(_("unable to create '%s'"), pack_name); + } nothread_data.pack_fd = output_fd; } else { input_fd = open(pack_name, O_RDONLY); -- cgit v1.2.3 From 594fa9998c41277c579a94657100fa303160aa7e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 28 Mar 2017 15:45:43 -0400 Subject: 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 --- builtin/index-pack.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'builtin/index-pack.c') diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 49e7175d9a..f4af2ab37a 100644 --- 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) -- cgit v1.2.3 From 5b1ef2cef4ff9d3213ec81465b99affb4a7c8083 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 28 Mar 2017 15:46:50 -0400 Subject: replace unchecked snprintf calls with heap buffers We'd prefer to avoid unchecked snprintf calls because truncation can lead to unexpected results. These are all cases where truncation shouldn't ever happen, because the input to snprintf is fixed in size. That makes them candidates for xsnprintf(), but it's simpler still to just use the heap, and then nobody has to wonder if "100" is big enough. We'll use xstrfmt() where possible, and a strbuf when we need the resulting size or to reuse the same buffer in a loop. Signed-off-by: Jeff King --- builtin/index-pack.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'builtin/index-pack.c') diff --git a/builtin/index-pack.c b/builtin/index-pack.c index f4af2ab37a..197c51912d 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1443,10 +1443,11 @@ static void final(const char *final_pack_name, const char *curr_pack_name, if (!from_stdin) { printf("%s\n", sha1_to_hex(sha1)); } else { - char buf[48]; - int len = snprintf(buf, sizeof(buf), "%s\t%s\n", - report, sha1_to_hex(sha1)); - write_or_die(1, buf, len); + struct strbuf buf = STRBUF_INIT; + + strbuf_addf(&buf, "%s\t%s\n", report, sha1_to_hex(sha1)); + write_or_die(1, buf.buf, buf.len); + strbuf_release(&buf); /* * Let's just mimic git-unpack-objects here and write -- cgit v1.2.3