{"thread":{"id":"48519","subject":"[PATCH 0/5] snprintf truncation fixes","startedAt":"2018-05-19T01:54:51Z","lastAt":"2018-05-21T20:57:30Z","messageCount":13,"participants":["Jeff King","René Scharfe","Junio C Hamano","Ben Peart","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"348018","messageId":"20180519015444.GA12080@sigill.intra.peff.net","threadId":"48519","inReplyTo":null,"subject":"[PATCH 0/5] snprintf truncation fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-19T01:54:45Z","receivedAt":"2018-05-19T01:54:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I happened today to be looking at a piece of code that used a bare\nsnprintf() without checking for truncation, and I got annoyed enough to\nroot out the last few cases in our codebase. After this series, looking\nover the results of:\n\n  git grep '[^vxn]snprintf' '*.c' :^compat\n\nis pretty pleasant.\n\nThis series also gets rid of some uses of PATH_MAX, which is another pet\npeeve of mine. :)\n\n  [1/5]: http: use strbufs instead of fixed buffers\n  [2/5]: log_write_email_headers: use strbufs\n  [3/5]: query_fsmonitor: use xsnprintf for formatting integers\n  [4/5]: shorten_unambiguous_ref: use xsnprintf\n  [5/5]: fmt_with_err: add a comment that truncation is OK\n\n fsmonitor.c |  4 ++--\n http.c      | 66 +++++++++++++++++++++++++++++------------------------\n http.h      |  4 ++--\n log-tree.c  | 16 ++++++++-----\n refs.c      |  4 ++--\n usage.c     |  1 +\n 6 files changed, 53 insertions(+), 42 deletions(-)\n\n-Peff\n"},{"id":"348019","messageId":"20180519015636.GA32492@sigill.intra.peff.net","threadId":"48519","inReplyTo":"20180519015444.GA12080@sigill.intra.peff.net","subject":"[PATCH 1/5] http: use strbufs instead of fixed buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-19T01:56:37Z","receivedAt":"2018-05-19T01:56:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep the names of incoming packs and objects in fixed\nPATH_MAX-size buffers, and snprintf() into them. This is\nunlikely to end up with truncated filenames, but it is\npossible (especially on systems where PATH_MAX is shorter\nthan actual paths can be). Let's switch to using strbufs,\nwhich makes the question go away entirely.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe diff is a little noisy due to the s/tmpfile/&.buf/ everywhere. The\ninteresting lines to look at are initialization and release, which I\nthink I got right.\n\n http.c | 66 ++++++++++++++++++++++++++++++++--------------------------\n http.h |  4 ++--\n 2 files changed, 38 insertions(+), 32 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex fed13b2169..260bd10f95 100644\n--- a/http.c\n+++ b/http.c\n@@ -2082,6 +2082,7 @@ void release_http_pack_request(struct http_pack_request *preq)\n \t\tpreq->packfile = NULL;\n \t}\n \tpreq->slot = NULL;\n+\tstrbuf_release(&preq->tmpfile);\n \tfree(preq->url);\n \tfree(preq);\n }\n@@ -2104,19 +2105,19 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \t\tlst = &((*lst)->next);\n \t*lst = (*lst)->next;\n \n-\tif (!strip_suffix(preq->tmpfile, \".pack.temp\", &len))\n+\tif (!strip_suffix(preq->tmpfile.buf, \".pack.temp\", &len))\n \t\tdie(\"BUG: pack tmpfile does not end in .pack.temp?\");\n-\ttmp_idx = xstrfmt(\"%.*s.idx.temp\", (int)len, preq->tmpfile);\n+\ttmp_idx = xstrfmt(\"%.*s.idx.temp\", (int)len, preq->tmpfile.buf);\n \n \targv_array_push(&ip.args, \"index-pack\");\n \targv_array_pushl(&ip.args, \"-o\", tmp_idx, NULL);\n-\targv_array_push(&ip.args, preq->tmpfile);\n+\targv_array_push(&ip.args, preq->tmpfile.buf);\n \tip.git_cmd = 1;\n \tip.no_stdin = 1;\n \tip.no_stdout = 1;\n \n \tif (run_command(&ip)) {\n-\t\tunlink(preq->tmpfile);\n+\t\tunlink(preq->tmpfile.buf);\n \t\tunlink(tmp_idx);\n \t\tfree(tmp_idx);\n \t\treturn -1;\n@@ -2124,7 +2125,7 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \n \tunlink(sha1_pack_index_name(p->sha1));\n \n-\tif (finalize_object_file(preq->tmpfile, sha1_pack_name(p->sha1))\n+\tif (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->sha1))\n \t || finalize_object_file(tmp_idx, sha1_pack_index_name(p->sha1))) {\n \t\tfree(tmp_idx);\n \t\treturn -1;\n@@ -2143,6 +2144,7 @@ struct http_pack_request *new_http_pack_request(\n \tstruct http_pack_request *preq;\n \n \tpreq = xcalloc(1, sizeof(*preq));\n+\tstrbuf_init(&preq->tmpfile, 0);\n \tpreq->target = target;\n \n \tend_url_with_slash(&buf, base_url);\n@@ -2150,12 +2152,11 @@ struct http_pack_request *new_http_pack_request(\n \t\tsha1_to_hex(target->sha1));\n \tpreq->url = strbuf_detach(&buf, NULL);\n \n-\tsnprintf(preq->tmpfile, sizeof(preq->tmpfile), \"%s.temp\",\n-\t\tsha1_pack_name(target->sha1));\n-\tpreq->packfile = fopen(preq->tmpfile, \"a\");\n+\tstrbuf_addf(&preq->tmpfile, \"%s.temp\", sha1_pack_name(target->sha1));\n+\tpreq->packfile = fopen(preq->tmpfile.buf, \"a\");\n \tif (!preq->packfile) {\n \t\terror(\"Unable to open local file %s for pack\",\n-\t\t      preq->tmpfile);\n+\t\t      preq->tmpfile.buf);\n \t\tgoto abort;\n \t}\n \n@@ -2182,6 +2183,7 @@ struct http_pack_request *new_http_pack_request(\n \treturn preq;\n \n abort:\n+\tstrbuf_release(&preq->tmpfile);\n \tfree(preq->url);\n \tfree(preq);\n \treturn NULL;\n@@ -2232,7 +2234,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n {\n \tchar *hex = sha1_to_hex(sha1);\n \tstruct strbuf filename = STRBUF_INIT;\n-\tchar prevfile[PATH_MAX];\n+\tstruct strbuf prevfile = STRBUF_INIT;\n \tint prevlocal;\n \tchar prev_buf[PREV_BUF_SIZE];\n \tssize_t prev_read = 0;\n@@ -2240,40 +2242,41 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tstruct http_object_request *freq;\n \n \tfreq = xcalloc(1, sizeof(*freq));\n+\tstrbuf_init(&freq->tmpfile, 0);\n \thashcpy(freq->sha1, sha1);\n \tfreq->localfile = -1;\n \n \tsha1_file_name(the_repository, &filename, sha1);\n-\tsnprintf(freq->tmpfile, sizeof(freq->tmpfile),\n-\t\t \"%s.temp\", filename.buf);\n+\tstrbuf_addf(&freq->tmpfile, \"%s.temp\", filename.buf);\n \n-\tsnprintf(prevfile, sizeof(prevfile), \"%s.prev\", filename.buf);\n-\tunlink_or_warn(prevfile);\n-\trename(freq->tmpfile, prevfile);\n-\tunlink_or_warn(freq->tmpfile);\n+\tstrbuf_addf(&prevfile, \"%s.prev\", filename.buf);\n+\tunlink_or_warn(prevfile.buf);\n+\trename(freq->tmpfile.buf, prevfile.buf);\n+\tunlink_or_warn(freq->tmpfile.buf);\n \tstrbuf_release(&filename);\n \n \tif (freq->localfile != -1)\n \t\terror(\"fd leakage in start: %d\", freq->localfile);\n-\tfreq->localfile = open(freq->tmpfile,\n+\tfreq->localfile = open(freq->tmpfile.buf,\n \t\t\t       O_WRONLY | O_CREAT | O_EXCL, 0666);\n \t/*\n \t * This could have failed due to the \"lazy directory creation\";\n \t * try to mkdir the last path component.\n \t */\n \tif (freq->localfile < 0 && errno == ENOENT) {\n-\t\tchar *dir = strrchr(freq->tmpfile, '/');\n+\t\tchar *dir = strrchr(freq->tmpfile.buf, '/');\n \t\tif (dir) {\n \t\t\t*dir = 0;\n-\t\t\tmkdir(freq->tmpfile, 0777);\n+\t\t\tmkdir(freq->tmpfile.buf, 0777);\n \t\t\t*dir = '/';\n \t\t}\n-\t\tfreq->localfile = open(freq->tmpfile,\n+\t\tfreq->localfile = open(freq->tmpfile.buf,\n \t\t\t\t       O_WRONLY | O_CREAT | O_EXCL, 0666);\n \t}\n \n \tif (freq->localfile < 0) {\n-\t\terror_errno(\"Couldn't create temporary file %s\", freq->tmpfile);\n+\t\terror_errno(\"Couldn't create temporary file %s\",\n+\t\t\t    freq->tmpfile.buf);\n \t\tgoto abort;\n \t}\n \n@@ -2287,7 +2290,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \t * If a previous temp file is present, process what was already\n \t * fetched.\n \t */\n-\tprevlocal = open(prevfile, O_RDONLY);\n+\tprevlocal = open(prevfile.buf, O_RDONLY);\n \tif (prevlocal != -1) {\n \t\tdo {\n \t\t\tprev_read = xread(prevlocal, prev_buf, PREV_BUF_SIZE);\n@@ -2304,7 +2307,8 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \t\t} while (prev_read > 0);\n \t\tclose(prevlocal);\n \t}\n-\tunlink_or_warn(prevfile);\n+\tunlink_or_warn(prevfile.buf);\n+\tstrbuf_release(&prevfile);\n \n \t/*\n \t * Reset inflate/SHA1 if there was an error reading the previous temp\n@@ -2319,7 +2323,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \t\t\tlseek(freq->localfile, 0, SEEK_SET);\n \t\t\tif (ftruncate(freq->localfile, 0) < 0) {\n \t\t\t\terror_errno(\"Couldn't truncate temporary file %s\",\n-\t\t\t\t\t    freq->tmpfile);\n+\t\t\t\t\t    freq->tmpfile.buf);\n \t\t\t\tgoto abort;\n \t\t\t}\n \t\t}\n@@ -2349,6 +2353,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \treturn freq;\n \n abort:\n+\tstrbuf_release(&prevfile);\n \tfree(freq->url);\n \tfree(freq);\n \treturn NULL;\n@@ -2376,24 +2381,24 @@ int finish_http_object_request(struct http_object_request *freq)\n \tif (freq->http_code == 416) {\n \t\twarning(\"requested range invalid; we may already have all the data.\");\n \t} else if (freq->curl_result != CURLE_OK) {\n-\t\tif (stat(freq->tmpfile, &st) == 0)\n+\t\tif (stat(freq->tmpfile.buf, &st) == 0)\n \t\t\tif (st.st_size == 0)\n-\t\t\t\tunlink_or_warn(freq->tmpfile);\n+\t\t\t\tunlink_or_warn(freq->tmpfile.buf);\n \t\treturn -1;\n \t}\n \n \tgit_inflate_end(&freq->stream);\n \tgit_SHA1_Final(freq->real_sha1, &freq->c);\n \tif (freq->zret != Z_STREAM_END) {\n-\t\tunlink_or_warn(freq->tmpfile);\n+\t\tunlink_or_warn(freq->tmpfile.buf);\n \t\treturn -1;\n \t}\n \tif (hashcmp(freq->sha1, freq->real_sha1)) {\n-\t\tunlink_or_warn(freq->tmpfile);\n+\t\tunlink_or_warn(freq->tmpfile.buf);\n \t\treturn -1;\n \t}\n \tsha1_file_name(the_repository, &filename, freq->sha1);\n-\tfreq->rename = finalize_object_file(freq->tmpfile, filename.buf);\n+\tfreq->rename = finalize_object_file(freq->tmpfile.buf, filename.buf);\n \tstrbuf_release(&filename);\n \n \treturn freq->rename;\n@@ -2401,7 +2406,7 @@ int finish_http_object_request(struct http_object_request *freq)\n \n void abort_http_object_request(struct http_object_request *freq)\n {\n-\tunlink_or_warn(freq->tmpfile);\n+\tunlink_or_warn(freq->tmpfile.buf);\n \n \trelease_http_object_request(freq);\n }\n@@ -2421,4 +2426,5 @@ void release_http_object_request(struct http_object_request *freq)\n \t\trelease_active_slot(freq->slot);\n \t\tfreq->slot = NULL;\n \t}\n+\tstrbuf_release(&freq->tmpfile);\n }\ndiff --git a/http.h b/http.h\nindex 4df4a25e1a..d305ca1dc7 100644\n--- a/http.h\n+++ b/http.h\n@@ -207,7 +207,7 @@ struct http_pack_request {\n \tstruct packed_git *target;\n \tstruct packed_git **lst;\n \tFILE *packfile;\n-\tchar tmpfile[PATH_MAX];\n+\tstruct strbuf tmpfile;\n \tstruct active_request_slot *slot;\n };\n \n@@ -219,7 +219,7 @@ extern void release_http_pack_request(struct http_pack_request *preq);\n /* Helpers for fetching object */\n struct http_object_request {\n \tchar *url;\n-\tchar tmpfile[PATH_MAX];\n+\tstruct strbuf tmpfile;\n \tint localfile;\n \tCURLcode curl_result;\n \tchar errorstr[CURL_ERROR_SIZE];\n-- \n2.17.0.1052.g7d69f75dbf\n\n"},{"id":"348020","messageId":"20180519015743.GB32492@sigill.intra.peff.net","threadId":"48519","inReplyTo":"20180519015444.GA12080@sigill.intra.peff.net","subject":"[PATCH 2/5] log_write_email_headers: use strbufs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-19T01:57:44Z","receivedAt":"2018-05-19T01:57:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we write a MIME attachment, we write the mime headers\ninto fixed-size buffers. These are likely to be big enough\nin practice, but technically the input could be arbitrarily\nlarge (e.g., if the caller provided a lot of content in the\nextra_headers string), in which case we'd quietly truncate\nit and generate bogus output. Let's convert these buffers to\nstrbufs.\n\nThe memory ownership here is a bit funny. The original fixed\nbuffers were static, and we merely pass out pointers to them\nto be used by the caller (and in one case, we even just\nstuff our value into the opt->diffopt.stat_sep value).\nIdeally we'd actually pass back heap buffers, and the caller\nwould be responsible for freeing them.\n\nThis patch punts on that cleanup for now, and instead just\nmarks the strbufs as static. That means we keep ownership in\nthis function, making it not a complete leak. This also\ntakes us one step closer to fixing it in the long term\n(since we can eventually use strbuf_detach() to hand\nownership to the caller, once it's ready).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe rest of that cleanup is a possible #leftoverbits.\n\n log-tree.c | 16 ++++++++++------\n 1 file changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex d1c0bedf24..1173fdb057 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -386,11 +386,15 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\tgraph_show_oneline(opt->graph);\n \t}\n \tif (opt->mime_boundary) {\n-\t\tstatic char subject_buffer[1024];\n-\t\tstatic char buffer[1024];\n+\t\tstatic struct strbuf subject_buffer = STRBUF_INIT;\n+\t\tstatic struct strbuf buffer = STRBUF_INIT;\n \t\tstruct strbuf filename =  STRBUF_INIT;\n \t\t*need_8bit_cte_p = -1; /* NEVER */\n-\t\tsnprintf(subject_buffer, sizeof(subject_buffer) - 1,\n+\n+\t\tstrbuf_reset(&subject_buffer);\n+\t\tstrbuf_reset(&buffer);\n+\n+\t\tstrbuf_addf(&subject_buffer,\n \t\t\t \"%s\"\n \t\t\t \"MIME-Version: 1.0\\n\"\n \t\t\t \"Content-Type: multipart/mixed;\"\n@@ -405,13 +409,13 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t extra_headers ? extra_headers : \"\",\n \t\t\t mime_boundary_leader, opt->mime_boundary,\n \t\t\t mime_boundary_leader, opt->mime_boundary);\n-\t\textra_headers = subject_buffer;\n+\t\textra_headers = subject_buffer.buf;\n \n \t\tif (opt->numbered_files)\n \t\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n \t\telse\n \t\t\tfmt_output_commit(&filename, commit, opt);\n-\t\tsnprintf(buffer, sizeof(buffer) - 1,\n+\t\tstrbuf_addf(&buffer,\n \t\t\t \"\\n--%s%s\\n\"\n \t\t\t \"Content-Type: text/x-patch;\"\n \t\t\t \" name=\\\"%s\\\"\\n\"\n@@ -422,7 +426,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t filename.buf,\n \t\t\t opt->no_inline ? \"attachment\" : \"inline\",\n \t\t\t filename.buf);\n-\t\topt->diffopt.stat_sep = buffer;\n+\t\topt->diffopt.stat_sep = buffer.buf;\n \t\tstrbuf_release(&filename);\n \t}\n \t*extra_headers_p = extra_headers;\n-- \n2.17.0.1052.g7d69f75dbf\n\n"},{"id":"348021","messageId":"20180519015758.GC32492@sigill.intra.peff.net","threadId":"48519","inReplyTo":"20180519015444.GA12080@sigill.intra.peff.net","subject":"[PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-19T01:57:58Z","receivedAt":"2018-05-19T01:58:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These formatted integers should always fit into their\n64-byte buffers. Let's use xsnprintf() to add an assertion\nthat this is the case, which makes auditing for other\nunchecked snprintfs() easier.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fsmonitor.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex ed3d1a074d..cc19b27e1d 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -104,8 +104,8 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que\n \tif (!(argv[0] = core_fsmonitor))\n \t\treturn -1;\n \n-\tsnprintf(ver, sizeof(ver), \"%d\", version);\n-\tsnprintf(date, sizeof(date), \"%\" PRIuMAX, (uintmax_t)last_update);\n+\txsnprintf(ver, sizeof(ver), \"%d\", version);\n+\txsnprintf(date, sizeof(date), \"%\" PRIuMAX, (uintmax_t)last_update);\n \targv[1] = ver;\n \targv[2] = date;\n \targv[3] = NULL;\n-- \n2.17.0.1052.g7d69f75dbf\n\n"},{"id":"348022","messageId":"20180519015819.GD32492@sigill.intra.peff.net","threadId":"48519","inReplyTo":"20180519015444.GA12080@sigill.intra.peff.net","subject":"[PATCH 4/5] shorten_unambiguous_ref: use xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-19T01:58:20Z","receivedAt":"2018-05-19T01:58:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We convert the ref_rev_parse_rules array into scanf formats\non the fly, and use snprintf() to write into each string. We\nshould have enough memory to hold everything because of the\nearlier total_len computation. Let's use xsnprintf() to\ngive runtime confirmation that this is the case, and to make\nit easy for people auditing the code to know there's no\ntruncation bug.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 64aadd14c9..73c446628f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1147,8 +1147,8 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \t\tfor (i = 0; i < nr_rules; i++) {\n \t\t\tassert(offset < total_len);\n \t\t\tscanf_fmts[i] = (char *)&scanf_fmts[nr_rules] + offset;\n-\t\t\toffset += snprintf(scanf_fmts[i], total_len - offset,\n-\t\t\t\t\t   ref_rev_parse_rules[i], 2, \"%s\") + 1;\n+\t\t\toffset += xsnprintf(scanf_fmts[i], total_len - offset,\n+\t\t\t\t\t    ref_rev_parse_rules[i], 2, \"%s\") + 1;\n \t\t}\n \t}\n \n-- \n2.17.0.1052.g7d69f75dbf\n\n"},{"id":"348023","messageId":"20180519015843.GE32492@sigill.intra.peff.net","threadId":"48519","inReplyTo":"20180519015444.GA12080@sigill.intra.peff.net","subject":"[PATCH 5/5] fmt_with_err: add a comment that truncation is OK","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-19T01:58:44Z","receivedAt":"2018-05-19T01:58:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Functions like die_errno() use fmt_with_err() to combine the\ncaller-provided format with the strerror() string. We use a\nfixed stack buffer because we're already handling an error\nand don't have any way to report another one. Our buffer\nshould generally be big enough to fit this, but if it's not,\ntruncation is our best option. Let's add a comment to that\neffect, so that anybody auditing the code for truncation\nbugs knows that this is fine.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n usage.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/usage.c b/usage.c\nindex cdd534c9df..b3c78931ad 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -148,6 +148,7 @@ static const char *fmt_with_err(char *buf, int n, const char *fmt)\n \t\t}\n \t}\n \tstr_error[j] = 0;\n+\t/* Truncation is acceptable here */\n \tsnprintf(buf, n, \"%s: %s\", fmt, str_error);\n \treturn buf;\n }\n-- \n2.17.0.1052.g7d69f75dbf\n"},{"id":"348073","messageId":"cd6dba3c-19bb-e6c7-0c1f-5cea0c8f9d81@web.de","threadId":"48519","inReplyTo":"20180519015758.GC32492@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-05-19T08:27:46Z","receivedAt":"2018-05-19T08:28:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.05.2018 um 03:57 schrieb Jeff King:\n> These formatted integers should always fit into their\n> 64-byte buffers. Let's use xsnprintf() to add an assertion\n> that this is the case, which makes auditing for other\n> unchecked snprintfs() easier.\n\nHow about this instead?\n\n-- >8 --\nSubject: [PATCH] fsmonitor: use internal argv_array of struct child_process\n\nAvoid magic array sizes and indexes by constructing the fsmonitor\ncommand line using the embedded argv_array of the child_process.  The\nresulting code is shorter and easier to extend.\n\nGetting rid of the snprintf() calls is a bonus -- even though the\nbuffers were big enough here to avoid truncation -- as it makes auditing\nthe remaining callers easier.\n\nInspired-by: Jeff King <peff@peff.net>\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n fsmonitor.c | 14 ++++----------\n 1 file changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex ed3d1a074d..665bd2d425 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -97,19 +97,13 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tchar ver[64];\n-\tchar date[64];\n-\tconst char *argv[4];\n \n-\tif (!(argv[0] = core_fsmonitor))\n+\tif (!core_fsmonitor)\n \t\treturn -1;\n \n-\tsnprintf(ver, sizeof(ver), \"%d\", version);\n-\tsnprintf(date, sizeof(date), \"%\" PRIuMAX, (uintmax_t)last_update);\n-\targv[1] = ver;\n-\targv[2] = date;\n-\targv[3] = NULL;\n-\tcp.argv = argv;\n+\targv_array_push(&cp.args, core_fsmonitor);\n+\targv_array_pushf(&cp.args, \"%d\", version);\n+\targv_array_pushf(&cp.args, \"%\" PRIuMAX, (uintmax_t)last_update);\n \tcp.use_shell = 1;\n \tcp.dir = get_git_work_tree();\n \n-- \n2.17.0\n"},{"id":"348107","messageId":"20180520170814.GB11967@sigill.intra.peff.net","threadId":"48519","inReplyTo":"cd6dba3c-19bb-e6c7-0c1f-5cea0c8f9d81@web.de","subject":"Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-20T17:08:14Z","receivedAt":"2018-05-20T17:08:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 19, 2018 at 10:27:46AM +0200, René Scharfe wrote:\n\n> Am 19.05.2018 um 03:57 schrieb Jeff King:\n> > These formatted integers should always fit into their\n> > 64-byte buffers. Let's use xsnprintf() to add an assertion\n> > that this is the case, which makes auditing for other\n> > unchecked snprintfs() easier.\n> \n> How about this instead?\n> \n> -- >8 --\n> Subject: [PATCH] fsmonitor: use internal argv_array of struct child_process\n\nYeah, I agree that is much nicer (I was so focused on the snprintfs I\ndidn't bother to look at the context for a better solution).\n\nThanks, getting a review in the form of a complete patch is my favorite\nkind of review. :)\n\n> Inspired-by: Jeff King <peff@peff.net>\n\nHeh.\n\n-Peff\n"},{"id":"348141","messageId":"xmqqmuwt3jvn.fsf@gitster-ct.c.googlers.com","threadId":"48519","inReplyTo":"cd6dba3c-19bb-e6c7-0c1f-5cea0c8f9d81@web.de","subject":"Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-21T00:58:04Z","receivedAt":"2018-05-21T00:58:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> How about this instead?\n>\n> -- >8 --\n> Subject: [PATCH] fsmonitor: use internal argv_array of struct child_process\n>\n> Avoid magic array sizes and indexes by constructing the fsmonitor\n> command line using the embedded argv_array of the child_process.  The\n> resulting code is shorter and easier to extend.\n>\n> Getting rid of the snprintf() calls is a bonus -- even though the\n> buffers were big enough here to avoid truncation -- as it makes auditing\n> the remaining callers easier.\n> ...\n> -\tif (!(argv[0] = core_fsmonitor))\n> +\tif (!core_fsmonitor)\n>  \t\treturn -1;\n>  \n> -\tsnprintf(ver, sizeof(ver), \"%d\", version);\n\nI really like this change, as this exact line used to take\nsizeof(version) instead of sizeof(ver), which has been corrected\nrecently.\n\n> -\tsnprintf(date, sizeof(date), \"%\" PRIuMAX, (uintmax_t)last_update);\n> -\targv[1] = ver;\n> -\targv[2] = date;\n> -\targv[3] = NULL;\n> -\tcp.argv = argv;\n> +\targv_array_push(&cp.args, core_fsmonitor);\n> +\targv_array_pushf(&cp.args, \"%d\", version);\n\nIf it were written like this from the beginning, such a bug would\nnever have happened ;-)\n\n> +\targv_array_pushf(&cp.args, \"%\" PRIuMAX, (uintmax_t)last_update);\n>  \tcp.use_shell = 1;\n>  \tcp.dir = get_git_work_tree();\n"},{"id":"348197","messageId":"c5d9da23-447d-2878-cf4f-e9e7f12ce35f@gmail.com","threadId":"48519","inReplyTo":"cd6dba3c-19bb-e6c7-0c1f-5cea0c8f9d81@web.de","subject":"Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-05-21T12:36:06Z","receivedAt":"2018-05-21T12:36:12Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 5/19/2018 4:27 AM, René Scharfe wrote:\n> Am 19.05.2018 um 03:57 schrieb Jeff King:\n>> These formatted integers should always fit into their\n>> 64-byte buffers. Let's use xsnprintf() to add an assertion\n>> that this is the case, which makes auditing for other\n>> unchecked snprintfs() easier.\n> \n> How about this instead?\n> \n> -- >8 --\n>   \n> -\tsnprintf(ver, sizeof(ver), \"%d\", version);\n> -\tsnprintf(date, sizeof(date), \"%\" PRIuMAX, (uintmax_t)last_update);\n> -\targv[1] = ver;\n> -\targv[2] = date;\n> -\targv[3] = NULL;\n> -\tcp.argv = argv;\n> +\targv_array_push(&cp.args, core_fsmonitor);\n> +\targv_array_pushf(&cp.args, \"%d\", version);\n> +\targv_array_pushf(&cp.args, \"%\" PRIuMAX, (uintmax_t)last_update);\n\nLooks good.  Simpler, cleaner, less error prone.\n\n>   \tcp.use_shell = 1;\n>   \tcp.dir = get_git_work_tree();\n>   \n> \n"},{"id":"348230","messageId":"CAGZ79kYZ9HSCC95qCZ4Zix1oEFLscsU1HZe64s--6KuWEz2ojg@mail.gmail.com","threadId":"48519","inReplyTo":"20180519015636.GA32492@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] http: use strbufs instead of fixed buffers","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-21T18:11:51Z","receivedAt":"2018-05-21T18:11:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Jeff,\n\nOn Fri, May 18, 2018 at 6:56 PM, Jeff King <peff@peff.net> wrote:\n\n> @@ -2421,4 +2426,5 @@ void release_http_object_request(struct http_object_request *freq)\n..\n> +       strbuf_release(&freq->tmpfile);\n\nDo we need an equivalent in release_http_pack_request as well?\n"},{"id":"348244","messageId":"20180521194120.GA9742@sigill.intra.peff.net","threadId":"48519","inReplyTo":"CAGZ79kYZ9HSCC95qCZ4Zix1oEFLscsU1HZe64s--6KuWEz2ojg@mail.gmail.com","subject":"Re: [PATCH 1/5] http: use strbufs instead of fixed buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-21T19:41:20Z","receivedAt":"2018-05-21T19:41:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 21, 2018 at 11:11:51AM -0700, Stefan Beller wrote:\n\n> Hi Jeff,\n> \n> On Fri, May 18, 2018 at 6:56 PM, Jeff King <peff@peff.net> wrote:\n> \n> > @@ -2421,4 +2426,5 @@ void release_http_object_request(struct http_object_request *freq)\n> ..\n> > +       strbuf_release(&freq->tmpfile);\n> \n> Do we need an equivalent in release_http_pack_request as well?\n\nYes, but isn't there one?\n\nFrom the original patch:\n\n--- a/http.c\n+++ b/http.c\n@@ -2082,6 +2082,7 @@ void release_http_pack_request(struct http_pack_request *preq)\n                preq->packfile = NULL;\n        }\n        preq->slot = NULL;\n+       strbuf_release(&preq->tmpfile);\n        free(preq->url);\n        free(preq);\n }\n\nOr am I missing something?\n\n-Peff\n"},{"id":"348247","messageId":"CAGZ79kbPuR8w1goGu6qgMRqF+myMUhtmXYSA7Wxh8VQy8dRBig@mail.gmail.com","threadId":"48519","inReplyTo":"20180521194120.GA9742@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] http: use strbufs instead of fixed buffers","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-21T20:57:22Z","receivedAt":"2018-05-21T20:57:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 21, 2018 at 12:41 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, May 21, 2018 at 11:11:51AM -0700, Stefan Beller wrote:\n>\n>> Hi Jeff,\n>>\n>> On Fri, May 18, 2018 at 6:56 PM, Jeff King <peff@peff.net> wrote:\n>>\n>> > @@ -2421,4 +2426,5 @@ void release_http_object_request(struct http_object_request *freq)\n>> ..\n>> > +       strbuf_release(&freq->tmpfile);\n>>\n>> Do we need an equivalent in release_http_pack_request as well?\n>\n> Yes, but isn't there one?\n>\n> From the original patch:\n>\n> --- a/http.c\n> +++ b/http.c\n> @@ -2082,6 +2082,7 @@ void release_http_pack_request(struct http_pack_request *preq)\n>                 preq->packfile = NULL;\n>         }\n>         preq->slot = NULL;\n> +       strbuf_release(&preq->tmpfile);\n>         free(preq->url);\n>         free(preq);\n>  }\n\nwhich is at the very top, but I was not looking this far up,\nbut looked for functions order that my editor had.\n\nClearly I have to practice code reviews, still.\n\nSorry for the noise.\n\nStefan\n"}]}