{"thread":{"id":"53252","subject":"[PATCH] mailinfo.c::convert_to_utf8: reuse strlen info","startedAt":"2020-04-18T03:55:08Z","lastAt":"2020-04-21T18:47:15Z","messageCount":39,"participants":["Đoàn Trần Công Danh","Martin Ågren","Junio C Hamano","Danh Doan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"395679","messageId":"20200418035449.9450-1-congdanhqx@gmail.com","threadId":"53252","inReplyTo":null,"subject":"[PATCH] mailinfo.c::convert_to_utf8: reuse strlen info","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-18T03:54:49Z","receivedAt":"2020-04-18T03:55:08Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"We're passing buffer from strbuf to reencode_string,\nwhich will call strlen(3) on that buffer,\nand discard the length of newly created buffer.\n\nThen, we compute the length of the return buffer to attach to strbuf.\n\nWe can do better by reusing all the available information,\nand call the underlying lower level function that will be called\nindirectly by reencode_string.\n\nSigned-off-by: Đoàn Trần Công Danh \n---\n mailinfo.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 742fa376ab..0e9911df6d 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -447,19 +447,21 @@ static int convert_to_utf8(struct mailinfo *mi,\n \t\t\t struct strbuf *line, const char *charset)\n {\n \tchar *out;\n+\tsize_t out_len;\n \n \tif (!mi->metainfo_charset || !charset || !*charset)\n \t\treturn 0;\n \n \tif (same_encoding(mi->metainfo_charset, charset))\n \t\treturn 0;\n-\tout = reencode_string(line->buf, mi->metainfo_charset, charset);\n+\tout = reencode_string_len(line->buf, line->len,\n+\t\t\t\t mi->metainfo_charset, charset, &out_len);\n \tif (!out) {\n \t\tmi->input_error = -1;\n \t\treturn error(\"cannot convert from %s to %s\",\n \t\t\t charset, mi->metainfo_charset);\n \t}\n-\tstrbuf_attach(line, out, strlen(out), strlen(out));\n+\tstrbuf_attach(line, out, out_len, out_len);\n \treturn 0;\n }\n \n-- \n2.26.1.301.g55bc3eb7cb\n\n"},{"id":"395693","messageId":"CAN0heSppn6BBX4V1T1qgKc4XP+8i6qbcEqd1_3NqWQtZJLaJww@mail.gmail.com","threadId":"53252","inReplyTo":"20200418035449.9450-1-congdanhqx@gmail.com","subject":"Re: [PATCH] mailinfo.c::convert_to_utf8: reuse strlen info","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-18T19:56:34Z","receivedAt":"2020-04-18T19:56:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Danh (I think that's how you want to be addressed?),\n\nOn Sat, 18 Apr 2020 at 06:00, Đoàn Trần Công Danh wrote:\n> @@ -447,19 +447,21 @@ static int convert_to_utf8(struct mailinfo *mi,\n> struct strbuf *line, const char *charset)\n> {\n> char *out;\n> + size_t out_len;\n>\n> if (!mi->metainfo_charset || !charset || !*charset)\n> return 0;\n>\n> if (same_encoding(mi->metainfo_charset, charset))\n> return 0;\n> - out = reencode_string(line->buf, mi->metainfo_charset, charset);\n> + out = reencode_string_len(line->buf, line->len,\n> + mi->metainfo_charset, charset, &out_len);\n\nThis is equivalent as long as `line->len` is equal to\n`strlen(line->buf)`, which it will be (should be) because it's a\nstrbuf. Ok.\n\n> if (!out) {\n> mi->input_error = -1;\n> return error(\"cannot convert from %s to %s\",\n> charset, mi->metainfo_charset);\n> }\n> - strbuf_attach(line, out, strlen(out), strlen(out));\n> + strbuf_attach(line, out, out_len, out_len);\n\nThis conversion is ok as such. I wondered why we pass in the same\nvalue twice (before and after this patch). Turns out this usage is wrong\n(as per the documentation in strbuf.h) but safe (as per my understanding\nof the implementation in strbuf.c). I'll follow up with a series that\nfell out of that investigation.\n\n> return 0;\n> }\n\nAll in all, this conversion is correct and it doesn't leave the\nuse of `strbuf_attach()` any less correct than it already was.\n\n\nMartin\n"},{"id":"395694","messageId":"cover.1587240635.git.martin.agren@gmail.com","threadId":"53252","inReplyTo":"CAN0heSppn6BBX4V1T1qgKc4XP+8i6qbcEqd1_3NqWQtZJLaJww@mail.gmail.com","subject":"[PATCH 0/6] strbuf: simplify `strbuf_attach()` usage","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-18T20:18:23Z","receivedAt":"2020-04-18T20:18:51Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sat, 18 Apr 2020 at 21:56, Martin Ågren wrote:\n> > - strbuf_attach(line, out, strlen(out), strlen(out));\n> > + strbuf_attach(line, out, out_len, out_len);\n>\n> This conversion is ok as such. I wondered why we pass in the same\n> value twice (before and after this patch). Turns out this usage is wrong\n> (as per the documentation in strbuf.h) but safe (as per my understanding\n> of the implementation in strbuf.c). I'll follow up with a series that\n> fell out of that investigation.\n\nHere's that series. It could go in parallel to Danh's, or one could go\non top of the other. Any of those would be ok with me.\n\nI think `strbuf_attach()` is sufficiently hard to use that we might as\nwell simplify it for the majority of the users that don't care about the\ndistinction between the string's length and the size of the allocated\nmemory, and avoid it for the few remaining users that are just as well\noff using `strbuf_add()`.\n\nThe summary is that this function takes `len` and `alloc`, where the\nlatter must be greater than the former, yet several callers use the same\nvalue for both. I first thought this could cause quite hairy problems\nsuch as writing outside of allocated memory, but it doesn't look that\nway. See the patches for more information.\n\nAn alternative to the approach taken here would be to introduce\n`strbuf_attachstr()` and convert most existing users, then convert the\nfew remaining ones to use the new function or to move in another\ndirection. But the new name is a downer -- what else would you attach to\na strbuf if not a string?\n\nAnother alternative would be to first convert certain users away from\nthe function, then simplify it for the remainder. I preferred to first\nspend a few patches converting a few existing callers to make it clear\nthat those are ok.\n\nMartin\n\nMartin Ågren (6):\n am: use `strbuf_attach()` correctly\n strbuf_attach: correctly pass in `strlen() + 1` for `alloc`\n strbuf: use `strbuf_attach()` correctly\n fast-import: avoid awkward use of `strbuf_attach()`\n rerere: avoid awkward use of `strbuf_attach()`\n strbuf: simplify `strbuf_attach()` usage\n\n strbuf.h | 16 ++++++++--------\n apply.c | 2 +-\n archive.c | 2 +-\n blame.c | 2 +-\n builtin/am.c | 2 +-\n convert.c | 4 ++--\n fast-import.c | 7 ++++---\n imap-send.c | 2 +-\n mailinfo.c | 2 +-\n merge-recursive.c | 2 +-\n path.c | 3 +--\n pretty.c | 4 ++--\n refs/files-backend.c | 2 +-\n rerere.c | 3 ++-\n strbuf.c | 11 +++++++----\n trailer.c | 2 +-\n 16 files changed, 35 insertions(+), 31 deletions(-)\n\n-- \n2.26.1\n\n"},{"id":"395695","messageId":"fa514ce7dad31b8aba0eb693eef9648d509b5334.1587240635.git.martin.agren@gmail.com","threadId":"53252","inReplyTo":"cover.1587240635.git.martin.agren@gmail.com","subject":"[PATCH 1/6] am: use `strbuf_attach()` correctly","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-18T20:18:24Z","receivedAt":"2020-04-18T20:18:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Among other parameters, `strbuf_attach()` takes a length and an amount\nof allocated memory. In strbuf.h, it is documented that the latter \"must\nbe larger than the string length, because the string you pass is\nsupposed to be a NUL-terminated string\".\n\nIn builtin/am.c, we simply pass in the length of the string twice.\n\nMy first assumption was that we'd end up with `alloc == len` and that,\ne.g., a subsequent `strbuf_avail(sb)` would evaluate `sb->alloc\n- sb->len - 1`, resulting in a huge return value, which could be quite\nbad. But luckily, we end up in `strbuf_grow()` where we reallocate a\nlarger buffer, after which we reinstate a '\\0' and everything is fine.\n\nOne might ask if the function was documented incorrectly in dd613e6b87\n(\"Strbuf documentation: document most functions\", 2008-06-04), but on\nthe other hand, one really has to wonder whether it's actually useful to\nbe able to pass in `alloc == len` only to end up performing the\nallocation, copying and freeing which this function very much looks like\nit would keep us from having to do.\n\nPass in a value one greater than the length for the `alloc` parameter.\nThe string has been allocated correctly using the strbuf machinery in\n`read_commit_msg()` and we really do have an extra byte at the end with\na NUL. This means both that the buffer is as large as we claim it to be\nand that the addition is safe.\n\nSigned-off-by: Martin Ågren \n---\n builtin/am.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex e3dfd93c25..e6a9fe8111 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1101,7 +1101,7 @@ static void am_append_signoff(struct am_state *state)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tstrbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);\n+\tstrbuf_attach(&sb, state->msg, state->msg_len, state->msg_len + 1);\n \tappend_signoff(&sb, 0, 0);\n \tstate->msg = strbuf_detach(&sb, &state->msg_len);\n }\n-- \n2.26.1\n\n"},{"id":"395696","messageId":"54f3966f1f3a3e88bcd650cce89b47e650ba005b.1587240635.git.martin.agren@gmail.com","threadId":"53252","inReplyTo":"cover.1587240635.git.martin.agren@gmail.com","subject":"[PATCH 2/6] strbuf_attach: correctly pass in `strlen() + 1` for `alloc`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-18T20:18:25Z","receivedAt":"2020-04-18T20:18:59Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is similar to the previous commit where we corrected a use of\n`strbuf_attach()` to not pass in the same value for `len` and `alloc`.\nThe sites addressed in this commit all use `strlen()`, which makes it\nobvious that we're working with NUL-terminated strings and that we can\nsafely switch to providing `len + 1` for `alloc`.\n\nSigned-off-by: Martin Ågren \n---\n mailinfo.c | 2 +-\n path.c | 2 +-\n refs/files-backend.c | 2 +-\n trailer.c | 3 ++-\n 4 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 742fa376ab..af5d2cad31 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -459,7 +459,7 @@ static int convert_to_utf8(struct mailinfo *mi,\n \t\treturn error(\"cannot convert from %s to %s\",\n \t\t\t charset, mi->metainfo_charset);\n \t}\n-\tstrbuf_attach(line, out, strlen(out), strlen(out));\n+\tstrbuf_attach(line, out, strlen(out), strlen(out) + 1);\n \treturn 0;\n }\n \ndiff --git a/path.c b/path.c\nindex 9bd717c307..5745a71d36 100644\n--- a/path.c\n+++ b/path.c\n@@ -816,7 +816,7 @@ const char *enter_repo(const char *path, int strict)\n \t\t\tif (!newpath)\n \t\t\t\treturn NULL;\n \t\t\tstrbuf_attach(&used_path, newpath, strlen(newpath),\n-\t\t\t\t strlen(newpath));\n+\t\t\t\t strlen(newpath) + 1);\n \t\t}\n \t\tfor (i = 0; suffix[i]; i++) {\n \t\t\tstruct stat st;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 561c33ac8a..76bb2ef490 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1514,7 +1514,7 @@ static int commit_ref(struct ref_lock *lock)\n \t\tsize_t len = strlen(path);\n \t\tstruct strbuf sb_path = STRBUF_INIT;\n \n-\t\tstrbuf_attach(&sb_path, path, len, len);\n+\t\tstrbuf_attach(&sb_path, path, len, len + 1);\n \n \t\t/*\n \t\t * If this fails, commit_lock_file() will also fail\ndiff --git a/trailer.c b/trailer.c\nindex 0c414f2fed..135f71aef1 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1095,7 +1095,8 @@ void trailer_info_get(struct trailer_info *info, const char *str,\n \tfor (ptr = trailer_lines; *ptr; ptr++) {\n \t\tif (last && isspace((*ptr)->buf[0])) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tstrbuf_attach(&sb, *last, strlen(*last), strlen(*last));\n+\t\t\tstrbuf_attach(&sb, *last, strlen(*last),\n+\t\t\t\t strlen(*last) + 1);\n \t\t\tstrbuf_addbuf(&sb, *ptr);\n \t\t\t*last = strbuf_detach(&sb, NULL);\n \t\t\tcontinue;\n-- \n2.26.1\n\n"},{"id":"395697","messageId":"c21fd9b168ae8248954bc94896d283e9857702ef.1587240635.git.martin.agren@gmail.com","threadId":"53252","inReplyTo":"cover.1587240635.git.martin.agren@gmail.com","subject":"[PATCH 3/6] strbuf: use `strbuf_attach()` correctly","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-18T20:18:26Z","receivedAt":"2020-04-18T20:19:08Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to earlier commits, don't pass in the same value for `len` and\n`alloc` to `strbuf_attach()`. This string comes from\n`reencode_string_iconv()` which ensures we have an extra byte at the\nend.\n\nSigned-off-by: Martin Ågren \n---\n strbuf.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex bb0065ccaf..deb338412e 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -152,7 +152,7 @@ int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)\n \tif (!out)\n \t\treturn -1;\n \n-\tstrbuf_attach(sb, out, len, len);\n+\tstrbuf_attach(sb, out, len, len + 1);\n \treturn 0;\n }\n \n-- \n2.26.1\n\n"},{"id":"395698","messageId":"5db92b51c0363694c72b2de0c841449fa4e03f28.1587240635.git.martin.agren@gmail.com","threadId":"53252","inReplyTo":"cover.1587240635.git.martin.agren@gmail.com","subject":"[PATCH 4/6] fast-import: avoid awkward use of `strbuf_attach()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-18T20:18:27Z","receivedAt":"2020-04-18T20:19:14Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"As explained in an earlier commit, per the documentation of\n`strbuf_attach()`, it is incorrect to pass in the same value for `alloc`\nas we do for `len`. But, and this was also explained earlier, doing so\nis still ok-ish because we'll end up allocating a large enough buffer\nunder the hood.\n\nBut then one really has to wonder whether\n\n strbuf_attach(&sb, buf, size, size);\n\nis any better than\n\n strbuf_reset(&sb);\n strbuf_add(&sb, buf, size);\n free(buf);\n\nThe latter is certainly a lot less subtle about what is going on, and if\nwe're lucky, the strbuf's allocated buffer is large enough that we won't\neven need to allocate. So let's change to this more explicit form.\n\nIn short, this commit should not make things any worse.\n\nNearby commits are changing other callsites to pass in a larger 'alloc`\nparameter. Maybe that's safe here, too -- I admit I don't quite follow\nwhere this memory comes from. In the future, we could possibly switch\nback to `strbuf_attach()` here after looking into the allocations in\nmore detail. The immediate reason for this commit is that we want to\nsimplify the usage of `strbuf_attach()`, and we won't be able to pass in\n\"size, size\" any more.\n\nSigned-off-by: Martin Ågren \n---\n fast-import.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 202dda11a6..7fd501c5cf 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2946,10 +2946,11 @@ static void cat_blob(struct object_entry *oe, struct object_id *oid)\n \tcat_blob_write(\"\\n\", 1);\n \tif (oe && oe->pack_id == pack_id) {\n \t\tlast_blob.offset = oe->idx.offset;\n-\t\tstrbuf_attach(&last_blob.data, buf, size, size);\n+\t\tstrbuf_reset(&last_blob.data);\n+\t\tstrbuf_add(&last_blob.data, buf, size);\n \t\tlast_blob.depth = oe->depth;\n-\t} else\n-\t\tfree(buf);\n+\t}\n+\tfree(buf);\n }\n \n static void parse_get_mark(const char *p)\n-- \n2.26.1\n\n"},{"id":"395699","messageId":"e9a8e5b061fc07b6e6450b028f97378044f9c2cd.1587240635.git.martin.agren@gmail.com","threadId":"53252","inReplyTo":"cover.1587240635.git.martin.agren@gmail.com","subject":"[PATCH 5/6] rerere: avoid awkward use of `strbuf_attach()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-18T20:18:28Z","receivedAt":"2020-04-18T20:19:17Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to the previous commit, avoid passing `strbuf_attach()` the same\nvalue for `len` and `alloc` by switching to `strbuf_add()`. This avoids\na subtle allocate+copy+free dance in favour of a much more obvious one.\n\nUnlike in the previous commit, for this site, I know that it's not safe\nto pass in \"len, len + 1\". Trying that makes `strbuf_attach()` write a\ntrailing '\\0' at `result.ptr[len]`. Running our test suite with\nAddressSanitizer will spot an out-of-bounds write due to this.\n\nSigned-off-by: Martin Ågren \n---\n rerere.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 9281131a9f..c3e3714824 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -1007,7 +1007,8 @@ static int handle_cache(struct index_state *istate,\n \telse\n \t\tio.io.output = NULL;\n \tstrbuf_init(&io.input, 0);\n-\tstrbuf_attach(&io.input, result.ptr, result.size, result.size);\n+\tstrbuf_add(&io.input, result.ptr, result.size);\n+\tfree(result.ptr);\n \n \t/*\n \t * Grab the conflict ID and optionally write the original\n-- \n2.26.1\n\n"},{"id":"395700","messageId":"ff6febad652f3e31642f3a39fd501c4c31f93a38.1587240635.git.martin.agren@gmail.com","threadId":"53252","inReplyTo":"cover.1587240635.git.martin.agren@gmail.com","subject":"[PATCH 6/6] strbuf: simplify `strbuf_attach()` usage","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-18T20:18:29Z","receivedAt":"2020-04-18T20:19:30Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The last few commits have fixed a decent number of call sites that pass\nin the same value for `alloc` and `len` when attaching a string to a\nstrbuf, despite the documentation being quite clear about this not being\nok. As a result, all users currently pass in `len + 1`.\n\nBecause this interface was used incorrectly (albeit not necessarily\ninsecurely) before, simplify it by making `strbuf_attach()` simply assume that\nthe allocated length is exactly `len + 1`. If and when we do gain one or\ntwo call sites that know better, we can either keep on using this\npessimistic assumption and it might not actually hurt much, or we could\ngive them a dedicated function similar to the current more flexible (and\nproven error-prone) `strbuf_attach()`.\n\nThis means we can drop the call to `strbuf_grow()`. It really only\nserved to do the alloc+copy+free dance that we were supposedly avoiding.\n\nThat said, we should consider the edge case of `buf == NULL`. Let's\ncheck for this and BUG. Even for a zero-length string, you need\nsomewhere to stick the trailing NUL.\n\n(Also: If someone used to pass `alloc == len` for the case `len == 0`,\nwe used to end up leaking `buf` on the assumption that we were using\n`strbuf_slopbuf`. Not the biggest leak ever, but still.)\n\nFinally, one might wonder if we could go further and make\n`strbuf_attach()` call `strlen()`. Some call sites wouldn't mind, but\nothers do have the length already and we might as well use it. Those\nthat don't have the length will figure it out as they call this\nfunction. Before this commit, they'd do two calls to `strlen()` so they\neither didn't care much or they trusted the compiler to optimize for\nthem. They're not any worse off now.\n\nSigned-off-by: Martin Ågren \n---\n strbuf.h | 16 ++++++++--------\n apply.c | 2 +-\n archive.c | 2 +-\n blame.c | 2 +-\n builtin/am.c | 2 +-\n convert.c | 4 ++--\n imap-send.c | 2 +-\n mailinfo.c | 2 +-\n merge-recursive.c | 2 +-\n path.c | 3 +--\n pretty.c | 4 ++--\n refs/files-backend.c | 2 +-\n strbuf.c | 11 +++++++----\n trailer.c | 3 +--\n 14 files changed, 29 insertions(+), 28 deletions(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex ce8e49c0b2..72dcb43795 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -110,14 +110,14 @@ void strbuf_release(struct strbuf *sb);\n char *strbuf_detach(struct strbuf *sb, size_t *sz);\n \n /**\n- * Attach a string to a buffer. You should specify the string to attach,\n- * the current length of the string and the amount of allocated memory.\n- * The amount must be larger than the string length, because the string you\n- * pass is supposed to be a NUL-terminated string. This string _must_ be\n- * malloc()ed, and after attaching, the pointer cannot be relied upon\n- * anymore, and neither be free()d directly.\n- */\n-void strbuf_attach(struct strbuf *sb, void *str, size_t len, size_t mem);\n+ * Attach a string to a buffer. You should specify the string to attach\n+ * and its length. The amount of allocated memory will be assumed to be\n+ * one greater than the length. The string you pass is supposed to be a\n+ * NUL-terminated string. This string _must_ be malloc()ed, and after\n+ * attaching, the pointer cannot be relied upon anymore, nor should it\n+ * be free()d directly.\n+ */\n+void strbuf_attach(struct strbuf *sb, void *str, size_t len);\n \n /**\n * Swap the contents of two string buffers.\ndiff --git a/apply.c b/apply.c\nindex 144c19aaca..678cee5535 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3251,7 +3251,7 @@ static int read_blob_object(struct strbuf *buf, const struct object_id *oid, uns\n \t\tif (!result)\n \t\t\treturn -1;\n \t\t/* XXX read_sha1_file NUL-terminates */\n-\t\tstrbuf_attach(buf, result, sz, sz + 1);\n+\t\tstrbuf_attach(buf, result, sz);\n \t}\n \treturn 0;\n }\ndiff --git a/archive.c b/archive.c\nindex fb39706120..bef66f8574 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -89,7 +89,7 @@ void *object_file_to_archive(const struct archiver_args *args,\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tsize_t size = 0;\n \n-\t\tstrbuf_attach(&buf, buffer, *sizep, *sizep + 1);\n+\t\tstrbuf_attach(&buf, buffer, *sizep);\n \t\tconvert_to_working_tree(args->repo->index, path, buf.buf, buf.len, &buf, &meta);\n \t\tif (commit)\n \t\t\tformat_subst(commit, buf.buf, buf.len, &buf);\ndiff --git a/blame.c b/blame.c\nindex 29770e5c81..64218c2c6e 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -241,7 +241,7 @@ static struct commit *fake_working_tree_commit(struct repository *r,\n \t\tcase S_IFREG:\n \t\t\tif (opt->flags.allow_textconv &&\n \t\t\t textconv_object(r, read_from, mode, &null_oid, 0, &buf_ptr, &buf_len))\n-\t\t\t\tstrbuf_attach(&buf, buf_ptr, buf_len, buf_len + 1);\n+\t\t\t\tstrbuf_attach(&buf, buf_ptr, buf_len);\n \t\t\telse if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)\n \t\t\t\tdie_errno(\"cannot open or read '%s'\", read_from);\n \t\t\tbreak;\ndiff --git a/builtin/am.c b/builtin/am.c\nindex e6a9fe8111..0986ea4af2 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1101,7 +1101,7 @@ static void am_append_signoff(struct am_state *state)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tstrbuf_attach(&sb, state->msg, state->msg_len, state->msg_len + 1);\n+\tstrbuf_attach(&sb, state->msg, state->msg_len);\n \tappend_signoff(&sb, 0, 0);\n \tstate->msg = strbuf_detach(&sb, &state->msg_len);\n }\ndiff --git a/convert.c b/convert.c\nindex 5aa87d45e3..ee1941c662 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -467,7 +467,7 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,\n \t\tfree(re_src);\n \t}\n \n-\tstrbuf_attach(buf, dst, dst_len, dst_len + 1);\n+\tstrbuf_attach(buf, dst, dst_len);\n \treturn 1;\n }\n \n@@ -492,7 +492,7 @@ static int encode_to_worktree(const char *path, const char *src, size_t src_len,\n \t\treturn 0;\n \t}\n \n-\tstrbuf_attach(buf, dst, dst_len, dst_len + 1);\n+\tstrbuf_attach(buf, dst, dst_len);\n \treturn 1;\n }\n \ndiff --git a/imap-send.c b/imap-send.c\nindex 6c54d8c29d..b36b9ff62d 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1212,7 +1212,7 @@ static void lf_to_crlf(struct strbuf *msg)\n \t\t\tnew_msg[j++] = '\\r';\n \t\tlastc = new_msg[j++] = msg->buf[i];\n \t}\n-\tstrbuf_attach(msg, new_msg, j, j + 1);\n+\tstrbuf_attach(msg, new_msg, j);\n }\n \n /*\ndiff --git a/mailinfo.c b/mailinfo.c\nindex af5d2cad31..cdcb6af8c1 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -459,7 +459,7 @@ static int convert_to_utf8(struct mailinfo *mi,\n \t\treturn error(\"cannot convert from %s to %s\",\n \t\t\t charset, mi->metainfo_charset);\n \t}\n-\tstrbuf_attach(line, out, strlen(out), strlen(out) + 1);\n+\tstrbuf_attach(line, out, strlen(out));\n \treturn 0;\n }\n \ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d92e2acf1e..5cdd797ada 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2963,7 +2963,7 @@ static int read_oid_strbuf(struct merge_options *opt,\n \t\tfree(buf);\n \t\treturn err(opt, _(\"object %s is not a blob\"), oid_to_hex(oid));\n \t}\n-\tstrbuf_attach(dst, buf, size, size + 1);\n+\tstrbuf_attach(dst, buf, size);\n \treturn 0;\n }\n \ndiff --git a/path.c b/path.c\nindex 5745a71d36..5474bbd079 100644\n--- a/path.c\n+++ b/path.c\n@@ -815,8 +815,7 @@ const char *enter_repo(const char *path, int strict)\n \t\t\tchar *newpath = expand_user_path(used_path.buf, 0);\n \t\t\tif (!newpath)\n \t\t\t\treturn NULL;\n-\t\t\tstrbuf_attach(&used_path, newpath, strlen(newpath),\n-\t\t\t\t strlen(newpath) + 1);\n+\t\t\tstrbuf_attach(&used_path, newpath, strlen(newpath));\n \t\t}\n \t\tfor (i = 0; suffix[i]; i++) {\n \t\t\tstruct stat st;\ndiff --git a/pretty.c b/pretty.c\nindex 28afc701b6..e1abe8ac89 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -590,7 +590,7 @@ static char *replace_encoding_header(char *buf, const char *encoding)\n \t\treturn buf; /* should not happen but be defensive */\n \tlen = cp + 1 - (buf + start);\n \n-\tstrbuf_attach(&tmp, buf, strlen(buf), strlen(buf) + 1);\n+\tstrbuf_attach(&tmp, buf, strlen(buf));\n \tif (is_encoding_utf8(encoding)) {\n \t\t/* we have re-coded to UTF-8; drop the header */\n \t\tstrbuf_remove(&tmp, start, len);\n@@ -1687,7 +1687,7 @@ void repo_format_commit_message(struct repository *r,\n \t\tchar *out = reencode_string_len(sb->buf, sb->len,\n \t\t\t\t\t\toutput_enc, utf8, &outsz);\n \t\tif (out)\n-\t\t\tstrbuf_attach(sb, out, outsz, outsz + 1);\n+\t\t\tstrbuf_attach(sb, out, outsz);\n \t}\n \n \tfree(context.commit_encoding);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 76bb2ef490..4ce9f2cda4 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1514,7 +1514,7 @@ static int commit_ref(struct ref_lock *lock)\n \t\tsize_t len = strlen(path);\n \t\tstruct strbuf sb_path = STRBUF_INIT;\n \n-\t\tstrbuf_attach(&sb_path, path, len, len + 1);\n+\t\tstrbuf_attach(&sb_path, path, len);\n \n \t\t/*\n \t\t * If this fails, commit_lock_file() will also fail\ndiff --git a/strbuf.c b/strbuf.c\nindex deb338412e..e74253f91d 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -77,13 +77,16 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)\n \treturn res;\n }\n \n-void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)\n+void strbuf_attach(struct strbuf *sb, void *buf, size_t len)\n {\n+\tif (!buf)\n+\t\tBUG(\"NULL-buffer in strbuf_attach\");\n+\tif (unsigned_add_overflows(len, 1))\n+\t\tdie(\"you want to use way too much memory\");\n \tstrbuf_release(sb);\n \tsb->buf = buf;\n \tsb->len = len;\n-\tsb->alloc = alloc;\n-\tstrbuf_grow(sb, 0);\n+\tsb->alloc = len + 1;\n \tsb->buf[sb->len] = '\\0';\n }\n \n@@ -152,7 +155,7 @@ int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)\n \tif (!out)\n \t\treturn -1;\n \n-\tstrbuf_attach(sb, out, len, len + 1);\n+\tstrbuf_attach(sb, out, len);\n \treturn 0;\n }\n \ndiff --git a/trailer.c b/trailer.c\nindex 135f71aef1..30bc622723 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1095,8 +1095,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,\n \tfor (ptr = trailer_lines; *ptr; ptr++) {\n \t\tif (last && isspace((*ptr)->buf[0])) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tstrbuf_attach(&sb, *last, strlen(*last),\n-\t\t\t\t strlen(*last) + 1);\n+\t\t\tstrbuf_attach(&sb, *last, strlen(*last));\n \t\t\tstrbuf_addbuf(&sb, *ptr);\n \t\t\t*last = strbuf_detach(&sb, NULL);\n \t\t\tcontinue;\n-- \n2.26.1\n\n"},{"id":"395701","messageId":"xmqqk12ctmm1.fsf@gitster.c.googlers.com","threadId":"53252","inReplyTo":"CAN0heSppn6BBX4V1T1qgKc4XP+8i6qbcEqd1_3NqWQtZJLaJww@mail.gmail.com","subject":"Re: [PATCH] mailinfo.c::convert_to_utf8: reuse strlen info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-18T23:12:06Z","receivedAt":"2020-04-18T23:12:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> This is equivalent as long as `line->len` is equal to\n> `strlen(line->buf)`, which it will be (should be) because it's a\n> strbuf. Ok.\n\nFor the guarantee to hold true, line->buf[0..line->len] should not\nhave any '\\0' byte in it. \n\nThis helper has two callers, but in either case, it needs to be\nprepared to work on output of decode_[bq]_segment(). Is there code\nanywhere that guarntees that the decoding won't stuff '\\0' in the\nline?\n"},{"id":"395702","messageId":"20200419024805.GC9169@danh.dev","threadId":"53252","inReplyTo":"xmqqk12ctmm1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] mailinfo.c::convert_to_utf8: reuse strlen info","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-19T02:48:05Z","receivedAt":"2020-04-19T02:48:11Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-04-18 16:12:06-0700, Junio C Hamano wrote:\n> Martin Ågren writes:\n> \n> > This is equivalent as long as `line->len` is equal to\n> > `strlen(line->buf)`, which it will be (should be) because it's a\n> > strbuf. Ok.\n> \n> For the guarantee to hold true, line->buf[0..line->len] should not\n> have any '\\0' byte in it. \n> \n> This helper has two callers, but in either case, it needs to be\n> prepared to work on output of decode_[bq]_segment(). Is there code\n> anywhere that guarntees that the decoding won't stuff '\\0' in the\n> line?\n\n- First caller: `decode_header`, we don't have such guarantee,\n The old code will discard everything after first NUL characters.\n I'm _not_ really familiar with email standard, does email allow\n UTF-16 (albeit in [bq]-encoded) in header? If yes, the current code\n is likely disallow it. The new code accidentally, accept\n [bq]-encoded-utf-16 header and reencode to utf-8 for commit.\n Yes, it's very likely only UTF-8, ISO-8859-1, some variant of\n ISO-2022 is used nowaday in email.\n *If* we would like to not exclude UTF-16, the new question is should\n we trust the length of newly converted utf-8 string.\n\n- Second caller: `handle_commit_msg`: everything get interesting from\n here.\n\nGet back to the question of trusting the length of newly converted\nutf-8 string.\n\nI _think_ we should,\nbecause I _think_ there shouldn't be any discrimination against any\nencoding (be it utf-8, ISO-8859-1, etc...),\nthe current code allows NUL character in utf-8 [bq]-encoded string\nin this function (early return) and its caller,\nand report an error later:\n\n\terror: a NUL byte in commit log message not allowed.\n\nmeanwhile, if the email was sent in other encoding, the current code \ndiscards everything after NUL in that line,\nthus silently accept broken commit message.\n\nAttached is the faulty patch in ISO-8859-1, which was used to\ndemonstrate my words.\nThe current code will make a commit with only \"Áb\" in the body,\nwhile the new code rightly claims we have a faulty email.\n\n-- \nDanh\n\n\nFrom e27fb59f5a4854efc0badd902673fd636bcab52e Mon Sep 17 00:00:00 2001\nFrom: =?ISO-8859-1?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?=\n \nDate: Sun, 19 Apr 2020 09:31:22 +0700\nSubject: [PATCH] =?ISO-8859-1?q?=C4=CB=D1=CF=D6?=\nMIME-Version: 1.0\nContent-Type: text/plain; charset=ISO-8859-1\nContent-Transfer-Encoding: 8bit\n\n�b�d�fg\n\n---\n afile | 0\n 1 file changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 afile\n\ndiff --git a/afile b/afile\nnew file mode 100644\nindex 0000000000..e69de29bb2\n-- \n2.26.1.301.g55bc3eb7cb\n\n"},{"id":"395704","messageId":"CAN0heSoj9fwpx5WbME+Oew-7NLfBfQCfAKissix+wnFk0UW5uw@mail.gmail.com","threadId":"53252","inReplyTo":"20200419024805.GC9169@danh.dev","subject":"Re: [PATCH] mailinfo.c::convert_to_utf8: reuse strlen info","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-19T04:34:10Z","receivedAt":"2020-04-19T04:34:32Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sun, 19 Apr 2020 at 04:48, Danh Doan wrote:\n>\n> On 2020-04-18 16:12:06-0700, Junio C Hamano wrote:\n> > Martin Ågren writes:\n> >\n> > > This is equivalent as long as `line->len` is equal to\n> > > `strlen(line->buf)`, which it will be (should be) because it's a\n> > > strbuf. Ok.\n> >\n> > For the guarantee to hold true, line->buf[0..line->len] should not\n> > have any '\\0' byte in it.\n\nYes. I sort of assumed it shouldn't (\"because strbuf\"), but it's a good\nquestion. Especially considering this is about various encodings..\n\nThis assumption that \"strlen is length so the conversion is a no-op\"\ncould potentially be broken both for input (line->len) and output\n(out_len).\n\n> > This helper has two callers, but in either case, it needs to be\n> > prepared to work on output of decode_[bq]_segment(). Is there code\n> > anywhere that guarntees that the decoding won't stuff '\\0' in the\n> > line?\n>\n> the current code allows NUL character in utf-8 [bq]-encoded string\n> in this function (early return) and its caller,\n> and report an error later:\n>\n> error: a NUL byte in commit log message not allowed.\n>\n> meanwhile, if the email was sent in other encoding, the current code\n> discards everything after NUL in that line,\n> thus silently accept broken commit message.\n\nMy knee-jerk reaction to Junio's question was along the same line:\nsurely if we could have a NUL in there, the current `strlen()` would use\nit as an excuse to silently truncate the string, either before\nprocessing or afterwards. Thanks for looking into that more.\n\n> Attached is the faulty patch in ISO-8859-1, which was used to\n> demonstrate my words.\n> The current code will make a commit with only \"Áb\" in the body,\n> while the new code rightly claims we have a faulty email.\n\nGood find.\n\nMartin\n"},{"id":"395705","messageId":"CAN0heSrzVfb1=xUReZidBVhE427r7MRPtZ_RDOorXESq4r-wwQ@mail.gmail.com","threadId":"53252","inReplyTo":"cover.1587240635.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/6] strbuf: simplify `strbuf_attach()` usage","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-19T04:44:06Z","receivedAt":"2020-04-19T04:44:22Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sat, 18 Apr 2020 at 22:18, Martin Ågren wrote:\n>\n> On Sat, 18 Apr 2020 at 21:56, Martin Ågren wrote:\n> > > - strbuf_attach(line, out, strlen(out), strlen(out));\n> > > + strbuf_attach(line, out, out_len, out_len);\n> >\n> > This conversion is ok as such. I wondered why we pass in the same\n> > value twice (before and after this patch). Turns out this usage is wrong\n> > (as per the documentation in strbuf.h) but safe (as per my understanding\n> > of the implementation in strbuf.c). I'll follow up with a series that\n> > fell out of that investigation.\n>\n> Here's that series. It could go in parallel to Danh's, or one could go\n> on top of the other. Any of those would be ok with me.\n\nI realize now that some of the reasoning in this series is incorrect.\n`strbuf_grow()` will use `ALLOC_GROW` meaning we might reallocate\ncheaply rather than doing an entire allocate+copy+free cycle.\n\nStill, there is some buggyness in the usage because we might reallocate\n(with a 50% overhead) even when we shouldn't really need to.\n\nI'll resubmit this after tackling it from a different angle: I'll\nintroduce `strbuf_attachstr()`, simplifying and robustifyng most users.\nThen I'll switch a few incorrect users of `strbuf_attach()` to\n`strbuf_attachstr()`. Then a small number of users will continue using\n`strbuf_attach()` with `alloc == len` for $reasons.\n\n> The summary is that this function takes `len` and `alloc`, where the\n> latter must be greater than the former, yet several callers use the same\n> value for both. I first thought this could cause quite hairy problems\n> such as writing outside of allocated memory, but it doesn't look that\n> way. See the patches for more information.\n>\n> An alternative to the approach taken here would be to introduce\n> `strbuf_attachstr()` and convert most existing users, then convert the\n> few remaining ones to use the new function or to move in another\n> direction. But the new name is a downer -- what else would you attach to\n> a strbuf if not a string?\n\nSo this is what I'll do instead. The reasoning is, you can attach a\nstring (a NUL-terminated buffer) or a non-string (non-NUL-terminated\nbuffer).\n\nMartin\n"},{"id":"395707","messageId":"xmqq8sist501.fsf@gitster.c.googlers.com","threadId":"53252","inReplyTo":"CAN0heSoj9fwpx5WbME+Oew-7NLfBfQCfAKissix+wnFk0UW5uw@mail.gmail.com","subject":"Re: [PATCH] mailinfo.c::convert_to_utf8: reuse strlen info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-19T05:32:30Z","receivedAt":"2020-04-19T05:32:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> My knee-jerk reaction to Junio's question was along the same line:\n> surely if we could have a NUL in there, the current `strlen()` would use\n> it as an excuse to silently truncate the string, either before\n> processing or afterwards. Thanks for looking into that more.\n\nExactly.\n\nThe change introduces a behaviour change, and we should describe it\nin the log message to help future developers know that we did this\nchange, knowingly that we are changing the behaviour, and believing\nthat the new behaviour is a better one.\n\nThanks.\n"},{"id":"395712","messageId":"cover.1587289680.git.congdanhqx@gmail.com","threadId":"53252","inReplyTo":"20200418035449.9450-1-congdanhqx@gmail.com","subject":"[PATCH v2 0/3] mailinfo: disallow and complains about NUL character","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-19T11:00:39Z","receivedAt":"2020-04-19T11:00:53Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"\nOn 2020-04-18 22:32:30-0700, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n> > My knee-jerk reaction to Junio's question was along the same line:\n> > surely if we could have a NUL in there, the current `strlen()` would use\n> > it as an excuse to silently truncate the string, either before\n> > processing or afterwards. Thanks for looking into that more.\n>\n> Exactly.\n>\n> The change introduces a behaviour change, and we should describe it\n> in the log message to help future developers know that we did this\n> change, knowingly that we are changing the behaviour, and believing\n> that the new behaviour is a better one.\n\nWell, I intended to do something else first (because of Thunderbird started\nwith HTML email and replace space with non-breaking-space).\n\nI made this change without thinking that much :( And I weren't sure if this\nchange is well received.\n\nI've rephased the commit message, added some test, and disallow another NUL\ncharacters mailinfo.\n\n-------------------8<------------------------\n\nAs of current state, Git disallow NUL character in commit message.\n\nThis indirectly disallow NUL in the body of commit message that sent by email\nin UTF-8 encoding.\n\nIn those below cases:\n\n* NUL character in header field (be it Subject, Author, Email, etc...)\n* NUL in the body of commit message that originally sent in other than UTF-8\n encoding\n\nGit silently accepts them, albeit, truncate at the NUL character.\n\nThose email is clearly not generated by Git, they must be crafted.\n\nComplains loudly about those NUL characters.\n\nĐoàn Trần Công Danh (3):\n t4254: merge 2 steps of a single test\n mailinfo.c::convert_to_utf8: reuse strlen info\n mailinfo: disallow NUL character in mail's header\n\n mailinfo.c | 11 +++++++--\n t/t4254-am-corrupt.sh | 52 +++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 57 insertions(+), 6 deletions(-)\n\nRange-diff against v1:\n-: ---------- > 1: d1bc31e692 t4254: merge 2 steps of a single test\n1: 9ce4b7c350 ! 2: e3e542d388 mailinfo.c::convert_to_utf8: reuse strlen info\n @@ Commit message\n We're passing buffer from strbuf to reencode_string,\n which will call strlen(3) on that buffer,\n and discard the length of newly created buffer.\n -\n Then, we compute the length of the return buffer to attach to strbuf.\n \n + During this process, we introduce a discrimination between mail\n + originally written in utf-8 and other encoding.\n +\n + * if the email was written in utf-8, we leave it as is. If there is\n + a NUL character in that line, we complains loudly:\n +\n + error: a NUL byte in commit log message not allowed.\n +\n + * if the email was written in other encoding, we truncate the data as\n + the NUL character in that line, then we used the truncated line for\n + the metadata.\n +\n We can do better by reusing all the available information,\n and call the underlying lower level function that will be called\n - indirectly by reencode_string.\n + indirectly by reencode_string. By doing this, we will also postpone\n + the NUL character processing to the commit step, which will\n + complains about the faulty metadata.\n \n Signed-off-by: Đoàn Trần Công Danh \n \n @@ mailinfo.c: static int convert_to_utf8(struct mailinfo *mi,\n \treturn 0;\n }\n \n +\n + ## t/t4254-am-corrupt.sh ##\n +@@\n + test_description='git am with corrupt input'\n + . ./test-lib.sh\n + \n ++write_nul_patch() {\n ++\tspace=' '\n ++\tqNUL=\n ++\tcase \"$1\" in\n ++\t\tsubject) qNUL='=00' ;;\n ++\tesac\n ++\tcat <<-EOF\n ++\tFrom ec7364544f690c560304f5a5de9428ea3b978b26 Mon Sep 17 00:00:00 2001\n ++\tFrom: A U Thor \n ++\tDate: Sun, 19 Apr 2020 13:42:07 +0700\n ++\tSubject: [PATCH] =?ISO-8859-1?q?=C4=CB${qNUL}=D1=CF=D6?=\n ++\tMIME-Version: 1.0\n ++\tContent-Type: text/plain; charset=ISO-8859-1\n ++\tContent-Transfer-Encoding: 8bit\n ++\n ++\tEOF\n ++\tif test \"$1\" = body\n ++\tthen\n ++\t\tprintf \"%s\\0%s\\n\" abc def\n ++\tfi\n ++\tcat <<-\\EOF\n ++\t---\n ++\tdiff --git a/afile b/afile\n ++\tnew file mode 100644\n ++\tindex 0000000000..e69de29bb2\n ++\t--$space\n ++\t2.26.1\n ++\tEOF\n ++}\n ++\n + test_expect_success setup '\n + \t# Note the missing \"+++\" line:\n + \tcat >bad-patch.diff <<-\\EOF &&\n +@@ t/t4254-am-corrupt.sh: test_expect_success 'try to apply corrupted patch' '\n + \ttest_i18ncmp expected actual\n + '\n + \n ++test_expect_success \"NUL in commit message's body\" '\n ++\ttest_when_finished \"git am --abort\" &&\n ++\twrite_nul_patch body >body.patch &&\n ++\ttest_must_fail git am body.patch 2>err &&\n ++\tgrep \"a NUL byte in commit log message not allowed\" err\n ++'\n ++\n ++test_expect_failure \"NUL in commit message's header\" '\n ++\ttest_when_finished \"git am --abort\" &&\n ++\twrite_nul_patch subject >subject.patch &&\n ++\ttest_must_fail git am subject.patch 2>err &&\n ++\tgrep \"a NUL byte in Subject is not allowed\" err\n ++'\n ++\n + test_done\n-: ---------- > 3: cb96947b36 mailinfo: disallow NUL character in mail's header\n-- \n2.26.1.301.g55bc3eb7cb\n\n"},{"id":"395713","messageId":"d1bc31e692d08d73bc577f737b0190e163440ee9.1587289680.git.congdanhqx@gmail.com","threadId":"53252","inReplyTo":"cover.1587289680.git.congdanhqx@gmail.com","subject":"[PATCH v2 1/3] t4254: merge 2 steps of a single test","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-19T11:00:40Z","receivedAt":"2020-04-19T11:00:55Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"While we are at it, make sure we run a clean up after testing.\nIn a later patch, we will test for more corrupted patch.\n\nSigned-off-by: Đoàn Trần Công Danh \n---\n t/t4254-am-corrupt.sh | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\nindex fd3bdbfe2c..ddd35498db 100755\n--- a/t/t4254-am-corrupt.sh\n+++ b/t/t4254-am-corrupt.sh\n@@ -25,10 +25,8 @@ test_expect_success setup '\n # fatal: unable to write file '(null)' mode 100644: Bad address\n # Also, it had the unwanted side-effect of deleting f.\n test_expect_success 'try to apply corrupted patch' '\n-\ttest_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual\n-'\n-\n-test_expect_success 'compare diagnostic; ensure file is still here' '\n+\ttest_when_finished \"git am --abort\" &&\n+\ttest_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual &&\n \techo \"error: git diff header lacks filename information (line 4)\" >expected &&\n \ttest_path_is_file f &&\n \ttest_i18ncmp expected actual\n-- \n2.26.1.301.g55bc3eb7cb\n\n"},{"id":"395714","messageId":"cb96947b36b8ab314f5692daee6f627273ca66d3.1587289680.git.congdanhqx@gmail.com","threadId":"53252","inReplyTo":"cover.1587289680.git.congdanhqx@gmail.com","subject":"[PATCH v2 3/3] mailinfo: disallow NUL character in mail's header","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-19T11:00:42Z","receivedAt":"2020-04-19T11:00:58Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Signed-off-by: Đoàn Trần Công Danh \n---\n mailinfo.c | 5 +++++\n t/t4254-am-corrupt.sh | 4 +++-\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 0e9911df6d..c31991e621 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -1138,6 +1138,11 @@ static void handle_info(struct mailinfo *mi)\n \t\telse\n \t\t\tcontinue;\n \n+\t\tif (memchr(hdr->buf, '\\0', hdr->len)) {\n+\t\t\terror(\"a NUL byte in %s is not allowed.\", header[i]);\n+\t\t\tmi->input_error = -1;\n+\t\t}\n+\n \t\tif (!strcmp(header[i], \"Subject\")) {\n \t\t\tif (!mi->keep_subject) {\n \t\t\t\tcleanup_subject(mi, hdr);\ndiff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\nindex 98cda32d0a..d9d1ac6c7d 100755\n--- a/t/t4254-am-corrupt.sh\n+++ b/t/t4254-am-corrupt.sh\n@@ -69,9 +69,11 @@ test_expect_success \"NUL in commit message's body\" '\n \tgrep \"a NUL byte in commit log message not allowed\" err\n '\n \n-test_expect_failure \"NUL in commit message's header\" '\n+test_expect_success \"NUL in commit message's header\" '\n \ttest_when_finished \"git am --abort\" &&\n \twrite_nul_patch subject >subject.patch &&\n+\ttest_must_fail git mailinfo msg patch err &&\n+\tgrep \"a NUL byte in Subject is not allowed\" err &&\n \ttest_must_fail git am subject.patch 2>err &&\n \tgrep \"a NUL byte in Subject is not allowed\" err\n '\n-- \n2.26.1.301.g55bc3eb7cb\n\n"},{"id":"395715","messageId":"e3e542d38818b762f8d6d6b50bc42e01b070772b.1587289680.git.congdanhqx@gmail.com","threadId":"53252","inReplyTo":"cover.1587289680.git.congdanhqx@gmail.com","subject":"[PATCH v2 2/3] mailinfo.c::convert_to_utf8: reuse strlen info","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-19T11:00:41Z","receivedAt":"2020-04-19T11:00:59Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"We're passing buffer from strbuf to reencode_string,\nwhich will call strlen(3) on that buffer,\nand discard the length of newly created buffer.\nThen, we compute the length of the return buffer to attach to strbuf.\n\nDuring this process, we introduce a discrimination between mail\noriginally written in utf-8 and other encoding.\n\n* if the email was written in utf-8, we leave it as is. If there is\n a NUL character in that line, we complains loudly:\n\n \terror: a NUL byte in commit log message not allowed.\n\n* if the email was written in other encoding, we truncate the data as\n the NUL character in that line, then we used the truncated line for\n the metadata.\n\nWe can do better by reusing all the available information,\nand call the underlying lower level function that will be called\nindirectly by reencode_string. By doing this, we will also postpone\nthe NUL character processing to the commit step, which will\ncomplains about the faulty metadata.\n\nSigned-off-by: Đoàn Trần Công Danh \n---\n mailinfo.c | 6 ++++--\n t/t4254-am-corrupt.sh | 44 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 2 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 742fa376ab..0e9911df6d 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -447,19 +447,21 @@ static int convert_to_utf8(struct mailinfo *mi,\n \t\t\t struct strbuf *line, const char *charset)\n {\n \tchar *out;\n+\tsize_t out_len;\n \n \tif (!mi->metainfo_charset || !charset || !*charset)\n \t\treturn 0;\n \n \tif (same_encoding(mi->metainfo_charset, charset))\n \t\treturn 0;\n-\tout = reencode_string(line->buf, mi->metainfo_charset, charset);\n+\tout = reencode_string_len(line->buf, line->len,\n+\t\t\t\t mi->metainfo_charset, charset, &out_len);\n \tif (!out) {\n \t\tmi->input_error = -1;\n \t\treturn error(\"cannot convert from %s to %s\",\n \t\t\t charset, mi->metainfo_charset);\n \t}\n-\tstrbuf_attach(line, out, strlen(out), strlen(out));\n+\tstrbuf_attach(line, out, out_len, out_len);\n \treturn 0;\n }\n \ndiff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\nindex ddd35498db..98cda32d0a 100755\n--- a/t/t4254-am-corrupt.sh\n+++ b/t/t4254-am-corrupt.sh\n@@ -3,6 +3,36 @@\n test_description='git am with corrupt input'\n . ./test-lib.sh\n \n+write_nul_patch() {\n+\tspace=' '\n+\tqNUL=\n+\tcase \"$1\" in\n+\t\tsubject) qNUL='=00' ;;\n+\tesac\n+\tcat <<-EOF\n+\tFrom ec7364544f690c560304f5a5de9428ea3b978b26 Mon Sep 17 00:00:00 2001\n+\tFrom: A U Thor \n+\tDate: Sun, 19 Apr 2020 13:42:07 +0700\n+\tSubject: [PATCH] =?ISO-8859-1?q?=C4=CB${qNUL}=D1=CF=D6?=\n+\tMIME-Version: 1.0\n+\tContent-Type: text/plain; charset=ISO-8859-1\n+\tContent-Transfer-Encoding: 8bit\n+\n+\tEOF\n+\tif test \"$1\" = body\n+\tthen\n+\t\tprintf \"%s\\0%s\\n\" abc def\n+\tfi\n+\tcat <<-\\EOF\n+\t---\n+\tdiff --git a/afile b/afile\n+\tnew file mode 100644\n+\tindex 0000000000..e69de29bb2\n+\t--$space\n+\t2.26.1\n+\tEOF\n+}\n+\n test_expect_success setup '\n \t# Note the missing \"+++\" line:\n \tcat >bad-patch.diff <<-\\EOF &&\n@@ -32,4 +62,18 @@ test_expect_success 'try to apply corrupted patch' '\n \ttest_i18ncmp expected actual\n '\n \n+test_expect_success \"NUL in commit message's body\" '\n+\ttest_when_finished \"git am --abort\" &&\n+\twrite_nul_patch body >body.patch &&\n+\ttest_must_fail git am body.patch 2>err &&\n+\tgrep \"a NUL byte in commit log message not allowed\" err\n+'\n+\n+test_expect_failure \"NUL in commit message's header\" '\n+\ttest_when_finished \"git am --abort\" &&\n+\twrite_nul_patch subject >subject.patch &&\n+\ttest_must_fail git am subject.patch 2>err &&\n+\tgrep \"a NUL byte in Subject is not allowed\" err\n+'\n+\n test_done\n-- \n2.26.1.301.g55bc3eb7cb\n\n"},{"id":"395717","messageId":"CAN0heSqnKfepx1c-7Exy1bj3_p_JdAuBLyt5WtkwZCSkOuroMg@mail.gmail.com","threadId":"53252","inReplyTo":"d1bc31e692d08d73bc577f737b0190e163440ee9.1587289680.git.congdanhqx@gmail.com","subject":"Re: [PATCH v2 1/3] t4254: merge 2 steps of a single test","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-19T12:25:41Z","receivedAt":"2020-04-19T12:25:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sun, 19 Apr 2020 at 13:03, Đoàn Trần Công Danh wrote:\n> # fatal: unable to write file '(null)' mode 100644: Bad address\n> # Also, it had the unwanted side-effect of deleting f.\n\n(This comment looks a bit unnecessary, but that's not new.)\n\n> test_expect_success 'try to apply corrupted patch' '\n> - test_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual\n> -'\n> -\n> -test_expect_success 'compare diagnostic; ensure file is still here' '\n> + test_when_finished \"git am --abort\" &&\n> + test_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual &&\n> echo \"error: git diff header lacks filename information (line 4)\" >expected &&\n> test_path_is_file f &&\n> test_i18ncmp expected actual\n\nMakes sense. The first mini-test used to just set up \"actual\" and there\nwas only one test using it, so it's hard to argue it was a \"setup\"\nphase. This looks better.\n\n\nMartin\n"},{"id":"395718","messageId":"CAN0heSqi8KvbfSD80UA9s_aj9dGYTvbeW4GXVY59014_Sm-HmQ@mail.gmail.com","threadId":"53252","inReplyTo":"e3e542d38818b762f8d6d6b50bc42e01b070772b.1587289680.git.congdanhqx@gmail.com","subject":"Re: [PATCH v2 2/3] mailinfo.c::convert_to_utf8: reuse strlen info","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-19T12:29:06Z","receivedAt":"2020-04-19T12:29:20Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sun, 19 Apr 2020 at 13:03, Đoàn Trần Công Danh wrote:\n>\n> We're passing buffer from strbuf to reencode_string,\n> which will call strlen(3) on that buffer,\n> and discard the length of newly created buffer.\n> Then, we compute the length of the return buffer to attach to strbuf.\n>\n> During this process, we introduce a discrimination between mail\n> originally written in utf-8 and other encoding.\n>\n> * if the email was written in utf-8, we leave it as is. If there is\n> a NUL character in that line, we complains loudly:\n>\n> error: a NUL byte in commit log message not allowed.\n>\n> * if the email was written in other encoding, we truncate the data as\n> the NUL character in that line, then we used the truncated line for\n> the metadata.\n>\n> We can do better by reusing all the available information,\n> and call the underlying lower level function that will be called\n> indirectly by reencode_string. By doing this, we will also postpone\n> the NUL character processing to the commit step, which will\n> complains about the faulty metadata.\n>\n> Signed-off-by: Đoàn Trần Công Danh \n\nThis makes sense to me.\n\nI think the subject could be adapted though? Now it's not about \"reusing\ninfo\" anymore, it's about using *other*, *better* info. Maybe\n\n mailinfo.c: avoid strlen on strings that might contain NUL\n\n? Could probably be improved further..\n\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -447,19 +447,21 @@ static int convert_to_utf8(struct mailinfo *mi,\n> struct strbuf *line, const char *charset)\n> {\n> char *out;\n> + size_t out_len;\n>\n> if (!mi->metainfo_charset || !charset || !*charset)\n> return 0;\n>\n> if (same_encoding(mi->metainfo_charset, charset))\n> return 0;\n> - out = reencode_string(line->buf, mi->metainfo_charset, charset);\n> + out = reencode_string_len(line->buf, line->len,\n> + mi->metainfo_charset, charset, &out_len);\n> if (!out) {\n> mi->input_error = -1;\n> return error(\"cannot convert from %s to %s\",\n> charset, mi->metainfo_charset);\n> }\n> - strbuf_attach(line, out, strlen(out), strlen(out));\n> + strbuf_attach(line, out, out_len, out_len);\n> return 0;\n> }\n\nSame diff as before, ok.\n\n> +write_nul_patch() {\n> + space=' '\n> + qNUL=\n> + case \"$1\" in\n> + subject) qNUL='=00' ;;\n> + esac\n\nHere it's case/esac...\n\n> + cat <<-EOF\n> + From ec7364544f690c560304f5a5de9428ea3b978b26 Mon Sep 17 00:00:00 2001\n> + From: A U Thor \n> + Date: Sun, 19 Apr 2020 13:42:07 +0700\n> + Subject: [PATCH] =?ISO-8859-1?q?=C4=CB${qNUL}=D1=CF=D6?=\n> + MIME-Version: 1.0\n> + Content-Type: text/plain; charset=ISO-8859-1\n> + Content-Transfer-Encoding: 8bit\n> +\n> + EOF\n> + if test \"$1\" = body\n> + then\n> + printf \"%s\\0%s\\n\" abc def\n> + fi\n\nHere it's if/fi. Looks a bit inconsistent.\n\nI suppose you tried to have a case for \"body\" above but couldn't get it\nto work? Somehow, it would seem more consistent to have a qSubject and\nqBody and handle them the same way, but maybe that's not possible?\nMaybe using q_to_nul to create qBody containing a NUL?\n\n> + cat <<-\\EOF\n> + ---\n> + diff --git a/afile b/afile\n> + new file mode 100644\n> + index 0000000000..e69de29bb2\n> + --$space\n> + 2.26.1\n> + EOF\n> +}\n\nI think this helper function should use &&-chaining.\n\n> +\n> test_expect_success setup '\n> # Note the missing \"+++\" line:\n> cat >bad-patch.diff <<-\\EOF &&\n> @@ -32,4 +62,18 @@ test_expect_success 'try to apply corrupted patch' '\n> test_i18ncmp expected actual\n> '\n>\n> +test_expect_success \"NUL in commit message's body\" '\n> + test_when_finished \"git am --abort\" &&\n> + write_nul_patch body >body.patch &&\n> + test_must_fail git am body.patch 2>err &&\n> + grep \"a NUL byte in commit log message not allowed\" err\n> +'\n\nMakes sense.\n\n> +\n> +test_expect_failure \"NUL in commit message's header\" '\n> + test_when_finished \"git am --abort\" &&\n> + write_nul_patch subject >subject.patch &&\n> + test_must_fail git am subject.patch 2>err &&\n> + grep \"a NUL byte in Subject is not allowed\" err\n> +'\n\nInteresting. :-)\n\n\nMartin\n"},{"id":"395719","messageId":"CAN0heSpZR6HtntK80DhQxtRQZo21GBd3uAKrsJxz_opyun8n4g@mail.gmail.com","threadId":"53252","inReplyTo":"cb96947b36b8ab314f5692daee6f627273ca66d3.1587289680.git.congdanhqx@gmail.com","subject":"Re: [PATCH v2 3/3] mailinfo: disallow NUL character in mail's header","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-19T12:30:19Z","receivedAt":"2020-04-19T12:30:33Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sun, 19 Apr 2020 at 13:05, Đoàn Trần Công Danh wrote:\n> --- a/t/t4254-am-corrupt.sh\n> +++ b/t/t4254-am-corrupt.sh\n> @@ -69,9 +69,11 @@ test_expect_success \"NUL in commit message's body\" '\n> grep \"a NUL byte in commit log message not allowed\" err\n> '\n>\n> -test_expect_failure \"NUL in commit message's header\" '\n> +test_expect_success \"NUL in commit message's header\" '\n> test_when_finished \"git am --abort\" &&\n> write_nul_patch subject >subject.patch &&\n> + test_must_fail git mailinfo msg patch err &&\n> + grep \"a NUL byte in Subject is not allowed\" err &&\n> test_must_fail git am subject.patch 2>err &&\n> grep \"a NUL byte in Subject is not allowed\" err\n> '\n\nWe used to fail for some reason and it's sort of clear from the size\nof the test what that reason is. Now that we flip \"failure\" to \"success\"\nwe can add some more stuff here that works. Makes sense. Of course,\nsomewhere is a line for stuffing too much into the test, i.e., you'll\nreach a point where you should have separate tests, but I think this is\nok.\n\n\nMartin\n"},{"id":"395720","messageId":"cover.1587297254.git.martin.agren@gmail.com","threadId":"53252","inReplyTo":"cover.1587240635.git.martin.agren@gmail.com","subject":"[PATCH 0/4] strbuf: fix doc for `strbuf_attach()` and avoid it","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-19T12:32:26Z","receivedAt":"2020-04-19T12:32:52Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is v2 of my attempt to look into how `strbuf_attach()` works, how\nit's documented and how it's actually used.\n\nThis is based on top of Danh's series [1]. Both their patch 2/3 and my\n4/4 reason about \"NUL\" at a particular spot. In short, they conflict\nabout whether to use function \"A\" or \"B\" at one spot and a merge should\n(in my opinion) use function \"C\". That seemed odd enough to me that I\ndecided to build on top instead.\n\n`strbuf_attach()` has always handled `mem == len`. This was intentional.\nWhen the function was eventually documented, this usage was incorrectly\ndocumented as being wrong.\n\nThat documentation didn't stop lots of callers from using it\n\"incorrectly\", though. I first fix the documentation. I then try to\navoid such usage anyway, partly because it can be confusing, partly\nbecause if we know we have enough memory, there's no need to reallocate\na larger buffer.\n\nMartin\n\n[1] https://lore.kernel.org/git/cover.1587289680.git.congdanhqx@gmail.com/\n\nMartin Ågren (4):\n strbuf: fix doc for `strbuf_attach()`\n strbuf: introduce `strbuf_attachstr_len()`\n strbuf: introduce `strbuf_attachstr()`\n strbuf_attach: prefer `strbuf_attachstr_len()`\n\n strbuf.h | 40 ++++++++++++++++++++++++++++++++++++----\n apply.c | 2 +-\n archive.c | 2 +-\n blame.c | 2 +-\n builtin/am.c | 2 +-\n convert.c | 4 ++--\n fast-import.c | 2 +-\n imap-send.c | 2 +-\n mailinfo.c | 2 +-\n merge-recursive.c | 2 +-\n path.c | 3 +--\n pretty.c | 4 ++--\n refs/files-backend.c | 3 +--\n trailer.c | 2 +-\n 14 files changed, 51 insertions(+), 21 deletions(-)\n\n-- \n2.26.1\n\n"},{"id":"395721","messageId":"54e10bb06a39d23591ea4d02665083d705682468.1587297254.git.martin.agren@gmail.com","threadId":"53252","inReplyTo":"cover.1587297254.git.martin.agren@gmail.com","subject":"[PATCH 1/4] strbuf: fix doc for `strbuf_attach()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-19T12:32:27Z","receivedAt":"2020-04-19T12:32:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Commit 917c9a7133 (\"New strbuf APIs: splice and attach.\", 2007-09-15)\nadded `strbuf_attach()`. In the commit message, it is explicitly\ndiscussed how using `mem == len` is ok, although possibly costly. When\nthis function was documented in dd613e6b87 (\"Strbuf documentation:\ndocument most functions\", 2008-06-04), this distinction was missed and\nthe documentation simply forbids this case.\n\nThe function handles reallocation and truncation, yet the docs say that\nthe \"amount [of allocated memory] must be larger than the string length,\nbecause the string you pass is supposed to be a NUL-terminated string\".\n\nSeveral callers pass in `mem == len` meaning they are compatible with\nthe implementation and the original commit message, but not with the\ndocumented behavior. In fact, compared to the documentation, they look\noutright dangerous.\n\nLater commits will convert most of those call sites to use new, simpler\ninterfaces, but in at least one instance we really do want to use this\npossibility of passing the same value twice: In rerere.c, it is not safe\nto pass in \"len, len + 1\". Doing so makes `strbuf_attach()` write a\ntrailing NUL at `result.ptr[len]` and running our test suite with\nAddressSanitizer will spot an out-of-bounds write due to this.\n\nThat is, rerere.c really has use for the flexibility that `mem == len`\ngives us. Let's update the documentation to clarify that this is ok.\n\nSigned-off-by: Martin Ågren \n---\n strbuf.h | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex ce8e49c0b2..2a462f70cc 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -112,10 +112,12 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz);\n /**\n * Attach a string to a buffer. You should specify the string to attach,\n * the current length of the string and the amount of allocated memory.\n- * The amount must be larger than the string length, because the string you\n- * pass is supposed to be a NUL-terminated string. This string _must_ be\n- * malloc()ed, and after attaching, the pointer cannot be relied upon\n- * anymore, and neither be free()d directly.\n+ * The amount must be at least as large as the string length. If the two\n+ * lengths are equal, reallocation will be handled as appropriate and in\n+ * any case, the string will be NUL-truncated as implied by `len`.\n+ *\n+ * This string _must_ be malloc()ed, and after attaching, the pointer\n+ * cannot be relied upon anymore, and neither be free()d directly.\n */\n void strbuf_attach(struct strbuf *sb, void *str, size_t len, size_t mem);\n \n-- \n2.26.1\n\n"},{"id":"395722","messageId":"d9db28d6e6616017a4a8e57508d166ab5aa2f185.1587297254.git.martin.agren@gmail.com","threadId":"53252","inReplyTo":"cover.1587297254.git.martin.agren@gmail.com","subject":"[PATCH 2/4] strbuf: introduce `strbuf_attachstr_len()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-19T12:32:28Z","receivedAt":"2020-04-19T12:32:56Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Most callers of `strbuf_attach()` provide `len + 1` for the `mem`\nparameter. That's a bit tedious and, as we will see in the next commit,\nalso fairly prone to mistakes.\n\nProvide `strbuf_attachstr_len()` for this common case to simplify\nseveral callers of `strbuf_attach()` by making this new function simply\nassume that the size of the allocated buffer is one greater than the\nlength.\n\nWe know that the string has already been allocated with space for the\ntrailing NUL, meaning it is safe to compute `len + 1`.\n\nDisallow NULL-pointers entirely. We could handle `(buf, len) == (NULL,\n0)` specially, but none of the callers we convert here seem to worry\nabout such a case. Handling this corner case specially can still be done\nusing the regular `strbuf_attach()`.\n\nAnother edge case is where someone doesn't have a NUL at `buf[len]`,\ni.e., they are (hopefully) trying to attach a substring of some larger\nstring. One could argue that they should be using `strbuf_attach()` and\nthat this is BUG-worthy, or that it would be easy enough for us to place\na NUL there for robustness and carry on. This commit does the latter,\nbut does not have a strong opinion about it.\n\nSigned-off-by: Martin Ågren \n---\n strbuf.h | 19 +++++++++++++++++++\n apply.c | 2 +-\n archive.c | 2 +-\n blame.c | 2 +-\n convert.c | 4 ++--\n imap-send.c | 2 +-\n merge-recursive.c | 2 +-\n pretty.c | 2 +-\n 8 files changed, 27 insertions(+), 8 deletions(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 2a462f70cc..7d0aeda434 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -121,6 +121,25 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz);\n */\n void strbuf_attach(struct strbuf *sb, void *str, size_t len, size_t mem);\n \n+/**\n+ * Attach a string to a buffer. You should specify the string to attach\n+ * and its length.\n+ *\n+ * The amount of allocated memory will be assumed to be one greater than\n+ * its length. The string you pass _must_ be a NUL-terminated string.\n+ * This string _must_ be malloc()ed, and after attaching, the pointer\n+ * cannot be relied upon anymore, nor should it be free()d directly.\n+ *\n+ * Do _not_ use this to truncate the string. That is, the length really\n+ * must be `len` already. To truncate (yet keeping track of the amount\n+ * of allocated memory), use `strbuf_attach()`.\n+ */\n+static inline void strbuf_attachstr_len(struct strbuf *sb,\n+\t\t\t\t\tchar *str, size_t len)\n+{\n+\tstrbuf_attach(sb, str, len, len + 1);\n+}\n+\n /**\n * Swap the contents of two string buffers.\n */\ndiff --git a/apply.c b/apply.c\nindex 144c19aaca..cab4055ea4 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3251,7 +3251,7 @@ static int read_blob_object(struct strbuf *buf, const struct object_id *oid, uns\n \t\tif (!result)\n \t\t\treturn -1;\n \t\t/* XXX read_sha1_file NUL-terminates */\n-\t\tstrbuf_attach(buf, result, sz, sz + 1);\n+\t\tstrbuf_attachstr_len(buf, result, sz);\n \t}\n \treturn 0;\n }\ndiff --git a/archive.c b/archive.c\nindex fb39706120..17b8add930 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -89,7 +89,7 @@ void *object_file_to_archive(const struct archiver_args *args,\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tsize_t size = 0;\n \n-\t\tstrbuf_attach(&buf, buffer, *sizep, *sizep + 1);\n+\t\tstrbuf_attachstr_len(&buf, buffer, *sizep);\n \t\tconvert_to_working_tree(args->repo->index, path, buf.buf, buf.len, &buf, &meta);\n \t\tif (commit)\n \t\t\tformat_subst(commit, buf.buf, buf.len, &buf);\ndiff --git a/blame.c b/blame.c\nindex 29770e5c81..12ce104fcb 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -241,7 +241,7 @@ static struct commit *fake_working_tree_commit(struct repository *r,\n \t\tcase S_IFREG:\n \t\t\tif (opt->flags.allow_textconv &&\n \t\t\t textconv_object(r, read_from, mode, &null_oid, 0, &buf_ptr, &buf_len))\n-\t\t\t\tstrbuf_attach(&buf, buf_ptr, buf_len, buf_len + 1);\n+\t\t\t\tstrbuf_attachstr_len(&buf, buf_ptr, buf_len);\n \t\t\telse if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)\n \t\t\t\tdie_errno(\"cannot open or read '%s'\", read_from);\n \t\t\tbreak;\ndiff --git a/convert.c b/convert.c\nindex 5aa87d45e3..9b3a1218a7 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -467,7 +467,7 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,\n \t\tfree(re_src);\n \t}\n \n-\tstrbuf_attach(buf, dst, dst_len, dst_len + 1);\n+\tstrbuf_attachstr_len(buf, dst, dst_len);\n \treturn 1;\n }\n \n@@ -492,7 +492,7 @@ static int encode_to_worktree(const char *path, const char *src, size_t src_len,\n \t\treturn 0;\n \t}\n \n-\tstrbuf_attach(buf, dst, dst_len, dst_len + 1);\n+\tstrbuf_attachstr_len(buf, dst, dst_len);\n \treturn 1;\n }\n \ndiff --git a/imap-send.c b/imap-send.c\nindex 6c54d8c29d..37e5b13e51 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1212,7 +1212,7 @@ static void lf_to_crlf(struct strbuf *msg)\n \t\t\tnew_msg[j++] = '\\r';\n \t\tlastc = new_msg[j++] = msg->buf[i];\n \t}\n-\tstrbuf_attach(msg, new_msg, j, j + 1);\n+\tstrbuf_attachstr_len(msg, new_msg, j);\n }\n \n /*\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d92e2acf1e..ef259e4b74 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2963,7 +2963,7 @@ static int read_oid_strbuf(struct merge_options *opt,\n \t\tfree(buf);\n \t\treturn err(opt, _(\"object %s is not a blob\"), oid_to_hex(oid));\n \t}\n-\tstrbuf_attach(dst, buf, size, size + 1);\n+\tstrbuf_attachstr_len(dst, buf, size);\n \treturn 0;\n }\n \ndiff --git a/pretty.c b/pretty.c\nindex 28afc701b6..e171830389 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1687,7 +1687,7 @@ void repo_format_commit_message(struct repository *r,\n \t\tchar *out = reencode_string_len(sb->buf, sb->len,\n \t\t\t\t\t\toutput_enc, utf8, &outsz);\n \t\tif (out)\n-\t\t\tstrbuf_attach(sb, out, outsz, outsz + 1);\n+\t\t\tstrbuf_attachstr_len(sb, out, outsz);\n \t}\n \n \tfree(context.commit_encoding);\n-- \n2.26.1\n\n"},{"id":"395723","messageId":"c3012f1da361af354a904f821b83d61f2534ccb2.1587297254.git.martin.agren@gmail.com","threadId":"53252","inReplyTo":"cover.1587297254.git.martin.agren@gmail.com","subject":"[PATCH 3/4] strbuf: introduce `strbuf_attachstr()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-19T12:32:29Z","receivedAt":"2020-04-19T12:32:57Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to the previous commit, introduce `strbuf_attachstr()` where we\ndon't even have to pass in the length of the string that we want to\nattach. Convert existing callers of `strbuf_attachstr()` that use\n`strlen()`.\n\nNote how only one caller passes in `mem == len + 1` and that the others\nhave been using `strbuf_attach()` in direct contradiction to how it was\n(incorrectly) documented up until a few commits ago.\n\nNow that the documentation has been fixed, you might say these are all\nfine. But the calling convention of `strbuf_attach()` seems sufficiently\nhard to get right that it's probably a good idea to introduce this\nhelper.\n\nThis could help reduce reallocations and memory waste. When we\npessimistically pass in `strlen(foo)` for `mem`, the strbuf will have\n`alloc == len` and will do a reallocation, not just to get one more byte\nfor the NUL (which would have been a no-op), but because we're using\n`ALLOC_GROW` under the hood, we will ask for 16 more bytes and another\n50% on top of that.\n\nSigned-off-by: Martin Ågren \n---\n strbuf.h | 11 +++++++++++\n path.c | 3 +--\n pretty.c | 2 +-\n refs/files-backend.c | 3 +--\n trailer.c | 2 +-\n 5 files changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 7d0aeda434..32cc15de0c 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -140,6 +140,17 @@ static inline void strbuf_attachstr_len(struct strbuf *sb,\n \tstrbuf_attach(sb, str, len, len + 1);\n }\n \n+/**\n+ * Attach a string to a buffer similar to `strbuf_attachstr_len()`.\n+ * Useful if you do not know the length of the string.\n+ */\n+static inline void strbuf_attachstr(struct strbuf *sb, char *str)\n+{\n+\tsize_t len = strlen(str);\n+\n+\tstrbuf_attach(sb, str, len, len + 1);\n+}\n+\n /**\n * Swap the contents of two string buffers.\n */\ndiff --git a/path.c b/path.c\nindex 9bd717c307..3cd8fd56b4 100644\n--- a/path.c\n+++ b/path.c\n@@ -815,8 +815,7 @@ const char *enter_repo(const char *path, int strict)\n \t\t\tchar *newpath = expand_user_path(used_path.buf, 0);\n \t\t\tif (!newpath)\n \t\t\t\treturn NULL;\n-\t\t\tstrbuf_attach(&used_path, newpath, strlen(newpath),\n-\t\t\t\t strlen(newpath));\n+\t\t\tstrbuf_attachstr(&used_path, newpath);\n \t\t}\n \t\tfor (i = 0; suffix[i]; i++) {\n \t\t\tstruct stat st;\ndiff --git a/pretty.c b/pretty.c\nindex e171830389..5ecdf0cbb2 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -590,7 +590,7 @@ static char *replace_encoding_header(char *buf, const char *encoding)\n \t\treturn buf; /* should not happen but be defensive */\n \tlen = cp + 1 - (buf + start);\n \n-\tstrbuf_attach(&tmp, buf, strlen(buf), strlen(buf) + 1);\n+\tstrbuf_attachstr(&tmp, buf);\n \tif (is_encoding_utf8(encoding)) {\n \t\t/* we have re-coded to UTF-8; drop the header */\n \t\tstrbuf_remove(&tmp, start, len);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 561c33ac8a..eb058d85b6 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1511,10 +1511,9 @@ static int commit_ref(struct ref_lock *lock)\n \t\t * the lockfile to. Hopefully it is empty; try to\n \t\t * delete it.\n \t\t */\n-\t\tsize_t len = strlen(path);\n \t\tstruct strbuf sb_path = STRBUF_INIT;\n \n-\t\tstrbuf_attach(&sb_path, path, len, len);\n+\t\tstrbuf_attachstr(&sb_path, path);\n \n \t\t/*\n \t\t * If this fails, commit_lock_file() will also fail\ndiff --git a/trailer.c b/trailer.c\nindex 0c414f2fed..56c4027943 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1095,7 +1095,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,\n \tfor (ptr = trailer_lines; *ptr; ptr++) {\n \t\tif (last && isspace((*ptr)->buf[0])) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\tstrbuf_attach(&sb, *last, strlen(*last), strlen(*last));\n+\t\t\tstrbuf_attachstr(&sb, *last);\n \t\t\tstrbuf_addbuf(&sb, *ptr);\n \t\t\t*last = strbuf_detach(&sb, NULL);\n \t\t\tcontinue;\n-- \n2.26.1\n\n"},{"id":"395724","messageId":"80a7f7570540e847ec986c5f3f8a6f4845866f8b.1587297254.git.martin.agren@gmail.com","threadId":"53252","inReplyTo":"cover.1587297254.git.martin.agren@gmail.com","subject":"[PATCH 4/4] strbuf_attach: prefer `strbuf_attachstr_len()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-19T12:32:30Z","receivedAt":"2020-04-19T12:32:59Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After the last few commits, we don't have many users of\n`strbuf_attach()`.\n\nConvert the sites in builtin/am.c, strbuf.c and mailinfo.c. They pass in\nthe same length twice for `len` and `mem` and will eventually hit\n`realloc(3)`, which will be a no-op. The string in am.c has been\nconstructed using the strbuf machinery in `read_commit_msg()`. In\nstrbuf.c, we've used `reencode_string_iconv()`. In mailinfo.c, we used\n`reencode_string_len()`. So in all cases, we really do have an extra\nbyte at the end with a NUL.\n\nAs explained in the previous commit, it's not just that we avoid calling\n`realloc()` to make room for a single NUL byte that we already have, we\navoid asking it for 16 more bytes and another 50% on top of that.\n\nSigned-off-by: Martin Ågren \n---\n builtin/am.c | 2 +-\n fast-import.c | 2 +-\n mailinfo.c | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex e3dfd93c25..d777855c98 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1101,7 +1101,7 @@ static void am_append_signoff(struct am_state *state)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tstrbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);\n+\tstrbuf_attachstr_len(&sb, state->msg, state->msg_len);\n \tappend_signoff(&sb, 0, 0);\n \tstate->msg = strbuf_detach(&sb, &state->msg_len);\n }\ndiff --git a/fast-import.c b/fast-import.c\nindex 202dda11a6..28fbc4792b 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2946,7 +2946,7 @@ static void cat_blob(struct object_entry *oe, struct object_id *oid)\n \tcat_blob_write(\"\\n\", 1);\n \tif (oe && oe->pack_id == pack_id) {\n \t\tlast_blob.offset = oe->idx.offset;\n-\t\tstrbuf_attach(&last_blob.data, buf, size, size);\n+\t\tstrbuf_attachstr_len(&last_blob.data, buf, size);\n \t\tlast_blob.depth = oe->depth;\n \t} else\n \t\tfree(buf);\ndiff --git a/mailinfo.c b/mailinfo.c\nindex c31991e621..942c363bfd 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -461,7 +461,7 @@ static int convert_to_utf8(struct mailinfo *mi,\n \t\treturn error(\"cannot convert from %s to %s\",\n \t\t\t charset, mi->metainfo_charset);\n \t}\n-\tstrbuf_attach(line, out, out_len, out_len);\n+\tstrbuf_attachstr_len(line, out, out_len);\n \treturn 0;\n }\n \n-- \n2.26.1\n\n"},{"id":"395726","messageId":"20200419141637.GA28207@danh.dev","threadId":"53252","inReplyTo":"CAN0heSqi8KvbfSD80UA9s_aj9dGYTvbeW4GXVY59014_Sm-HmQ@mail.gmail.com","subject":"Re: [PATCH v2 2/3] mailinfo.c::convert_to_utf8: reuse strlen info","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-19T14:16:37Z","receivedAt":"2020-04-19T14:16:43Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-04-19 14:29:06+0200, Martin Ågren wrote:\n> I think the subject could be adapted though? Now it's not about \"reusing\n> info\" anymore, it's about using *other*, *better* info. Maybe\n> \n> mailinfo.c: avoid strlen on strings that might contain NUL\n\nThis is better than mine.\nI couldn't think about any other alternative.\n\n> \n> > --- a/mailinfo.c\n> > +++ b/mailinfo.c\n> > @@ -447,19 +447,21 @@ static int convert_to_utf8(struct mailinfo *mi,\n> > struct strbuf *line, const char *charset)\n> > {\n> > char *out;\n> > + size_t out_len;\n> >\n> > if (!mi->metainfo_charset || !charset || !*charset)\n> > return 0;\n> >\n> > if (same_encoding(mi->metainfo_charset, charset))\n> > return 0;\n> > - out = reencode_string(line->buf, mi->metainfo_charset, charset);\n> > + out = reencode_string_len(line->buf, line->len,\n> > + mi->metainfo_charset, charset, &out_len);\n> > if (!out) {\n> > mi->input_error = -1;\n> > return error(\"cannot convert from %s to %s\",\n> > charset, mi->metainfo_charset);\n> > }\n> > - strbuf_attach(line, out, strlen(out), strlen(out));\n> > + strbuf_attach(line, out, out_len, out_len);\n> > return 0;\n> > }\n> \n> Same diff as before, ok.\n> \n> > +write_nul_patch() {\n> > + space=' '\n> > + qNUL=\n> > + case \"$1\" in\n> > + subject) qNUL='=00' ;;\n> > + esac\n> \n> Here it's case/esac...\n> \n> > + cat <<-EOF\n> > + From ec7364544f690c560304f5a5de9428ea3b978b26 Mon Sep 17 00:00:00 2001\n> > + From: A U Thor \n> > + Date: Sun, 19 Apr 2020 13:42:07 +0700\n> > + Subject: [PATCH] =?ISO-8859-1?q?=C4=CB${qNUL}=D1=CF=D6?=\n> > + MIME-Version: 1.0\n> > + Content-Type: text/plain; charset=ISO-8859-1\n> > + Content-Transfer-Encoding: 8bit\n> > +\n> > + EOF\n> > + if test \"$1\" = body\n> > + then\n> > + printf \"%s\\0%s\\n\" abc def\n> > + fi\n> \n> Here it's if/fi. Looks a bit inconsistent.\n> \n> I suppose you tried to have a case for \"body\" above but couldn't get it\n> to work? Somehow, it would seem more consistent to have a qSubject and\n> qBody and handle them the same way, but maybe that's not possible?\n> Maybe using q_to_nul to create qBody containing a NUL?\n\nThose patch was written in different time, with different thought in\nmind.\n\nI need to send a re-roll to update the subject and moving oneline in\ntest code to later patch.\n\nI'll update this hunk with that re-roll.\nYour new series _won't_ be affected, though.\n\n> > + cat <<-\\EOF\n> > + ---\n> > + diff --git a/afile b/afile\n> > + new file mode 100644\n> > + index 0000000000..e69de29bb2\n> > + --$space\n> > + 2.26.1\n> > + EOF\n> > +}\n> \n> I think this helper function should use &&-chaining.\n\nCorrect.\n\n> > +\n> > test_expect_success setup '\n> > # Note the missing \"+++\" line:\n> > cat >bad-patch.diff <<-\\EOF &&\n> > @@ -32,4 +62,18 @@ test_expect_success 'try to apply corrupted patch' '\n> > test_i18ncmp expected actual\n> > '\n> >\n> > +test_expect_success \"NUL in commit message's body\" '\n> > + test_when_finished \"git am --abort\" &&\n> > + write_nul_patch body >body.patch &&\n> > + test_must_fail git am body.patch 2>err &&\n> > + grep \"a NUL byte in commit log message not allowed\" err\n> > +'\n> \n> Makes sense.\n> \n> > +\n> > +test_expect_failure \"NUL in commit message's header\" '\n> > + test_when_finished \"git am --abort\" &&\n> > + write_nul_patch subject >subject.patch &&\n> > + test_must_fail git am subject.patch 2>err &&\n> > + grep \"a NUL byte in Subject is not allowed\" err\n> > +'\n> \n> Interesting. :-)\n\nGoing through the mail now make me think about moving the last grep to\nnext patch in order to clearly indicate that we expect \"git-am\" is\nfailling but it's passed instead.\n\nAnd that message isn't introduced until the very next change.\n\n-- \nDanh\n"},{"id":"395727","messageId":"20200419141754.GB28207@danh.dev","threadId":"53252","inReplyTo":"CAN0heSqnKfepx1c-7Exy1bj3_p_JdAuBLyt5WtkwZCSkOuroMg@mail.gmail.com","subject":"Re: [PATCH v2 1/3] t4254: merge 2 steps of a single test","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-19T14:17:54Z","receivedAt":"2020-04-19T14:17:58Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-04-19 14:25:41+0200, Martin Ågren wrote:\n> On Sun, 19 Apr 2020 at 13:03, Đoàn Trần Công Danh wrote:\n> > # fatal: unable to write file '(null)' mode 100644: Bad address\n> > # Also, it had the unwanted side-effect of deleting f.\n> \n> (This comment looks a bit unnecessary, but that's not new.)\n\nI haven't looked very hard into this part, I guess I could take more\ntime to look at it and decide if we should remove it or not.\n\n> > test_expect_success 'try to apply corrupted patch' '\n> > - test_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual\n> > -'\n> > -\n> > -test_expect_success 'compare diagnostic; ensure file is still here' '\n> > + test_when_finished \"git am --abort\" &&\n> > + test_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual &&\n> > echo \"error: git diff header lacks filename information (line 4)\" >expected &&\n> > test_path_is_file f &&\n> > test_i18ncmp expected actual\n> \n> Makes sense. The first mini-test used to just set up \"actual\" and there\n> was only one test using it, so it's hard to argue it was a \"setup\"\n> phase. This looks better.\n\n-- \nDanh\n"},{"id":"395728","messageId":"20200419142423.GC28207@danh.dev","threadId":"53252","inReplyTo":"CAN0heSpZR6HtntK80DhQxtRQZo21GBd3uAKrsJxz_opyun8n4g@mail.gmail.com","subject":"Re: [PATCH v2 3/3] mailinfo: disallow NUL character in mail's header","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-19T14:24:23Z","receivedAt":"2020-04-19T14:24:29Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-04-19 14:30:19+0200, Martin Ågren wrote:\n> On Sun, 19 Apr 2020 at 13:05, Đoàn Trần Công Danh wrote:\n> > --- a/t/t4254-am-corrupt.sh\n> > +++ b/t/t4254-am-corrupt.sh\n> > @@ -69,9 +69,11 @@ test_expect_success \"NUL in commit message's body\" '\n> > grep \"a NUL byte in commit log message not allowed\" err\n> > '\n> >\n> > -test_expect_failure \"NUL in commit message's header\" '\n> > +test_expect_success \"NUL in commit message's header\" '\n> > test_when_finished \"git am --abort\" &&\n> > write_nul_patch subject >subject.patch &&\n> > + test_must_fail git mailinfo msg patch err &&\n> > + grep \"a NUL byte in Subject is not allowed\" err &&\n> > test_must_fail git am subject.patch 2>err &&\n> > grep \"a NUL byte in Subject is not allowed\" err\n> > '\n> \n> We used to fail for some reason and it's sort of clear from the size\n> of the test what that reason is. Now that we flip \"failure\" to \"success\"\n> we can add some more stuff here that works. Makes sense. Of course,\n> somewhere is a line for stuffing too much into the test, i.e., you'll\n> reach a point where you should have separate tests, but I think this is\n> ok.\n\nI'm not really keen to check the \"git-mailinfo\" failure here.\nSince, another developer may come and decide that we should keep the\nmailinfo as is, i.e. keep NUL character in Subject, Author, Email,\netc... And let's git-commit complains instead.\n\nAdding a check for git-mailinfo here may limit future development.\nOther's feedback is welcomed.\n\n-- \nDanh\n"},{"id":"395750","messageId":"xmqqd082rrns.fsf@gitster.c.googlers.com","threadId":"53252","inReplyTo":"54e10bb06a39d23591ea4d02665083d705682468.1587297254.git.martin.agren@gmail.com","subject":"Re: [PATCH 1/4] strbuf: fix doc for `strbuf_attach()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-20T17:30:31Z","receivedAt":"2020-04-20T17:30:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> Commit 917c9a7133 (\"New strbuf APIs: splice and attach.\", 2007-09-15)\n> added `strbuf_attach()`. In the commit message, it is explicitly\n> discussed how using `mem == len` is ok, although possibly costly. When\n> this function was documented in dd613e6b87 (\"Strbuf documentation:\n> document most functions\", 2008-06-04), this distinction was missed and\n> the documentation simply forbids this case.\n\nIn other words, mem==len implies that the original lacks the\nterminating '\\0', so attach needs to reallocate from the get go, so\ndiscouraging such use may make sense, but it is too strong to forbid\nit, as the strbuf invariant is held safely? If so, the description\nmakes sense to me.\n\n> The function handles reallocation and truncation, yet the docs say that\n> the \"amount [of allocated memory] must be larger than the string length,\n> because the string you pass is supposed to be a NUL-terminated string\".\n\nIOW, _attach() does not mind if the original lacks '\\0' at the end.\n\n> diff --git a/strbuf.h b/strbuf.h\n> index ce8e49c0b2..2a462f70cc 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -112,10 +112,12 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz);\n> /**\n> * Attach a string to a buffer. You should specify the string to attach,\n> * the current length of the string and the amount of allocated memory.\n> + * The amount must be at least as large as the string length. If the two\n> + * lengths are equal, reallocation will be handled as appropriate and in\n> + * any case, the string will be NUL-truncated as implied by `len`.\n\nNUL-truncated? Ah, if mem and len are the same, the string is reallocated\nto fit an extra byte to NUL-terminate, to make sure strlen(sb->buf)==len\nholds. Makes sense.\n\n> + *\n> + * This string _must_ be malloc()ed, and after attaching, the pointer\n> + * cannot be relied upon anymore, and neither be free()d directly.\n\nThat's a good thing to explain.\n\n> */\n> void strbuf_attach(struct strbuf *sb, void *str, size_t len, size_t mem);\n"},{"id":"395754","messageId":"xmqqzhb6q74r.fsf@gitster.c.googlers.com","threadId":"53252","inReplyTo":"c3012f1da361af354a904f821b83d61f2534ccb2.1587297254.git.martin.agren@gmail.com","subject":"Re: [PATCH 3/4] strbuf: introduce `strbuf_attachstr()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-20T19:39:16Z","receivedAt":"2020-04-20T19:39:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> +/**\n> + * Attach a string to a buffer similar to `strbuf_attachstr_len()`.\n> + * Useful if you do not know the length of the string.\n> + */\n> +static inline void strbuf_attachstr(struct strbuf *sb, char *str)\n> +{\n> +\tsize_t len = strlen(str);\n> +\n> +\tstrbuf_attach(sb, str, len, len + 1);\n> +}\n\nThis is somewhat worrysome in that the interface is _so_ simple that\npeople may fail to see that str must be allocated piece of memory,\nand it is preferrable if string fully fills the allocation.\n\nWe should repeat that (instead of just trusting \"similar to ...\"\nwould tell them enough) in the doc, perhaps?\n\n> diff --git a/trailer.c b/trailer.c\n> index 0c414f2fed..56c4027943 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -1095,7 +1095,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,\n> \tfor (ptr = trailer_lines; *ptr; ptr++) {\n> \t\tif (last && isspace((*ptr)->buf[0])) {\n> \t\t\tstruct strbuf sb = STRBUF_INIT;\n> -\t\t\tstrbuf_attach(&sb, *last, strlen(*last), strlen(*last));\n> +\t\t\tstrbuf_attachstr(&sb, *last);\n> \t\t\tstrbuf_addbuf(&sb, *ptr);\n> \t\t\t*last = strbuf_detach(&sb, NULL);\n> \t\t\tcontinue;\n\nThis is not wrong per-se, but it is unclear if use of strbuf_attach*\nfamily to avoid an explicit malloc/copy/free is buying much at this\ncallsite. Simplifying the code here of course is not within the\nscope of this series.\n\n"},{"id":"395758","messageId":"xmqqv9luq66u.fsf@gitster.c.googlers.com","threadId":"53252","inReplyTo":"e3e542d38818b762f8d6d6b50bc42e01b070772b.1587289680.git.congdanhqx@gmail.com","subject":"Re: [PATCH v2 2/3] mailinfo.c::convert_to_utf8: reuse strlen info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-20T19:59:37Z","receivedAt":"2020-04-20T19:59:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Đoàn Trần Công Danh writes:\n\n> diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\n> index ddd35498db..98cda32d0a 100755\n> --- a/t/t4254-am-corrupt.sh\n> +++ b/t/t4254-am-corrupt.sh\n> @@ -3,6 +3,36 @@\n> test_description='git am with corrupt input'\n> . ./test-lib.sh\n> \n> +write_nul_patch() {\n\nStyle: SP on both sides of (), i.e.\n\n\twrite_nul_patch () {\n\nBut isn't this misnamed? You are interested in injecting '\\0' byte\nin the e-mail headers and bodies, not necessarily part of the patch,\nbut \"nul-patch\" somehow hints readers that we are writing out a Null\nPatch (something that does not do anything, perhaps?).\n\nsample_mbox_with_nul is the best alternative I can come up with\noffhand, which is not great either, but at least it does not say\npatch.\n\n> +\tspace=' '\n> +\tqNUL=\n> +\tcase \"$1\" in\n> +\t\tsubject) qNUL='=00' ;;\n> +\tesac\n\nStyle: case/esac aligns with case arms, i.e.\n\n\tcase \"$1\" in\n\tsubject) qNUL='=00' ;;\n\tesac\n\n> +\tcat <<-EOF\n> +\tFrom ec7364544f690c560304f5a5de9428ea3b978b26 Mon Sep 17 00:00:00 2001\n> +\tFrom: A U Thor \n> +\tDate: Sun, 19 Apr 2020 13:42:07 +0700\n> +\tSubject: [PATCH] =?ISO-8859-1?q?=C4=CB${qNUL}=D1=CF=D6?=\n> +\tMIME-Version: 1.0\n> +\tContent-Type: text/plain; charset=ISO-8859-1\n> +\tContent-Transfer-Encoding: 8bit\n> +\n> +\tEOF\n\nSince the above does have ${qNUL} interpolated, not quoting <<-EOF\nis correct. Good.\n\n> +\tif test \"$1\" = body\n> +\tthen\n> +\t\tprintf \"%s\\0%s\\n\" abc def\n> +\tfi\n\nOK. So we won't be able to inject NUL byte in both header and body\nat the same time. If you wanted to allow it, you could write\n\n\tcase \",$1,\" in\n\t*,subject,*)\tqNUL=\"=00\" ;;\n\tesac\n\nin the early part, and then rewrite this one like so:\n\n\tcase \",$1,\" in\n\t*,body,*)\tprintf \"...\" ;;\n\tesac\n\nThen those callers who want to ask for both can say\n\n\tsample_mbox_with_nul subject,body\n\n> +\tcat <<-\\EOF\n> +\t---\n> +\tdiff --git a/afile b/afile\n> +\tnew file mode 100644\n> +\tindex 0000000000..e69de29bb2\n> +\t--$space\n> +\t2.26.1\n> +\tEOF\n\nDoesn't this want to interpolate $space in the output? I think you\nwant to say <<-EOF, without quoting.\n\n cd t && sh t4254-am-corrupt.sh -d && cat trash*.t4254-*/body.patch\n\ntells me that \"--$space\" is left in the output, not \"-- \".\n\n> +}\n> +\n> test_expect_success setup '\n> \t# Note the missing \"+++\" line:\n> \tcat >bad-patch.diff <<-\\EOF &&\n> @@ -32,4 +62,18 @@ test_expect_success 'try to apply corrupted patch' '\n> \ttest_i18ncmp expected actual\n> '\n> \n> +test_expect_success \"NUL in commit message's body\" '\n> +\ttest_when_finished \"git am --abort\" &&\n> +\twrite_nul_patch body >body.patch &&\n> +\ttest_must_fail git am body.patch 2>err &&\n> +\tgrep \"a NUL byte in commit log message not allowed\" err\n> +'\n> +\n> +test_expect_failure \"NUL in commit message's header\" '\n> +\ttest_when_finished \"git am --abort\" &&\n> +\twrite_nul_patch subject >subject.patch &&\n> +\ttest_must_fail git am subject.patch 2>err &&\n> +\tgrep \"a NUL byte in Subject is not allowed\" err\n> +'\n> +\n> test_done\n"},{"id":"395787","messageId":"20200420235302.GA2377@danh.dev","threadId":"53252","inReplyTo":"xmqqv9luq66u.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] mailinfo.c::convert_to_utf8: reuse strlen info","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-20T23:53:02Z","receivedAt":"2020-04-20T23:53:07Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-04-20 12:59:37-0700, Junio C Hamano wrote:\n> Đoàn Trần Công Danh writes:\n> \n> > diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\n> > index ddd35498db..98cda32d0a 100755\n> > --- a/t/t4254-am-corrupt.sh\n> > +++ b/t/t4254-am-corrupt.sh\n> > @@ -3,6 +3,36 @@\n> > test_description='git am with corrupt input'\n> > . ./test-lib.sh\n> > \n> > +write_nul_patch() {\n> \n> Style: SP on both sides of (), i.e.\n> \n> \twrite_nul_patch () {\n> \n> But isn't this misnamed? You are interested in injecting '\\0' byte\n\nOriginally, this function was written to create a file named\n\"nul.patch\", but it's prohibited in Windows land.\n\nIt's still misnamed, though.\n\n> in the e-mail headers and bodies, not necessarily part of the patch,\n> but \"nul-patch\" somehow hints readers that we are writing out a Null\n> Patch (something that does not do anything, perhaps?).\n> \n> sample_mbox_with_nul is the best alternative I can come up with\n> offhand, which is not great either, but at least it does not say\n> patch.\n\nI prefer having a verb, but make_sample_mbox_with_nul is too long.\nI'll take make_mbox_with_nul. Naming is hard.\n\n> > +\tcat <<-\\EOF\n> > +\t---\n> > +\tdiff --git a/afile b/afile\n> > +\tnew file mode 100644\n> > +\tindex 0000000000..e69de29bb2\n> > +\t--$space\n> > +\t2.26.1\n> > +\tEOF\n> \n> Doesn't this want to interpolate $space in the output? I think you\n> want to say <<-EOF, without quoting.\n> \n> cd t && sh t4254-am-corrupt.sh -d && cat trash*.t4254-*/body.patch\n> \n> tells me that \"--$space\" is left in the output, not \"-- \".\n\nI recalled it now. Originially, I wrote \"-- \" in that line,\nWhen I try git-am(1) the mail, I saw a warning about trailing space.\nI want to get rid of it but forget to change \"-\\EOF\"\n\nThose last 2 lines isn't strictly required, I wanted to mimic a real\npatch created by git-format-patch, though.\n\n\n-- \nDanh\n"},{"id":"395789","messageId":"cover.1587426713.git.congdanhqx@gmail.com","threadId":"53252","inReplyTo":"20200418035449.9450-1-congdanhqx@gmail.com","subject":"[PATCH v3 0/3] Disallow NUL character in mailinfo","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-20T23:54:33Z","receivedAt":"2020-04-20T23:55:00Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"\nAs of current state, Git disallow NUL character in commit message.\n\nThis indirectly disallow NUL in the body of commit message that sent by email\nin UTF-8 encoding.\n\nIn those below cases:\n\n* NUL character in header field (be it Subject, Author, Email, etc...)\n* NUL in the body of commit message that originally sent in other than UTF-8\n encoding\n\nGit silently accepts them, albeit, truncate at the NUL character.\n\nThose email is clearly not generated by Git, they must be crafted.\n\nComplains loudly about those NUL characters.\n\nChange from v2:\n\n* reword 2/3 commit message\n* rename helper for crafting faulty patch, use single heredoc,\n and allow quoted NUL in both subject and body\n* postpone a grep in test for NUL in subject to clearly indicate that git-am\n is failing instead of wrong error message.\n* quote the header that contains NUL in the error message.\n\n\nĐoàn Trần Công Danh (3):\n t4254: merge 2 steps of a single test\n mailinfo.c: avoid strlen on strings that can contains NUL\n mailinfo: disallow NUL character in mail's header\n\n mailinfo.c | 11 +++++++--\n t/t4254-am-corrupt.sh | 53 +++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 58 insertions(+), 6 deletions(-)\n\nRange-diff against v2:\n1: d1bc31e692 = 1: d1bc31e692 t4254: merge 2 steps of a single test\n2: e3e542d388 ! 2: 97ede4aab2 mailinfo.c::convert_to_utf8: reuse strlen info\n @@ Metadata\n Author: Đoàn Trần Công Danh \n \n ## Commit message ##\n - mailinfo.c::convert_to_utf8: reuse strlen info\n + mailinfo.c: avoid strlen on strings that can contains NUL\n \n We're passing buffer from strbuf to reencode_string,\n which will call strlen(3) on that buffer,\n @@ t/t4254-am-corrupt.sh\n test_description='git am with corrupt input'\n . ./test-lib.sh\n \n -+write_nul_patch() {\n ++make_mbox_with_nul () {\n +\tspace=' '\n -+\tqNUL=\n -+\tcase \"$1\" in\n -+\t\tsubject) qNUL='=00' ;;\n -+\tesac\n ++\tq_nul_in_subject=\n ++\tq_nul_in_body=\n ++\twhile test $# -ne 0\n ++\tdo\n ++\t\tcase \"$1\" in\n ++\t\tsubject) q_nul_in_subject='=00' ;;\n ++\t\tbody) q_nul_in_body='=00' ;;\n ++\t\tesac &&\n ++\t\tshift\n ++\tdone &&\n +\tcat <<-EOF\n +\tFrom ec7364544f690c560304f5a5de9428ea3b978b26 Mon Sep 17 00:00:00 2001\n +\tFrom: A U Thor \n +\tDate: Sun, 19 Apr 2020 13:42:07 +0700\n -+\tSubject: [PATCH] =?ISO-8859-1?q?=C4=CB${qNUL}=D1=CF=D6?=\n ++\tSubject: [PATCH] =?ISO-8859-1?q?=C4=CB${q_nul_in_subject}=D1=CF=D6?=\n +\tMIME-Version: 1.0\n +\tContent-Type: text/plain; charset=ISO-8859-1\n -+\tContent-Transfer-Encoding: 8bit\n ++\tContent-Transfer-Encoding: quoted-printable\n +\n -+\tEOF\n -+\tif test \"$1\" = body\n -+\tthen\n -+\t\tprintf \"%s\\0%s\\n\" abc def\n -+\tfi\n -+\tcat <<-\\EOF\n ++\tabc${q_nul_in_body}def\n +\t---\n +\tdiff --git a/afile b/afile\n +\tnew file mode 100644\n @@ t/t4254-am-corrupt.sh: test_expect_success 'try to apply corrupted patch' '\n \n +test_expect_success \"NUL in commit message's body\" '\n +\ttest_when_finished \"git am --abort\" &&\n -+\twrite_nul_patch body >body.patch &&\n ++\tmake_mbox_with_nul body >body.patch &&\n +\ttest_must_fail git am body.patch 2>err &&\n +\tgrep \"a NUL byte in commit log message not allowed\" err\n +'\n +\n -+test_expect_failure \"NUL in commit message's header\" '\n -+\ttest_when_finished \"git am --abort\" &&\n -+\twrite_nul_patch subject >subject.patch &&\n -+\ttest_must_fail git am subject.patch 2>err &&\n -+\tgrep \"a NUL byte in Subject is not allowed\" err\n -+'\n ++test_expect_failure \"NUL in commit message's header\" \"\n ++\ttest_when_finished 'git am --abort' &&\n ++\tmake_mbox_with_nul subject >subject.patch &&\n ++\ttest_must_fail git am subject.patch\n ++\"\n +\n test_done\n3: cb96947b36 ! 3: b2e760227e mailinfo: disallow NUL character in mail's header\n @@ mailinfo.c: static void handle_info(struct mailinfo *mi)\n \t\t\tcontinue;\n \n +\t\tif (memchr(hdr->buf, '\\0', hdr->len)) {\n -+\t\t\terror(\"a NUL byte in %s is not allowed.\", header[i]);\n ++\t\t\terror(\"a NUL byte in '%s' is not allowed.\", header[i]);\n +\t\t\tmi->input_error = -1;\n +\t\t}\n +\n @@ t/t4254-am-corrupt.sh: test_expect_success \"NUL in commit message's body\" '\n \tgrep \"a NUL byte in commit log message not allowed\" err\n '\n \n --test_expect_failure \"NUL in commit message's header\" '\n -+test_expect_success \"NUL in commit message's header\" '\n - \ttest_when_finished \"git am --abort\" &&\n - \twrite_nul_patch subject >subject.patch &&\n +-test_expect_failure \"NUL in commit message's header\" \"\n ++test_expect_success \"NUL in commit message's header\" \"\n + \ttest_when_finished 'git am --abort' &&\n + \tmake_mbox_with_nul subject >subject.patch &&\n +-\ttest_must_fail git am subject.patch\n +\ttest_must_fail git mailinfo msg patch err &&\n -+\tgrep \"a NUL byte in Subject is not allowed\" err &&\n - \ttest_must_fail git am subject.patch 2>err &&\n - \tgrep \"a NUL byte in Subject is not allowed\" err\n - '\n ++\tgrep \\\"a NUL byte in 'Subject' is not allowed\\\" err &&\n ++\ttest_must_fail git am subject.patch 2>err &&\n ++\tgrep \\\"a NUL byte in 'Subject' is not allowed\\\" err\n + \"\n + \n + test_done\n-- \n2.26.1.301.g55bc3eb7cb\n\n"},{"id":"395790","messageId":"d1bc31e692d08d73bc577f737b0190e163440ee9.1587426713.git.congdanhqx@gmail.com","threadId":"53252","inReplyTo":"cover.1587426713.git.congdanhqx@gmail.com","subject":"[PATCH v3 1/3] t4254: merge 2 steps of a single test","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-20T23:54:34Z","receivedAt":"2020-04-20T23:55:02Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"While we are at it, make sure we run a clean up after testing.\nIn a later patch, we will test for more corrupted patch.\n\nSigned-off-by: Đoàn Trần Công Danh \n---\n t/t4254-am-corrupt.sh | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\nindex fd3bdbfe2c..ddd35498db 100755\n--- a/t/t4254-am-corrupt.sh\n+++ b/t/t4254-am-corrupt.sh\n@@ -25,10 +25,8 @@ test_expect_success setup '\n # fatal: unable to write file '(null)' mode 100644: Bad address\n # Also, it had the unwanted side-effect of deleting f.\n test_expect_success 'try to apply corrupted patch' '\n-\ttest_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual\n-'\n-\n-test_expect_success 'compare diagnostic; ensure file is still here' '\n+\ttest_when_finished \"git am --abort\" &&\n+\ttest_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual &&\n \techo \"error: git diff header lacks filename information (line 4)\" >expected &&\n \ttest_path_is_file f &&\n \ttest_i18ncmp expected actual\n-- \n2.26.1.301.g55bc3eb7cb\n\n"},{"id":"395791","messageId":"97ede4aab2ece936eab3caf06db5a8f5d0eea4ba.1587426713.git.congdanhqx@gmail.com","threadId":"53252","inReplyTo":"cover.1587426713.git.congdanhqx@gmail.com","subject":"[PATCH v3 2/3] mailinfo.c: avoid strlen on strings that can contains NUL","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-20T23:54:35Z","receivedAt":"2020-04-20T23:55:05Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"We're passing buffer from strbuf to reencode_string,\nwhich will call strlen(3) on that buffer,\nand discard the length of newly created buffer.\nThen, we compute the length of the return buffer to attach to strbuf.\n\nDuring this process, we introduce a discrimination between mail\noriginally written in utf-8 and other encoding.\n\n* if the email was written in utf-8, we leave it as is. If there is\n a NUL character in that line, we complains loudly:\n\n \terror: a NUL byte in commit log message not allowed.\n\n* if the email was written in other encoding, we truncate the data as\n the NUL character in that line, then we used the truncated line for\n the metadata.\n\nWe can do better by reusing all the available information,\nand call the underlying lower level function that will be called\nindirectly by reencode_string. By doing this, we will also postpone\nthe NUL character processing to the commit step, which will\ncomplains about the faulty metadata.\n\nSigned-off-by: Đoàn Trần Công Danh \n---\n mailinfo.c | 6 ++++--\n t/t4254-am-corrupt.sh | 44 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 2 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 742fa376ab..0e9911df6d 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -447,19 +447,21 @@ static int convert_to_utf8(struct mailinfo *mi,\n \t\t\t struct strbuf *line, const char *charset)\n {\n \tchar *out;\n+\tsize_t out_len;\n \n \tif (!mi->metainfo_charset || !charset || !*charset)\n \t\treturn 0;\n \n \tif (same_encoding(mi->metainfo_charset, charset))\n \t\treturn 0;\n-\tout = reencode_string(line->buf, mi->metainfo_charset, charset);\n+\tout = reencode_string_len(line->buf, line->len,\n+\t\t\t\t mi->metainfo_charset, charset, &out_len);\n \tif (!out) {\n \t\tmi->input_error = -1;\n \t\treturn error(\"cannot convert from %s to %s\",\n \t\t\t charset, mi->metainfo_charset);\n \t}\n-\tstrbuf_attach(line, out, strlen(out), strlen(out));\n+\tstrbuf_attach(line, out, out_len, out_len);\n \treturn 0;\n }\n \ndiff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\nindex ddd35498db..1bbc37bc92 100755\n--- a/t/t4254-am-corrupt.sh\n+++ b/t/t4254-am-corrupt.sh\n@@ -3,6 +3,37 @@\n test_description='git am with corrupt input'\n . ./test-lib.sh\n \n+make_mbox_with_nul () {\n+\tspace=' '\n+\tq_nul_in_subject=\n+\tq_nul_in_body=\n+\twhile test $# -ne 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\tsubject) q_nul_in_subject='=00' ;;\n+\t\tbody) q_nul_in_body='=00' ;;\n+\t\tesac &&\n+\t\tshift\n+\tdone &&\n+\tcat <<-EOF\n+\tFrom ec7364544f690c560304f5a5de9428ea3b978b26 Mon Sep 17 00:00:00 2001\n+\tFrom: A U Thor \n+\tDate: Sun, 19 Apr 2020 13:42:07 +0700\n+\tSubject: [PATCH] =?ISO-8859-1?q?=C4=CB${q_nul_in_subject}=D1=CF=D6?=\n+\tMIME-Version: 1.0\n+\tContent-Type: text/plain; charset=ISO-8859-1\n+\tContent-Transfer-Encoding: quoted-printable\n+\n+\tabc${q_nul_in_body}def\n+\t---\n+\tdiff --git a/afile b/afile\n+\tnew file mode 100644\n+\tindex 0000000000..e69de29bb2\n+\t--$space\n+\t2.26.1\n+\tEOF\n+}\n+\n test_expect_success setup '\n \t# Note the missing \"+++\" line:\n \tcat >bad-patch.diff <<-\\EOF &&\n@@ -32,4 +63,17 @@ test_expect_success 'try to apply corrupted patch' '\n \ttest_i18ncmp expected actual\n '\n \n+test_expect_success \"NUL in commit message's body\" '\n+\ttest_when_finished \"git am --abort\" &&\n+\tmake_mbox_with_nul body >body.patch &&\n+\ttest_must_fail git am body.patch 2>err &&\n+\tgrep \"a NUL byte in commit log message not allowed\" err\n+'\n+\n+test_expect_failure \"NUL in commit message's header\" \"\n+\ttest_when_finished 'git am --abort' &&\n+\tmake_mbox_with_nul subject >subject.patch &&\n+\ttest_must_fail git am subject.patch\n+\"\n+\n test_done\n-- \n2.26.1.301.g55bc3eb7cb\n\n"},{"id":"395792","messageId":"b2e760227e4c80af67ce3c1c405207dc7a1f7bf4.1587426713.git.congdanhqx@gmail.com","threadId":"53252","inReplyTo":"cover.1587426713.git.congdanhqx@gmail.com","subject":"[PATCH v3 3/3] mailinfo: disallow NUL character in mail's header","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-20T23:54:36Z","receivedAt":"2020-04-20T23:55:06Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Signed-off-by: Đoàn Trần Công Danh \n---\n mailinfo.c | 5 +++++\n t/t4254-am-corrupt.sh | 7 +++++--\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 0e9911df6d..5681d9130d 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -1138,6 +1138,11 @@ static void handle_info(struct mailinfo *mi)\n \t\telse\n \t\t\tcontinue;\n \n+\t\tif (memchr(hdr->buf, '\\0', hdr->len)) {\n+\t\t\terror(\"a NUL byte in '%s' is not allowed.\", header[i]);\n+\t\t\tmi->input_error = -1;\n+\t\t}\n+\n \t\tif (!strcmp(header[i], \"Subject\")) {\n \t\t\tif (!mi->keep_subject) {\n \t\t\t\tcleanup_subject(mi, hdr);\ndiff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\nindex 1bbc37bc92..daf01c309d 100755\n--- a/t/t4254-am-corrupt.sh\n+++ b/t/t4254-am-corrupt.sh\n@@ -70,10 +70,13 @@ test_expect_success \"NUL in commit message's body\" '\n \tgrep \"a NUL byte in commit log message not allowed\" err\n '\n \n-test_expect_failure \"NUL in commit message's header\" \"\n+test_expect_success \"NUL in commit message's header\" \"\n \ttest_when_finished 'git am --abort' &&\n \tmake_mbox_with_nul subject >subject.patch &&\n-\ttest_must_fail git am subject.patch\n+\ttest_must_fail git mailinfo msg patch err &&\n+\tgrep \\\"a NUL byte in 'Subject' is not allowed\\\" err &&\n+\ttest_must_fail git am subject.patch 2>err &&\n+\tgrep \\\"a NUL byte in 'Subject' is not allowed\\\" err\n \"\n \n test_done\n-- \n2.26.1.301.g55bc3eb7cb\n\n"},{"id":"395842","messageId":"CAN0heSrqDYrZnR8aGRksf4qLZE0YPgPYUbK4AFnnOX470M34Ow@mail.gmail.com","threadId":"53252","inReplyTo":"xmqqd082rrns.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/4] strbuf: fix doc for `strbuf_attach()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-21T18:44:23Z","receivedAt":"2020-04-21T18:44:37Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Mon, 20 Apr 2020 at 19:30, Junio C Hamano wrote:\n>\n> Martin Ågren writes:\n>\n> > The function handles reallocation and truncation, yet the docs say that\n> > the \"amount [of allocated memory] must be larger than the string length,\n> > because the string you pass is supposed to be a NUL-terminated string\".\n>\n> IOW, _attach() does not mind if the original lacks '\\0' at the end.\n\nRight, nor if it lacks the space for it.\n\n> > diff --git a/strbuf.h b/strbuf.h\n> > index ce8e49c0b2..2a462f70cc 100644\n> > --- a/strbuf.h\n> > +++ b/strbuf.h\n> > @@ -112,10 +112,12 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz);\n> > /**\n> > * Attach a string to a buffer. You should specify the string to attach,\n> > * the current length of the string and the amount of allocated memory.\n> > + * The amount must be at least as large as the string length. If the two\n> > + * lengths are equal, reallocation will be handled as appropriate and in\n> > + * any case, the string will be NUL-truncated as implied by `len`.\n>\n> NUL-truncated? Ah, if mem and len are the same, the string is reallocated\n> to fit an extra byte to NUL-terminate, to make sure strlen(sb->buf)==len\n> holds. Makes sense.\n\nExactly. NUL-terminated would be better. I think I'll split that last\nsentence and replace it with something like the following:\n\n If the two lengths are equal, reallocation will be handled as\n needed. And regardless, the string will be NUL-terminated at `len`.\n\n(One might still have strlen(sb->buf) < len though. This just guarantees\n\"<=\")\n\n\nMartin\n"},{"id":"395843","messageId":"CAN0heSp3KWU8Q8_+-GPUePfQorbyWdwm-eo4eGBRcnTtxOvS4A@mail.gmail.com","threadId":"53252","inReplyTo":"xmqqzhb6q74r.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/4] strbuf: introduce `strbuf_attachstr()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-04-21T18:47:00Z","receivedAt":"2020-04-21T18:47:15Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Mon, 20 Apr 2020 at 21:39, Junio C Hamano wrote:\n>\n> Martin Ågren writes:\n>\n> > +/**\n> > + * Attach a string to a buffer similar to `strbuf_attachstr_len()`.\n> > + * Useful if you do not know the length of the string.\n> > + */\n> > +static inline void strbuf_attachstr(struct strbuf *sb, char *str)\n> > +{\n> > + size_t len = strlen(str);\n> > +\n> > + strbuf_attach(sb, str, len, len + 1);\n> > +}\n>\n> This is somewhat worrysome in that the interface is _so_ simple that\n> people may fail to see that str must be allocated piece of memory,\n> and it is preferrable if string fully fills the allocation.\n>\n> We should repeat that (instead of just trusting \"similar to ...\"\n> would tell them enough) in the doc, perhaps?\n\nYeah, that's a good point. I'll expand on this to try to better get\nthrough that there are things to consider here.\n\n> > @@ -1095,7 +1095,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,\n> > for (ptr = trailer_lines; *ptr; ptr++) {\n> > if (last && isspace((*ptr)->buf[0])) {\n> > struct strbuf sb = STRBUF_INIT;\n> > - strbuf_attach(&sb, *last, strlen(*last), strlen(*last));\n> > + strbuf_attachstr(&sb, *last);\n> > strbuf_addbuf(&sb, *ptr);\n> > *last = strbuf_detach(&sb, NULL);\n> > continue;\n>\n> This is not wrong per-se, but it is unclear if use of strbuf_attach*\n> family to avoid an explicit malloc/copy/free is buying much at this\n> callsite. Simplifying the code here of course is not within the\n> scope of this series.\n\nFor the other patches in this series, I spent some time and effort\ninvestigating where strings came from, \"do I really feel certain that\nthey're NUL-terminated?\". But for this patch, I more or less went \"we've\nbeen using strlen on this all this time, surely if it wasn't guaranteed\nto be NUL-terminated we'd have messed up already\". And I don't think I'm\nmaking anything worse. But yeah, I didn't really step back to look at\nwhat these sites are really doing, and how, as much as I did for the\nothers.\n\n\nMartin\n"}]}