* [PATCH 0/5] snprintf truncation fixes @ 2018-05-19 1:54 Jeff King 2018-05-19 1:56 ` [PATCH 1/5] http: use strbufs instead of fixed buffers Jeff King ` (4 more replies) 0 siblings, 5 replies; 13+ messages in thread From: Jeff King @ 2018-05-19 1:54 UTC (permalink / raw) To: git I happened today to be looking at a piece of code that used a bare snprintf() without checking for truncation, and I got annoyed enough to root out the last few cases in our codebase. After this series, looking over the results of: git grep '[^vxn]snprintf' '*.c' :^compat is pretty pleasant. This series also gets rid of some uses of PATH_MAX, which is another pet peeve of mine. :) [1/5]: http: use strbufs instead of fixed buffers [2/5]: log_write_email_headers: use strbufs [3/5]: query_fsmonitor: use xsnprintf for formatting integers [4/5]: shorten_unambiguous_ref: use xsnprintf [5/5]: fmt_with_err: add a comment that truncation is OK fsmonitor.c | 4 ++-- http.c | 66 +++++++++++++++++++++++++++++------------------------ http.h | 4 ++-- log-tree.c | 16 ++++++++----- refs.c | 4 ++-- usage.c | 1 + 6 files changed, 53 insertions(+), 42 deletions(-) -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/5] http: use strbufs instead of fixed buffers 2018-05-19 1:54 [PATCH 0/5] snprintf truncation fixes Jeff King @ 2018-05-19 1:56 ` Jeff King 2018-05-21 18:11 ` Stefan Beller 2018-05-19 1:57 ` [PATCH 2/5] log_write_email_headers: use strbufs Jeff King ` (3 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2018-05-19 1:56 UTC (permalink / raw) To: git 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 <peff@peff.net> --- The diff is a little noisy due to the s/tmpfile/&.buf/ everywhere. The interesting lines to look at are initialization and release, which I think I got right. http.c | 66 ++++++++++++++++++++++++++++++++-------------------------- http.h | 4 ++-- 2 files changed, 38 insertions(+), 32 deletions(-) diff --git a/http.c b/http.c index fed13b2169..260bd10f95 100644 --- a/http.c +++ b/http.c @@ -2082,6 +2082,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); } @@ -2104,19 +2105,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; @@ -2124,7 +2125,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; @@ -2143,6 +2144,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); @@ -2150,12 +2152,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; } @@ -2182,6 +2183,7 @@ struct http_pack_request *new_http_pack_request( return preq; abort: + strbuf_release(&preq->tmpfile); free(preq->url); free(preq); return NULL; @@ -2232,7 +2234,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; @@ -2240,40 +2242,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(the_repository, &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; } @@ -2287,7 +2290,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); @@ -2304,7 +2307,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 @@ -2319,7 +2323,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; } } @@ -2349,6 +2353,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; @@ -2376,24 +2381,24 @@ 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(the_repository, &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; @@ -2401,7 +2406,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); } @@ -2421,4 +2426,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 4df4a25e1a..d305ca1dc7 100644 --- a/http.h +++ b/http.h @@ -207,7 +207,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; }; @@ -219,7 +219,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]; -- 2.17.0.1052.g7d69f75dbf ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] http: use strbufs instead of fixed buffers 2018-05-19 1:56 ` [PATCH 1/5] http: use strbufs instead of fixed buffers Jeff King @ 2018-05-21 18:11 ` Stefan Beller 2018-05-21 19:41 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Stefan Beller @ 2018-05-21 18:11 UTC (permalink / raw) To: Jeff King; +Cc: git Hi Jeff, On Fri, May 18, 2018 at 6:56 PM, Jeff King <peff@peff.net> wrote: > @@ -2421,4 +2426,5 @@ void release_http_object_request(struct http_object_request *freq) .. > + strbuf_release(&freq->tmpfile); Do we need an equivalent in release_http_pack_request as well? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] http: use strbufs instead of fixed buffers 2018-05-21 18:11 ` Stefan Beller @ 2018-05-21 19:41 ` Jeff King 2018-05-21 20:57 ` Stefan Beller 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2018-05-21 19:41 UTC (permalink / raw) To: Stefan Beller; +Cc: git On Mon, May 21, 2018 at 11:11:51AM -0700, Stefan Beller wrote: > Hi Jeff, > > On Fri, May 18, 2018 at 6:56 PM, Jeff King <peff@peff.net> wrote: > > > @@ -2421,4 +2426,5 @@ void release_http_object_request(struct http_object_request *freq) > .. > > + strbuf_release(&freq->tmpfile); > > Do we need an equivalent in release_http_pack_request as well? Yes, but isn't there one? From the original patch: --- a/http.c +++ b/http.c @@ -2082,6 +2082,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); } Or am I missing something? -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] http: use strbufs instead of fixed buffers 2018-05-21 19:41 ` Jeff King @ 2018-05-21 20:57 ` Stefan Beller 0 siblings, 0 replies; 13+ messages in thread From: Stefan Beller @ 2018-05-21 20:57 UTC (permalink / raw) To: Jeff King; +Cc: git On Mon, May 21, 2018 at 12:41 PM, Jeff King <peff@peff.net> wrote: > On Mon, May 21, 2018 at 11:11:51AM -0700, Stefan Beller wrote: > >> Hi Jeff, >> >> On Fri, May 18, 2018 at 6:56 PM, Jeff King <peff@peff.net> wrote: >> >> > @@ -2421,4 +2426,5 @@ void release_http_object_request(struct http_object_request *freq) >> .. >> > + strbuf_release(&freq->tmpfile); >> >> Do we need an equivalent in release_http_pack_request as well? > > Yes, but isn't there one? > > From the original patch: > > --- a/http.c > +++ b/http.c > @@ -2082,6 +2082,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); > } which is at the very top, but I was not looking this far up, but looked for functions order that my editor had. Clearly I have to practice code reviews, still. Sorry for the noise. Stefan ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/5] log_write_email_headers: use strbufs 2018-05-19 1:54 [PATCH 0/5] snprintf truncation fixes Jeff King 2018-05-19 1:56 ` [PATCH 1/5] http: use strbufs instead of fixed buffers Jeff King @ 2018-05-19 1:57 ` Jeff King 2018-05-19 1:57 ` [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers Jeff King ` (2 subsequent siblings) 4 siblings, 0 replies; 13+ messages in thread From: Jeff King @ 2018-05-19 1:57 UTC (permalink / raw) To: git 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 <peff@peff.net> --- The rest of that cleanup is a possible #leftoverbits. log-tree.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/log-tree.c b/log-tree.c index d1c0bedf24..1173fdb057 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; -- 2.17.0.1052.g7d69f75dbf ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers 2018-05-19 1:54 [PATCH 0/5] snprintf truncation fixes Jeff King 2018-05-19 1:56 ` [PATCH 1/5] http: use strbufs instead of fixed buffers Jeff King 2018-05-19 1:57 ` [PATCH 2/5] log_write_email_headers: use strbufs Jeff King @ 2018-05-19 1:57 ` Jeff King 2018-05-19 8:27 ` René Scharfe 2018-05-19 1:58 ` [PATCH 4/5] shorten_unambiguous_ref: use xsnprintf Jeff King 2018-05-19 1:58 ` [PATCH 5/5] fmt_with_err: add a comment that truncation is OK Jeff King 4 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2018-05-19 1:57 UTC (permalink / raw) To: git These formatted integers should always fit into their 64-byte buffers. Let's use xsnprintf() to add an assertion that this is the case, which makes auditing for other unchecked snprintfs() easier. Signed-off-by: Jeff King <peff@peff.net> --- fsmonitor.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fsmonitor.c b/fsmonitor.c index ed3d1a074d..cc19b27e1d 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -104,8 +104,8 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que if (!(argv[0] = core_fsmonitor)) return -1; - snprintf(ver, sizeof(ver), "%d", version); - snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update); + xsnprintf(ver, sizeof(ver), "%d", version); + xsnprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update); argv[1] = ver; argv[2] = date; argv[3] = NULL; -- 2.17.0.1052.g7d69f75dbf ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers 2018-05-19 1:57 ` [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers Jeff King @ 2018-05-19 8:27 ` René Scharfe 2018-05-20 17:08 ` Jeff King ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: René Scharfe @ 2018-05-19 8:27 UTC (permalink / raw) To: Jeff King, git; +Cc: Ben Peart Am 19.05.2018 um 03:57 schrieb Jeff King: > These formatted integers should always fit into their > 64-byte buffers. Let's use xsnprintf() to add an assertion > that this is the case, which makes auditing for other > unchecked snprintfs() easier. How about this instead? -- >8 -- Subject: [PATCH] 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 <peff@peff.net> Signed-off-by: Rene Scharfe <l.s.r@web.de> --- fsmonitor.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/fsmonitor.c b/fsmonitor.c index ed3d1a074d..665bd2d425 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(ver), "%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(); -- 2.17.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers 2018-05-19 8:27 ` René Scharfe @ 2018-05-20 17:08 ` Jeff King 2018-05-21 0:58 ` Junio C Hamano 2018-05-21 12:36 ` Ben Peart 2 siblings, 0 replies; 13+ messages in thread From: Jeff King @ 2018-05-20 17:08 UTC (permalink / raw) To: René Scharfe; +Cc: git, Ben Peart On Sat, May 19, 2018 at 10:27:46AM +0200, René Scharfe wrote: > Am 19.05.2018 um 03:57 schrieb Jeff King: > > These formatted integers should always fit into their > > 64-byte buffers. Let's use xsnprintf() to add an assertion > > that this is the case, which makes auditing for other > > unchecked snprintfs() easier. > > How about this instead? > > -- >8 -- > Subject: [PATCH] fsmonitor: use internal argv_array of struct child_process Yeah, I agree that is much nicer (I was so focused on the snprintfs I didn't bother to look at the context for a better solution). Thanks, getting a review in the form of a complete patch is my favorite kind of review. :) > Inspired-by: Jeff King <peff@peff.net> Heh. -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers 2018-05-19 8:27 ` René Scharfe 2018-05-20 17:08 ` Jeff King @ 2018-05-21 0:58 ` Junio C Hamano 2018-05-21 12:36 ` Ben Peart 2 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2018-05-21 0:58 UTC (permalink / raw) To: René Scharfe; +Cc: Jeff King, git, Ben Peart René Scharfe <l.s.r@web.de> writes: > How about this instead? > > -- >8 -- > Subject: [PATCH] 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. > ... > - if (!(argv[0] = core_fsmonitor)) > + if (!core_fsmonitor) > return -1; > > - snprintf(ver, sizeof(ver), "%d", version); I really like this change, as this exact line used to take sizeof(version) instead of sizeof(ver), which has been corrected recently. > - 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); If it were written like this from the beginning, such a bug would never have happened ;-) > + argv_array_pushf(&cp.args, "%" PRIuMAX, (uintmax_t)last_update); > cp.use_shell = 1; > cp.dir = get_git_work_tree(); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers 2018-05-19 8:27 ` René Scharfe 2018-05-20 17:08 ` Jeff King 2018-05-21 0:58 ` Junio C Hamano @ 2018-05-21 12:36 ` Ben Peart 2 siblings, 0 replies; 13+ messages in thread From: Ben Peart @ 2018-05-21 12:36 UTC (permalink / raw) To: René Scharfe, Jeff King, git; +Cc: Ben Peart On 5/19/2018 4:27 AM, René Scharfe wrote: > Am 19.05.2018 um 03:57 schrieb Jeff King: >> These formatted integers should always fit into their >> 64-byte buffers. Let's use xsnprintf() to add an assertion >> that this is the case, which makes auditing for other >> unchecked snprintfs() easier. > > How about this instead? > > -- >8 -- > > - snprintf(ver, sizeof(ver), "%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); Looks good. Simpler, cleaner, less error prone. > cp.use_shell = 1; > cp.dir = get_git_work_tree(); > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/5] shorten_unambiguous_ref: use xsnprintf 2018-05-19 1:54 [PATCH 0/5] snprintf truncation fixes Jeff King ` (2 preceding siblings ...) 2018-05-19 1:57 ` [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers Jeff King @ 2018-05-19 1:58 ` Jeff King 2018-05-19 1:58 ` [PATCH 5/5] fmt_with_err: add a comment that truncation is OK Jeff King 4 siblings, 0 replies; 13+ messages in thread From: Jeff King @ 2018-05-19 1:58 UTC (permalink / raw) To: git 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 <peff@peff.net> --- refs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index 64aadd14c9..73c446628f 100644 --- a/refs.c +++ b/refs.c @@ -1147,8 +1147,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; } } -- 2.17.0.1052.g7d69f75dbf ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 5/5] fmt_with_err: add a comment that truncation is OK 2018-05-19 1:54 [PATCH 0/5] snprintf truncation fixes Jeff King ` (3 preceding siblings ...) 2018-05-19 1:58 ` [PATCH 4/5] shorten_unambiguous_ref: use xsnprintf Jeff King @ 2018-05-19 1:58 ` Jeff King 4 siblings, 0 replies; 13+ messages in thread From: Jeff King @ 2018-05-19 1:58 UTC (permalink / raw) To: git 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 <peff@peff.net> --- usage.c | 1 + 1 file changed, 1 insertion(+) diff --git a/usage.c b/usage.c index cdd534c9df..b3c78931ad 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; } -- 2.17.0.1052.g7d69f75dbf ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-05-21 20:57 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-19 1:54 [PATCH 0/5] snprintf truncation fixes Jeff King 2018-05-19 1:56 ` [PATCH 1/5] http: use strbufs instead of fixed buffers Jeff King 2018-05-21 18:11 ` Stefan Beller 2018-05-21 19:41 ` Jeff King 2018-05-21 20:57 ` Stefan Beller 2018-05-19 1:57 ` [PATCH 2/5] log_write_email_headers: use strbufs Jeff King 2018-05-19 1:57 ` [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers Jeff King 2018-05-19 8:27 ` René Scharfe 2018-05-20 17:08 ` Jeff King 2018-05-21 0:58 ` Junio C Hamano 2018-05-21 12:36 ` Ben Peart 2018-05-19 1:58 ` [PATCH 4/5] shorten_unambiguous_ref: use xsnprintf Jeff King 2018-05-19 1:58 ` [PATCH 5/5] fmt_with_err: add a comment that truncation is OK Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).