{"thread":{"id":"42517","subject":"[PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf","startedAt":"2016-06-03T07:47:15Z","lastAt":"2016-06-03T17:10:01Z","messageCount":21,"participants":["Elia Pinto","Jeff King","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"288197","messageId":"20160603074724.12173-1-gitter.spiros@gmail.com","threadId":"42517","inReplyTo":null,"subject":"[PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-06-03T07:47:15Z","receivedAt":"2016-06-03T07:47:15Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"With the commits f2f02675 and 5096d490 we have been converted in some files the call\nfrom snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls\nto snprintf with xsnprintf under the following conditions:\n\n- The call to snprintf does not control the outcome of the command\n or the presence of truncation errors.\n- A call to snprintf can generate a fatal error, directly or indirectly.\n\nThe other few remaining cases in which a call to snprintf can generate a soft error\nhave not been changed.\n\nSigned-off-by: Elia Pinto \n---\n builtin/commit.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 443ff91..c65abaa 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1552,7 +1552,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n \tcode = start_command(&proc);\n \tif (code)\n \t\treturn code;\n-\tn = snprintf(buf, sizeof(buf), \"%s %s\\n\",\n+\tn = xsnprintf(buf, sizeof(buf), \"%s %s\\n\",\n \t\t sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n \tsigchain_push(SIGPIPE, SIG_IGN);\n \twrite_in_full(proc.in, buf, n);\n-- \n2.9.0.rc1.265.geb5d750\n"},{"id":"288203","messageId":"20160603074724.12173-2-gitter.spiros@gmail.com","threadId":"42517","inReplyTo":"20160603074724.12173-1-gitter.spiros@gmail.com","subject":"[PATCH 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-06-03T07:47:16Z","receivedAt":"2016-06-03T07:47:16Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"With the commits f2f02675 and 5096d490 we have been converted in some files the call\nfrom snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls\nto snprintf with xsnprintf under the following conditions:\n\n- The call to snprintf does not control the outcome of the command\n or the presence of truncation errors.\n- A call to snprintf can generate a fatal error, directly or indirectly.\n\nThe other few remaining cases in which a call to snprintf can generate a soft error\nhave not been changed.\n\nSigned-off-by: Elia Pinto \n---\n builtin/index-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex e8c71fc..c032fe7 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1443,7 +1443,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\n \t} else {\n \t\tchar buf[48];\n-\t\tint len = snprintf(buf, sizeof(buf), \"%s\\t%s\\n\",\n+\t\tint len = xsnprintf(buf, sizeof(buf), \"%s\\t%s\\n\",\n \t\t\t\t report, sha1_to_hex(sha1));\n \t\twrite_or_die(1, buf, len);\n \n-- \n2.9.0.rc1.265.geb5d750\n"},{"id":"288206","messageId":"20160603074724.12173-3-gitter.spiros@gmail.com","threadId":"42517","inReplyTo":"20160603074724.12173-1-gitter.spiros@gmail.com","subject":"[PATCH 03/10] builtin/tag.c: convert trivial snprintf calls to xsnprintf","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-06-03T07:47:17Z","receivedAt":"2016-06-03T07:47:17Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"With the commits f2f02675 and 5096d490 we have been converted in some files the call\nfrom snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls\nto snprintf with xsnprintf under the following conditions:\n\n- The call to snprintf does not control the outcome of the command\n or the presence of truncation errors.\n- A call to snprintf can generate a fatal error, directly or indirectly.\n\nThe other few remaining cases in which a call to snprintf can generate a soft error\nhave not been changed.\n\nSigned-off-by: Elia Pinto \n---\n builtin/tag.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 50e4ae5..0345ca3 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -225,7 +225,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n \tif (type <= OBJ_NONE)\n \t die(_(\"bad object type.\"));\n \n-\theader_len = snprintf(header_buf, sizeof(header_buf),\n+\theader_len = xsnprintf(header_buf, sizeof(header_buf),\n \t\t\t \"object %s\\n\"\n \t\t\t \"type %s\\n\"\n \t\t\t \"tag %s\\n\"\n-- \n2.9.0.rc1.265.geb5d750\n"},{"id":"288198","messageId":"20160603074724.12173-4-gitter.spiros@gmail.com","threadId":"42517","inReplyTo":"20160603074724.12173-1-gitter.spiros@gmail.com","subject":"[PATCH 04/10] combine-diff.c: convert trivial snprintf calls to xsnprintf","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-06-03T07:47:18Z","receivedAt":"2016-06-03T07:47:18Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"With the commits f2f02675 and 5096d490 we have been converted in some files the call\nfrom snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls\nto snprintf with xsnprintf under the following conditions:\n\n- The call to snprintf does not control the outcome of the command\n or the presence of truncation errors.\n- A call to snprintf can generate a fatal error, directly or indirectly.\n\nThe other few remaining cases in which a call to snprintf can generate a soft error\nhave not been changed.\n\nSigned-off-by: Elia Pinto \n---\n combine-diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 8f2313d..a91d9b3 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -293,7 +293,7 @@ static char *grab_blob(const struct object_id *oid, unsigned int mode,\n \n \tif (S_ISGITLINK(mode)) {\n \t\tblob = xmalloc(100);\n-\t\t*size = snprintf(blob, 100,\n+\t\t*size = xsnprintf(blob, 100,\n \t\t\t\t \"Subproject commit %s\\n\", oid_to_hex(oid));\n \t} else if (is_null_oid(oid)) {\n \t\t/* deleted blob */\n-- \n2.9.0.rc1.265.geb5d750\n"},{"id":"288205","messageId":"20160603074724.12173-5-gitter.spiros@gmail.com","threadId":"42517","inReplyTo":"20160603074724.12173-1-gitter.spiros@gmail.com","subject":"[PATCH 05/10] compat/inet_ntop.c: convert trivial snprintf calls to xsnprintf","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-06-03T07:47:19Z","receivedAt":"2016-06-03T07:47:19Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"With the commits f2f02675 and 5096d490 we have been converted in some files the call\nfrom snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls\nto snprintf with xsnprintf under the following conditions:\n\n- The call to snprintf does not control the outcome of the command\n or the presence of truncation errors.\n- A call to snprintf can generate a fatal error, directly or indirectly.\n\nThe other few remaining cases in which a call to snprintf can generate a soft error\nhave not been changed.\n\nSigned-off-by: Elia Pinto \n---\n compat/inet_ntop.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/inet_ntop.c b/compat/inet_ntop.c\nindex 6830726..a4a6546 100644\n--- a/compat/inet_ntop.c\n+++ b/compat/inet_ntop.c\n@@ -139,7 +139,7 @@ inet_ntop6(const u_char *src, char *dst, size_t size)\n \t\t\ttp += strlen(tp);\n \t\t\tbreak;\n \t\t}\n-\t\ttp += snprintf(tp, sizeof tmp - (tp - tmp), \"%x\", words[i]);\n+\t\ttp += xsnprintf(tp, sizeof tmp - (tp - tmp), \"%x\", words[i]);\n \t}\n \t/* Was it a trailing run of 0x00's? */\n \tif (best.base != -1 && (best.base + best.len) ==\n-- \n2.9.0.rc1.265.geb5d750\n"},{"id":"288201","messageId":"20160603074724.12173-6-gitter.spiros@gmail.com","threadId":"42517","inReplyTo":"20160603074724.12173-1-gitter.spiros@gmail.com","subject":"[PATCH 06/10] diff.c: convert trivial snprintf calls to xsnprintf","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-06-03T07:47:20Z","receivedAt":"2016-06-03T07:47:20Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"With the commits f2f02675 and 5096d490 we have been converted in some files the call\nfrom snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls\nto snprintf with xsnprintf under the following conditions:\n\n- The call to snprintf does not control the outcome of the command\n or the presence of truncation errors.\n- A call to snprintf can generate a fatal error, directly or indirectly.\n\nThe other few remaining cases in which a call to snprintf can generate a soft error\nhave not been changed.\n\nSigned-off-by: Elia Pinto \n---\n diff.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex d3734d3..fb61539 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4491,7 +4491,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \t\tlen1 = remove_space(p->one->path, strlen(p->one->path));\n \t\tlen2 = remove_space(p->two->path, strlen(p->two->path));\n \t\tif (p->one->mode == 0)\n-\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n+\t\t\tlen1 = xsnprintf(buffer, sizeof(buffer),\n \t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n \t\t\t\t\t\"newfilemode%06o\"\n \t\t\t\t\t\"---/dev/null\"\n@@ -4501,7 +4501,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \t\t\t\t\tp->two->mode,\n \t\t\t\t\tlen2, p->two->path);\n \t\telse if (p->two->mode == 0)\n-\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n+\t\t\tlen1 = xsnprintf(buffer, sizeof(buffer),\n \t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n \t\t\t\t\t\"deletedfilemode%06o\"\n \t\t\t\t\t\"---a/%.*s\"\n@@ -4511,7 +4511,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \t\t\t\t\tp->one->mode,\n \t\t\t\t\tlen1, p->one->path);\n \t\telse\n-\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n+\t\t\tlen1 = xsnprintf(buffer, sizeof(buffer),\n \t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n \t\t\t\t\t\"---a/%.*s\"\n \t\t\t\t\t\"+++b/%.*s\",\n-- \n2.9.0.rc1.265.geb5d750\n"},{"id":"288200","messageId":"20160603074724.12173-7-gitter.spiros@gmail.com","threadId":"42517","inReplyTo":"20160603074724.12173-1-gitter.spiros@gmail.com","subject":"[PATCH 07/10] fast-import.c: convert trivial snprintf calls to xsnprintf","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-06-03T07:47:21Z","receivedAt":"2016-06-03T07:47:21Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"With the commits f2f02675 and 5096d490 we have been converted in some files the call\nfrom snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls\nto snprintf with xsnprintf under the following conditions:\n\n- The call to snprintf does not control the outcome of the command\n or the presence of truncation errors.\n- A call to snprintf can generate a fatal error, directly or indirectly.\n\nThe other few remaining cases in which a call to snprintf can generate a soft error\nhave not been changed.\n\nSigned-off-by: Elia Pinto \n---\n fast-import.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 59630ce..5beb63d 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1240,10 +1240,7 @@ static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)\n \tsha1file_checkpoint(pack_file, &checkpoint);\n \toffset = checkpoint.offset;\n \n-\thdrlen = snprintf((char *)out_buf, out_sz, \"blob %\" PRIuMAX, len) + 1;\n-\tif (out_sz <= hdrlen)\n-\t\tdie(\"impossibly large object header\");\n-\n+\thdrlen = xsnprintf((char *)out_buf, out_sz, \"blob %\" PRIuMAX, len) + 1;\n \tgit_SHA1_Init(&c);\n \tgit_SHA1_Update(&c, out_buf, hdrlen);\n \n-- \n2.9.0.rc1.265.geb5d750\n"},{"id":"288199","messageId":"20160603074724.12173-8-gitter.spiros@gmail.com","threadId":"42517","inReplyTo":"20160603074724.12173-1-gitter.spiros@gmail.com","subject":"[PATCH 08/10] refs.c: convert trivial snprintf calls to xsnprintf","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-06-03T07:47:22Z","receivedAt":"2016-06-03T07:47:22Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"With the commits f2f02675 and 5096d490 we have been converted in some files the call\nfrom snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls\nto snprintf with xsnprintf under the following conditions:\n\n- The call to snprintf does not control the outcome of the command\n or the presence of truncation errors.\n- A call to snprintf can generate a fatal error, directly or indirectly.\n\nThe other few remaining cases in which a call to snprintf can generate a soft error\nhave not been changed.\n\nSigned-off-by: Elia Pinto \n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 87dc82f..c797ea0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -912,7 +912,7 @@ 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\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-- \n2.9.0.rc1.265.geb5d750\n"},{"id":"288204","messageId":"20160603074724.12173-9-gitter.spiros@gmail.com","threadId":"42517","inReplyTo":"20160603074724.12173-1-gitter.spiros@gmail.com","subject":"[PATCH 09/10] transport-helper.c: convert trivial snprintf calls to xsnprintf","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-06-03T07:47:23Z","receivedAt":"2016-06-03T07:47:23Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"With the commits f2f02675 and 5096d490 we have been converted in some files the call\nfrom snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls\nto snprintf with xsnprintf under the following conditions:\n\n- The call to snprintf does not control the outcome of the command\n or the presence of truncation errors.\n- A call to snprintf can generate a fatal error, directly or indirectly.\n\nThe other few remaining cases in which a call to snprintf can generate a soft error\nhave not been changed.\n\nSigned-off-by: Elia Pinto \n---\n transport-helper.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex bd666b2..18e9f44 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -317,9 +317,7 @@ static void standard_options(struct transport *t)\n \n \tset_helper_option(t, \"progress\", t->progress ? \"true\" : \"false\");\n \n-\tn = snprintf(buf, sizeof(buf), \"%d\", v + 1);\n-\tif (n >= sizeof(buf))\n-\t\tdie(\"impossibly large verbosity value\");\n+\tn = xsnprintf(buf, sizeof(buf), \"%d\", v + 1);\n \tset_helper_option(t, \"verbosity\", buf);\n \n \tswitch (t->family) {\n-- \n2.9.0.rc1.265.geb5d750\n"},{"id":"288202","messageId":"20160603074724.12173-10-gitter.spiros@gmail.com","threadId":"42517","inReplyTo":"20160603074724.12173-1-gitter.spiros@gmail.com","subject":"[PATCH 10/10] wrapper.c: convert trivial snprintf calls to xsnprintf","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-06-03T07:47:24Z","receivedAt":"2016-06-03T07:47:24Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"With the commits f2f02675 and 5096d490 we have been converted in some files the call\nfrom snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls\nto snprintf with xsnprintf under the following conditions:\n\n- The call to snprintf does not control the outcome of the command\n or the presence of truncation errors.\n- A call to snprintf can generate a fatal error, directly or indirectly.\n\nThe other few remaining cases in which a call to snprintf can generate a soft error\nhave not been changed.\n\nSigned-off-by: Elia Pinto \n---\n wrapper.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 5dc4e15..0d44835 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -433,16 +433,11 @@ int xmkstemp(char *template)\n int git_mkstemp(char *path, size_t len, const char *template)\n {\n \tconst char *tmp;\n-\tsize_t n;\n \n \ttmp = getenv(\"TMPDIR\");\n \tif (!tmp)\n \t\ttmp = \"/tmp\";\n-\tn = snprintf(path, len, \"%s/%s\", tmp, template);\n-\tif (len <= n) {\n-\t\terrno = ENAMETOOLONG;\n-\t\treturn -1;\n-\t}\n+\t(void)xsnprintf(path, len, \"%s/%s\", tmp, template);\n \treturn mkstemp(path);\n }\n \n-- \n2.9.0.rc1.265.geb5d750\n"},{"id":"288207","messageId":"20160603084917.GA28401@sigill.intra.peff.net","threadId":"42517","inReplyTo":"20160603074724.12173-1-gitter.spiros@gmail.com","subject":"Re: [PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-03T08:49:18Z","receivedAt":"2016-06-03T08:49:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2016 at 07:47:15AM +0000, Elia Pinto wrote:\n\n> With the commits f2f02675 and 5096d490 we have been converted in some files the call\n> from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls\n> to snprintf with xsnprintf under the following conditions:\n> \n> - The call to snprintf does not control the outcome of the command\n> or the presence of truncation errors.\n> - A call to snprintf can generate a fatal error, directly or indirectly.\n> \n> The other few remaining cases in which a call to snprintf can generate a soft error\n> have not been changed.\n\nI see that all 10 of these commits have the same commit message. IMHO,\nthat is a good sign that they should just be a single commit.\n\nIt is a good idea to break your changes up into small commits, but I\nthink it only makes sense to do so on _logical_ boundaries. For a\ncross code-base change like this, it doesn't really matter what file\nthese are in. They are all the same logical change; they have the same\nprerequisites to be a good candidate for the change, and the mechanism\nitself is the same in all cases. Somebody reviewing them would apply the\nsame criteria in all cases.\n\nI haven't looked carefully at each call site in detail yet, but from the\nprevious rounds of sprintf handling, I'd guess each site falls into one\nof two categories:\n\n 1. We've sized the buffer appropriately earlier in the function, so\n this is a \"should never truncate\" case. Using xsnprintf gives us a\n run-time assurance that there was no programming error.\n\n 2. The original author didn't give much thought to the buffer size and\n figured \"probably big enough\". These ones might actually cause\n truncation in pathological cases, but we'd prefer to generate a\n fatal error, since that's better than continuing with bogus data.\n\nBut again, that's just a guess. There might be other ways of grouping\nthe changes logically.\n\nFor this particular change:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 443ff91..c65abaa 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1552,7 +1552,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n> \tcode = start_command(&proc);\n> \tif (code)\n> \t\treturn code;\n> -\tn = snprintf(buf, sizeof(buf), \"%s %s\\n\",\n> +\tn = xsnprintf(buf, sizeof(buf), \"%s %s\\n\",\n> \t\t sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n\nI think it's the first type, as earlier we have:\n\n\t/* oldsha1 SP newsha1 LF NUL */\n\tstatic char buf[2*40 + 3];\n\nSo unless that calculation is wrong, this would never truncate. The move\nto xsnprintf is an improvement, but I wonder if we would be happier\nstill with:\n\n buf = xstrfmt(\"%s %s\\n\", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n\nThen we do not even have to wonder about the size computation. True, it\nuses a heap buffer when we do not need to, but I find it hard to care\nabout grabbing 80 bytes of heap when we are in the midst of exec-ing an\nentirely new process.\n\nBy the way, there are some other uses of snprintf in the same file, that\nI think would fall into the type 2 I mentioned above (they use PATH_MAX,\nwhich I think is shorter than necessary on some systems).\n\nThose ones would also benefit from using higher-level constructs. Like:\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 443ff91..9336724 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1563,24 +1563,23 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n \n int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)\n {\n-\tconst char *hook_env[3] = { NULL };\n-\tchar index[PATH_MAX];\n+\tstruct argv_array hook_env = ARGV_ARRAY_INIT;\n \tva_list args;\n \tint ret;\n \n-\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n-\thook_env[0] = index;\n+\targv_array_pushf(&hook_env, \"GIT_INDEX_FILE=%s\", index_file);\n \n \t/*\n \t * Let the hook know that no editor will be launched.\n \t */\n \tif (!editor_is_used)\n-\t\thook_env[1] = \"GIT_EDITOR=:\";\n+\t\targv_array_push(&hook_env, \"GIT_EDITOR=:\");\n \n \tva_start(args, name);\n \tret = run_hook_ve(hook_env, name, args);\n \tva_end(args);\n \n+\targv_array_clear(&hook_env);\n \treturn ret;\n }\n \n\n-Peff\n"},{"id":"288208","messageId":"20160603085231.GB28401@sigill.intra.peff.net","threadId":"42517","inReplyTo":"20160603074724.12173-3-gitter.spiros@gmail.com","subject":"Re: [PATCH 03/10] builtin/tag.c: convert trivial snprintf calls to xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-03T08:52:32Z","receivedAt":"2016-06-03T08:52:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2016 at 07:47:17AM +0000, Elia Pinto wrote:\n\n> builtin/tag.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 50e4ae5..0345ca3 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -225,7 +225,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n> \tif (type <= OBJ_NONE)\n> \t die(_(\"bad object type.\"));\n> \n> -\theader_len = snprintf(header_buf, sizeof(header_buf),\n> +\theader_len = xsnprintf(header_buf, sizeof(header_buf),\n> \t\t\t \"object %s\\n\"\n> \t\t\t \"type %s\\n\"\n> \t\t\t \"tag %s\\n\"\n\nThis is another of my \"type 2\" cases. I'd argue it should be using a\nheap buffer to handle tag and tagger names of arbitrary size.\n\n-Peff\n"},{"id":"288209","messageId":"20160603085320.GC28401@sigill.intra.peff.net","threadId":"42517","inReplyTo":"20160603074724.12173-2-gitter.spiros@gmail.com","subject":"Re: [PATCH 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-03T08:53:20Z","receivedAt":"2016-06-03T08:53:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2016 at 07:47:16AM +0000, Elia Pinto wrote:\n\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index e8c71fc..c032fe7 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -1443,7 +1443,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n> \t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\n> \t} else {\n> \t\tchar buf[48];\n> -\t\tint len = snprintf(buf, sizeof(buf), \"%s\\t%s\\n\",\n> +\t\tint len = xsnprintf(buf, sizeof(buf), \"%s\\t%s\\n\",\n> \t\t\t\t report, sha1_to_hex(sha1));\n> \t\twrite_or_die(1, buf, len);\n\nSo it's pretty unclear here whether that 48 is big enough (it is, if you\nread the whole function, because \"report\" is always a 4-char string).\nYuck. At least there should be a comment explaining why 48 is big\nenough.\n\n-Peff\n"},{"id":"288210","messageId":"20160603085441.GD28401@sigill.intra.peff.net","threadId":"42517","inReplyTo":"20160603074724.12173-4-gitter.spiros@gmail.com","subject":"Re: [PATCH 04/10] combine-diff.c: convert trivial snprintf calls to xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-03T08:54:41Z","receivedAt":"2016-06-03T08:54:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2016 at 07:47:18AM +0000, Elia Pinto wrote:\n\n> diff --git a/combine-diff.c b/combine-diff.c\n> index 8f2313d..a91d9b3 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -293,7 +293,7 @@ static char *grab_blob(const struct object_id *oid, unsigned int mode,\n> \n> \tif (S_ISGITLINK(mode)) {\n> \t\tblob = xmalloc(100);\n> -\t\t*size = snprintf(blob, 100,\n> +\t\t*size = xsnprintf(blob, 100,\n> \t\t\t\t \"Subproject commit %s\\n\", oid_to_hex(oid));\n\nThis one seems like a no-brainer to convert to xstrmft(), since we're\nalready using a heap buffer, and then we can get rid of that meaningless\n\"100\" magic number.\n\n-Peff\n"},{"id":"288211","messageId":"20160603090320.GE28401@sigill.intra.peff.net","threadId":"42517","inReplyTo":"20160603074724.12173-6-gitter.spiros@gmail.com","subject":"Re: [PATCH 06/10] diff.c: convert trivial snprintf calls to xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-03T09:03:20Z","receivedAt":"2016-06-03T09:03:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2016 at 07:47:20AM +0000, Elia Pinto wrote:\n\n> diff.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index d3734d3..fb61539 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4491,7 +4491,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n> \t\tlen1 = remove_space(p->one->path, strlen(p->one->path));\n> \t\tlen2 = remove_space(p->two->path, strlen(p->two->path));\n> \t\tif (p->one->mode == 0)\n> -\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n> +\t\t\tlen1 = xsnprintf(buffer, sizeof(buffer),\n> \t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n> \t\t\t\t\t\"newfilemode%06o\"\n> \t\t\t\t\t\"---/dev/null\"\n\nMore that assume that PATH_MAX is plenty big for any patch we would\nsee. I'd argue these should be on the heap, which removes that\nrestriction, and gets rid of the \"PATH_MAX * 4 + 20\" magic at the top of\nthe function.\n\nIt looks like the buffer just gets fed into sha1, so I was also tempted\nto suggest we simply feed the bits of string in directly. But we are\nusing the format string here to handle the mode, so you'd still have to\ndeal with that.\n\n-Peff\n"},{"id":"288212","messageId":"20160603090448.GF28401@sigill.intra.peff.net","threadId":"42517","inReplyTo":"20160603084917.GA28401@sigill.intra.peff.net","subject":"Re: [PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-03T09:04:48Z","receivedAt":"2016-06-03T09:04:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2016 at 04:49:18AM -0400, Jeff King wrote:\n\n> I haven't looked carefully at each call site in detail yet, but from the\n> previous rounds of sprintf handling, I'd guess each site falls into one\n> of two categories:\n\nSo that being said, I did just look at several of them. And I think they\n_do_ fall into these categories, but I think the solutions don't\nnecessarily. So what I would do is find solutions for each, and then go\nback and group them by situation and solution-type using \"git rebase\n-i\". It may be that you end up with 10 patches, or it may be that you\nsee some patterns. But I think grouping is helpful for reviewers,\nbecause it communicates the patterns you found while doing the work.\n\n-Peff\n"},{"id":"288213","messageId":"20160603091347.GG28401@sigill.intra.peff.net","threadId":"42517","inReplyTo":"20160603074724.12173-10-gitter.spiros@gmail.com","subject":"Re: [PATCH 10/10] wrapper.c: convert trivial snprintf calls to xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-03T09:13:47Z","receivedAt":"2016-06-03T09:13:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2016 at 07:47:24AM +0000, Elia Pinto wrote:\n\n> diff --git a/wrapper.c b/wrapper.c\n> index 5dc4e15..0d44835 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -433,16 +433,11 @@ int xmkstemp(char *template)\n> int git_mkstemp(char *path, size_t len, const char *template)\n> {\n> \tconst char *tmp;\n> -\tsize_t n;\n> \n> \ttmp = getenv(\"TMPDIR\");\n> \tif (!tmp)\n> \t\ttmp = \"/tmp\";\n> -\tn = snprintf(path, len, \"%s/%s\", tmp, template);\n> -\tif (len <= n) {\n> -\t\terrno = ENAMETOOLONG;\n> -\t\treturn -1;\n> -\t}\n> +\t(void)xsnprintf(path, len, \"%s/%s\", tmp, template);\n> \treturn mkstemp(path);\n> }\n\nI don't think this one is a good change. git_mkstemp is a library\nfunction, and there are places that indirectly call it (via the gpg\ncode) that do not die when it fails (and even if all the callers _did_\ndie, isn't \"could not create temporary file ...\" a better error message\nthan \"BUG: attempt to snprintf into too-small buffer\"?).\n\n-Peff\n"},{"id":"288256","messageId":"5751A168.7020408@ramsayjones.plus.com","threadId":"42517","inReplyTo":"20160603084917.GA28401@sigill.intra.peff.net","subject":"Re: [PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-06-03T15:25:28Z","receivedAt":"2016-06-03T15:25:28Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 03/06/16 09:49, Jeff King wrote:\n> On Fri, Jun 03, 2016 at 07:47:15AM +0000, Elia Pinto wrote:\n> \n[snip]\n\n> For this particular change:\n> \n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 443ff91..c65abaa 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -1552,7 +1552,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n>> \tcode = start_command(&proc);\n>> \tif (code)\n>> \t\treturn code;\n>> -\tn = snprintf(buf, sizeof(buf), \"%s %s\\n\",\n>> +\tn = xsnprintf(buf, sizeof(buf), \"%s %s\\n\",\n>> \t\t sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n> \n> I think it's the first type, as earlier we have:\n> \n> \t/* oldsha1 SP newsha1 LF NUL */\n> \tstatic char buf[2*40 + 3];\n> \n> So unless that calculation is wrong, this would never truncate. The move\n> to xsnprintf is an improvement, \n\nI was going to suggest, if we stay with the static buffer, that the size\nexpression be changed to '2 * GIT_SHA1_HEXSZ + 3'. However, ...\n\n> but I wonder if we would be happier\n> still with:\n> \n> buf = xstrfmt(\"%s %s\\n\", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n> \n> Then we do not even have to wonder about the size computation. True, it\n> uses a heap buffer when we do not need to, but I find it hard to care\n> about grabbing 80 bytes of heap when we are in the midst of exec-ing an\n> entirely new process.\n\n... I agree with this also.\n\n> \n> By the way, there are some other uses of snprintf in the same file, that\n> I think would fall into the type 2 I mentioned above (they use PATH_MAX,\n> which I think is shorter than necessary on some systems).\n> \n> Those ones would also benefit from using higher-level constructs. Like:\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 443ff91..9336724 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1563,24 +1563,23 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n> \n> int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)\n> {\n> -\tconst char *hook_env[3] = { NULL };\n> -\tchar index[PATH_MAX];\n> +\tstruct argv_array hook_env = ARGV_ARRAY_INIT;\n> \tva_list args;\n> \tint ret;\n> \n> -\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n> -\thook_env[0] = index;\n> +\targv_array_pushf(&hook_env, \"GIT_INDEX_FILE=%s\", index_file);\n> \n> \t/*\n> \t * Let the hook know that no editor will be launched.\n> \t */\n> \tif (!editor_is_used)\n> -\t\thook_env[1] = \"GIT_EDITOR=:\";\n> +\t\targv_array_push(&hook_env, \"GIT_EDITOR=:\");\n> \n> \tva_start(args, name);\n> \tret = run_hook_ve(hook_env, name, args);\n> \tva_end(args);\n> \n> +\targv_array_clear(&hook_env);\n> \treturn ret;\n> }\n\nIndeed.\n\nATB,\nRamsay Jones\n"},{"id":"288258","messageId":"5751A319.1030806@ramsayjones.plus.com","threadId":"42517","inReplyTo":"20160603085320.GC28401@sigill.intra.peff.net","subject":"Re: [PATCH 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-06-03T15:32:41Z","receivedAt":"2016-06-03T15:32:41Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 03/06/16 09:53, Jeff King wrote:\n> On Fri, Jun 03, 2016 at 07:47:16AM +0000, Elia Pinto wrote:\n> \n>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n>> index e8c71fc..c032fe7 100644\n>> --- a/builtin/index-pack.c\n>> +++ b/builtin/index-pack.c\n>> @@ -1443,7 +1443,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n>> \t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\n>> \t} else {\n>> \t\tchar buf[48];\n>> -\t\tint len = snprintf(buf, sizeof(buf), \"%s\\t%s\\n\",\n>> +\t\tint len = xsnprintf(buf, sizeof(buf), \"%s\\t%s\\n\",\n>> \t\t\t\t report, sha1_to_hex(sha1));\n>> \t\twrite_or_die(1, buf, len);\n> \n> So it's pretty unclear here whether that 48 is big enough (it is, if you\n> read the whole function, because \"report\" is always a 4-char string).\n> Yuck. At least there should be a comment explaining why 48 is big\n> enough.\n\nAgreed, again I would use something like:\n\n\t\tchar buf[GIT_SHA1_HEXSZ + 7]; /* 40 (sha1) + 4 (report) + 3 (\\t\\n\\0) */\n\n(and yes yuck - is report ever likely to increase? \"bitmap\" perhaps?)\n\nATB,\nRamsay Jones\n"},{"id":"288259","messageId":"5751A450.4040702@ramsayjones.plus.com","threadId":"42517","inReplyTo":"20160603085231.GB28401@sigill.intra.peff.net","subject":"Re: [PATCH 03/10] builtin/tag.c: convert trivial snprintf calls to xsnprintf","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-06-03T15:37:52Z","receivedAt":"2016-06-03T15:37:52Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 03/06/16 09:52, Jeff King wrote:\n> On Fri, Jun 03, 2016 at 07:47:17AM +0000, Elia Pinto wrote:\n> \n>> builtin/tag.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/tag.c b/builtin/tag.c\n>> index 50e4ae5..0345ca3 100644\n>> --- a/builtin/tag.c\n>> +++ b/builtin/tag.c\n>> @@ -225,7 +225,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n>> \tif (type <= OBJ_NONE)\n>> \t die(_(\"bad object type.\"));\n>> \n>> -\theader_len = snprintf(header_buf, sizeof(header_buf),\n>> +\theader_len = xsnprintf(header_buf, sizeof(header_buf),\n>> \t\t\t \"object %s\\n\"\n>> \t\t\t \"type %s\\n\"\n>> \t\t\t \"tag %s\\n\"\n> \n> This is another of my \"type 2\" cases. I'd argue it should be using a\n> heap buffer to handle tag and tagger names of arbitrary size.\n\nYep. As it stands, the code following this hunk:\n\n if (header_len > sizeof(header_buf) - 1)\n die(_(\"tag header too big.\"));\n\nis now dead code, and the new error message is not as useful.\n\nATB,\nRamsay Jones\n"},{"id":"288274","messageId":"20160603171001.GB3858@sigill.intra.peff.net","threadId":"42517","inReplyTo":"5751A319.1030806@ramsayjones.plus.com","subject":"Re: [PATCH 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-03T17:10:01Z","receivedAt":"2016-06-03T17:10:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2016 at 04:32:41PM +0100, Ramsay Jones wrote:\n\n> >> \t\tchar buf[48];\n> >> -\t\tint len = snprintf(buf, sizeof(buf), \"%s\\t%s\\n\",\n> >> +\t\tint len = xsnprintf(buf, sizeof(buf), \"%s\\t%s\\n\",\n> >> \t\t\t\t report, sha1_to_hex(sha1));\n> >> \t\twrite_or_die(1, buf, len);\n> > \n> > So it's pretty unclear here whether that 48 is big enough (it is, if you\n> > read the whole function, because \"report\" is always a 4-char string).\n> > Yuck. At least there should be a comment explaining why 48 is big\n> > enough.\n> \n> Agreed, again I would use something like:\n> \n> \t\tchar buf[GIT_SHA1_HEXSZ + 7]; /* 40 (sha1) + 4 (report) + 3 (\\t\\n\\0) */\n\nYes, that's much better, I think.\n\n> (and yes yuck - is report ever likely to increase? \"bitmap\" perhaps?)\n\nIt shouldn't. It's easy to think that's a filetype, but it really is\njust \"did you tell me --keep\". TBH, I am not really sure that switching\nit accomplishes anything.\n\n-Peff\n"}]}