From 390c6cbc5e643b6d89869b319b51b5b62a3f5a09 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 18 May 2018 18:56:37 -0700 Subject: http: use strbufs instead of fixed buffers We keep the names of incoming packs and objects in fixed PATH_MAX-size buffers, and snprintf() into them. This is unlikely to end up with truncated filenames, but it is possible (especially on systems where PATH_MAX is shorter than actual paths can be). Let's switch to using strbufs, which makes the question go away entirely. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/http.c b/http.c index a5bd5d6..fc5fff9 100644 --- a/http.c +++ b/http.c @@ -2087,6 +2087,7 @@ void release_http_pack_request(struct http_pack_request *preq) preq->packfile = NULL; } preq->slot = NULL; + strbuf_release(&preq->tmpfile); free(preq->url); free(preq); } @@ -2109,19 +2110,19 @@ int finish_http_pack_request(struct http_pack_request *preq) lst = &((*lst)->next); *lst = (*lst)->next; - if (!strip_suffix(preq->tmpfile, ".pack.temp", &len)) + if (!strip_suffix(preq->tmpfile.buf, ".pack.temp", &len)) die("BUG: pack tmpfile does not end in .pack.temp?"); - tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile); + tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile.buf); argv_array_push(&ip.args, "index-pack"); argv_array_pushl(&ip.args, "-o", tmp_idx, NULL); - argv_array_push(&ip.args, preq->tmpfile); + argv_array_push(&ip.args, preq->tmpfile.buf); ip.git_cmd = 1; ip.no_stdin = 1; ip.no_stdout = 1; if (run_command(&ip)) { - unlink(preq->tmpfile); + unlink(preq->tmpfile.buf); unlink(tmp_idx); free(tmp_idx); return -1; @@ -2129,7 +2130,7 @@ int finish_http_pack_request(struct http_pack_request *preq) unlink(sha1_pack_index_name(p->sha1)); - if (finalize_object_file(preq->tmpfile, sha1_pack_name(p->sha1)) + if (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->sha1)) || finalize_object_file(tmp_idx, sha1_pack_index_name(p->sha1))) { free(tmp_idx); return -1; @@ -2148,6 +2149,7 @@ struct http_pack_request *new_http_pack_request( struct http_pack_request *preq; preq = xcalloc(1, sizeof(*preq)); + strbuf_init(&preq->tmpfile, 0); preq->target = target; end_url_with_slash(&buf, base_url); @@ -2155,12 +2157,11 @@ struct http_pack_request *new_http_pack_request( sha1_to_hex(target->sha1)); preq->url = strbuf_detach(&buf, NULL); - snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp", - sha1_pack_name(target->sha1)); - preq->packfile = fopen(preq->tmpfile, "a"); + strbuf_addf(&preq->tmpfile, "%s.temp", sha1_pack_name(target->sha1)); + preq->packfile = fopen(preq->tmpfile.buf, "a"); if (!preq->packfile) { error("Unable to open local file %s for pack", - preq->tmpfile); + preq->tmpfile.buf); goto abort; } @@ -2187,6 +2188,7 @@ struct http_pack_request *new_http_pack_request( return preq; abort: + strbuf_release(&preq->tmpfile); free(preq->url); free(preq); return NULL; @@ -2237,7 +2239,7 @@ struct http_object_request *new_http_object_request(const char *base_url, { char *hex = sha1_to_hex(sha1); struct strbuf filename = STRBUF_INIT; - char prevfile[PATH_MAX]; + struct strbuf prevfile = STRBUF_INIT; int prevlocal; char prev_buf[PREV_BUF_SIZE]; ssize_t prev_read = 0; @@ -2245,40 +2247,41 @@ struct http_object_request *new_http_object_request(const char *base_url, struct http_object_request *freq; freq = xcalloc(1, sizeof(*freq)); + strbuf_init(&freq->tmpfile, 0); hashcpy(freq->sha1, sha1); freq->localfile = -1; sha1_file_name(&filename, sha1); - snprintf(freq->tmpfile, sizeof(freq->tmpfile), - "%s.temp", filename.buf); + strbuf_addf(&freq->tmpfile, "%s.temp", filename.buf); - snprintf(prevfile, sizeof(prevfile), "%s.prev", filename.buf); - unlink_or_warn(prevfile); - rename(freq->tmpfile, prevfile); - unlink_or_warn(freq->tmpfile); + strbuf_addf(&prevfile, "%s.prev", filename.buf); + unlink_or_warn(prevfile.buf); + rename(freq->tmpfile.buf, prevfile.buf); + unlink_or_warn(freq->tmpfile.buf); strbuf_release(&filename); if (freq->localfile != -1) error("fd leakage in start: %d", freq->localfile); - freq->localfile = open(freq->tmpfile, + freq->localfile = open(freq->tmpfile.buf, O_WRONLY | O_CREAT | O_EXCL, 0666); /* * This could have failed due to the "lazy directory creation"; * try to mkdir the last path component. */ if (freq->localfile < 0 && errno == ENOENT) { - char *dir = strrchr(freq->tmpfile, '/'); + char *dir = strrchr(freq->tmpfile.buf, '/'); if (dir) { *dir = 0; - mkdir(freq->tmpfile, 0777); + mkdir(freq->tmpfile.buf, 0777); *dir = '/'; } - freq->localfile = open(freq->tmpfile, + freq->localfile = open(freq->tmpfile.buf, O_WRONLY | O_CREAT | O_EXCL, 0666); } if (freq->localfile < 0) { - error_errno("Couldn't create temporary file %s", freq->tmpfile); + error_errno("Couldn't create temporary file %s", + freq->tmpfile.buf); goto abort; } @@ -2292,7 +2295,7 @@ struct http_object_request *new_http_object_request(const char *base_url, * If a previous temp file is present, process what was already * fetched. */ - prevlocal = open(prevfile, O_RDONLY); + prevlocal = open(prevfile.buf, O_RDONLY); if (prevlocal != -1) { do { prev_read = xread(prevlocal, prev_buf, PREV_BUF_SIZE); @@ -2309,7 +2312,8 @@ struct http_object_request *new_http_object_request(const char *base_url, } while (prev_read > 0); close(prevlocal); } - unlink_or_warn(prevfile); + unlink_or_warn(prevfile.buf); + strbuf_release(&prevfile); /* * Reset inflate/SHA1 if there was an error reading the previous temp @@ -2324,7 +2328,7 @@ struct http_object_request *new_http_object_request(const char *base_url, lseek(freq->localfile, 0, SEEK_SET); if (ftruncate(freq->localfile, 0) < 0) { error_errno("Couldn't truncate temporary file %s", - freq->tmpfile); + freq->tmpfile.buf); goto abort; } } @@ -2354,6 +2358,7 @@ struct http_object_request *new_http_object_request(const char *base_url, return freq; abort: + strbuf_release(&prevfile); free(freq->url); free(freq); return NULL; @@ -2381,25 +2386,25 @@ int finish_http_object_request(struct http_object_request *freq) if (freq->http_code == 416) { warning("requested range invalid; we may already have all the data."); } else if (freq->curl_result != CURLE_OK) { - if (stat(freq->tmpfile, &st) == 0) + if (stat(freq->tmpfile.buf, &st) == 0) if (st.st_size == 0) - unlink_or_warn(freq->tmpfile); + unlink_or_warn(freq->tmpfile.buf); return -1; } git_inflate_end(&freq->stream); git_SHA1_Final(freq->real_sha1, &freq->c); if (freq->zret != Z_STREAM_END) { - unlink_or_warn(freq->tmpfile); + unlink_or_warn(freq->tmpfile.buf); return -1; } if (hashcmp(freq->sha1, freq->real_sha1)) { - unlink_or_warn(freq->tmpfile); + unlink_or_warn(freq->tmpfile.buf); return -1; } sha1_file_name(&filename, freq->sha1); - freq->rename = finalize_object_file(freq->tmpfile, filename.buf); + freq->rename = finalize_object_file(freq->tmpfile.buf, filename.buf); strbuf_release(&filename); return freq->rename; @@ -2407,7 +2412,7 @@ int finish_http_object_request(struct http_object_request *freq) void abort_http_object_request(struct http_object_request *freq) { - unlink_or_warn(freq->tmpfile); + unlink_or_warn(freq->tmpfile.buf); release_http_object_request(freq); } @@ -2427,4 +2432,5 @@ void release_http_object_request(struct http_object_request *freq) release_active_slot(freq->slot); freq->slot = NULL; } + strbuf_release(&freq->tmpfile); } diff --git a/http.h b/http.h index f7bd3b2..d9379f9 100644 --- a/http.h +++ b/http.h @@ -200,7 +200,7 @@ struct http_pack_request { struct packed_git *target; struct packed_git **lst; FILE *packfile; - char tmpfile[PATH_MAX]; + struct strbuf tmpfile; struct active_request_slot *slot; }; @@ -212,7 +212,7 @@ extern void release_http_pack_request(struct http_pack_request *preq); /* Helpers for fetching object */ struct http_object_request { char *url; - char tmpfile[PATH_MAX]; + struct strbuf tmpfile; int localfile; CURLcode curl_result; char errorstr[CURL_ERROR_SIZE]; -- cgit v0.10.2-6-g49f6 From d50b69b868d1b6a7f36e94382c74d2e8cda2d64a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 18 May 2018 18:57:44 -0700 Subject: log_write_email_headers: use strbufs When we write a MIME attachment, we write the mime headers into fixed-size buffers. These are likely to be big enough in practice, but technically the input could be arbitrarily large (e.g., if the caller provided a lot of content in the extra_headers string), in which case we'd quietly truncate it and generate bogus output. Let's convert these buffers to strbufs. The memory ownership here is a bit funny. The original fixed buffers were static, and we merely pass out pointers to them to be used by the caller (and in one case, we even just stuff our value into the opt->diffopt.stat_sep value). Ideally we'd actually pass back heap buffers, and the caller would be responsible for freeing them. This patch punts on that cleanup for now, and instead just marks the strbufs as static. That means we keep ownership in this function, making it not a complete leak. This also takes us one step closer to fixing it in the long term (since we can eventually use strbuf_detach() to hand ownership to the caller, once it's ready). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/log-tree.c b/log-tree.c index bdf23c5..4e83d71 100644 --- a/log-tree.c +++ b/log-tree.c @@ -386,11 +386,15 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit, graph_show_oneline(opt->graph); } if (opt->mime_boundary) { - static char subject_buffer[1024]; - static char buffer[1024]; + static struct strbuf subject_buffer = STRBUF_INIT; + static struct strbuf buffer = STRBUF_INIT; struct strbuf filename = STRBUF_INIT; *need_8bit_cte_p = -1; /* NEVER */ - snprintf(subject_buffer, sizeof(subject_buffer) - 1, + + strbuf_reset(&subject_buffer); + strbuf_reset(&buffer); + + strbuf_addf(&subject_buffer, "%s" "MIME-Version: 1.0\n" "Content-Type: multipart/mixed;" @@ -405,13 +409,13 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit, extra_headers ? extra_headers : "", mime_boundary_leader, opt->mime_boundary, mime_boundary_leader, opt->mime_boundary); - extra_headers = subject_buffer; + extra_headers = subject_buffer.buf; if (opt->numbered_files) strbuf_addf(&filename, "%d", opt->nr); else fmt_output_commit(&filename, commit, opt); - snprintf(buffer, sizeof(buffer) - 1, + strbuf_addf(&buffer, "\n--%s%s\n" "Content-Type: text/x-patch;" " name=\"%s\"\n" @@ -422,7 +426,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit, filename.buf, opt->no_inline ? "attachment" : "inline", filename.buf); - opt->diffopt.stat_sep = buffer; + opt->diffopt.stat_sep = buffer.buf; strbuf_release(&filename); } *extra_headers_p = extra_headers; -- cgit v0.10.2-6-g49f6 From 735e4173b3feba0cc495bbb2dd85341b6378f628 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sat, 19 May 2018 10:27:46 +0200 Subject: fsmonitor: use internal argv_array of struct child_process Avoid magic array sizes and indexes by constructing the fsmonitor command line using the embedded argv_array of the child_process. The resulting code is shorter and easier to extend. Getting rid of the snprintf() calls is a bonus -- even though the buffers were big enough here to avoid truncation -- as it makes auditing the remaining callers easier. Inspired-by: Jeff King Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano diff --git a/fsmonitor.c b/fsmonitor.c index 6d7bcd5..b6fcb0f 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -97,19 +97,13 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate) static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result) { struct child_process cp = CHILD_PROCESS_INIT; - char ver[64]; - char date[64]; - const char *argv[4]; - if (!(argv[0] = core_fsmonitor)) + if (!core_fsmonitor) return -1; - snprintf(ver, sizeof(version), "%d", version); - snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update); - argv[1] = ver; - argv[2] = date; - argv[3] = NULL; - cp.argv = argv; + argv_array_push(&cp.args, core_fsmonitor); + argv_array_pushf(&cp.args, "%d", version); + argv_array_pushf(&cp.args, "%" PRIuMAX, (uintmax_t)last_update); cp.use_shell = 1; cp.dir = get_git_work_tree(); -- cgit v0.10.2-6-g49f6 From bf4baf1fed7916ed10f2759a6f30a38990a83cae Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 18 May 2018 18:58:20 -0700 Subject: shorten_unambiguous_ref: use xsnprintf We convert the ref_rev_parse_rules array into scanf formats on the fly, and use snprintf() to write into each string. We should have enough memory to hold everything because of the earlier total_len computation. Let's use xsnprintf() to give runtime confirmation that this is the case, and to make it easy for people auditing the code to know there's no truncation bug. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/refs.c b/refs.c index 20ba82b..bd7ac72 100644 --- a/refs.c +++ b/refs.c @@ -1132,8 +1132,8 @@ char *shorten_unambiguous_ref(const char *refname, int strict) for (i = 0; i < nr_rules; i++) { assert(offset < total_len); scanf_fmts[i] = (char *)&scanf_fmts[nr_rules] + offset; - offset += snprintf(scanf_fmts[i], total_len - offset, - ref_rev_parse_rules[i], 2, "%s") + 1; + offset += xsnprintf(scanf_fmts[i], total_len - offset, + ref_rev_parse_rules[i], 2, "%s") + 1; } } -- cgit v0.10.2-6-g49f6 From ac4896f007a624c12feda866aeb4abe8a1394e39 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 18 May 2018 18:58:44 -0700 Subject: fmt_with_err: add a comment that truncation is OK Functions like die_errno() use fmt_with_err() to combine the caller-provided format with the strerror() string. We use a fixed stack buffer because we're already handling an error and don't have any way to report another one. Our buffer should generally be big enough to fit this, but if it's not, truncation is our best option. Let's add a comment to that effect, so that anybody auditing the code for truncation bugs knows that this is fine. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/usage.c b/usage.c index cdd534c..b3c7893 100644 --- a/usage.c +++ b/usage.c @@ -148,6 +148,7 @@ static const char *fmt_with_err(char *buf, int n, const char *fmt) } } str_error[j] = 0; + /* Truncation is acceptable here */ snprintf(buf, n, "%s: %s", fmt, str_error); return buf; } -- cgit v0.10.2-6-g49f6