{"thread":{"id":"61070","subject":"[PATCH] wt-status: Don't find scissors line beyond buf len","startedAt":"2024-03-07T18:38:09Z","lastAt":"2024-04-06T01:37:31Z","messageCount":14,"participants":["Florian Schmidt","Junio C Hamano","Eric Sunshine","Kristoffer Haugsbakk","Linus Arver"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"490200","messageId":"20240307183743.219951-1-flosch@nutanix.com","threadId":"61070","inReplyTo":null,"subject":"[PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Florian Schmidt","fromEmail":"flosch@nutanix.com","sentAt":"2024-03-07T18:37:38Z","receivedAt":"2024-03-07T18:38:09Z","isPatch":true,"sender":{"key":"flosch@nutanix.com","avatar":"https://avatars.githubusercontent.com/u/25348638?v=4"},"body":"Currently, if\n(a) There is a \"---\" divider in a commit message,\n(b) At some point beyond that divider, there is a cut-line (that is,\n \"# ------------------------ >8 ------------------------\") in the\n commit message,\n(c) the user does not explicitly set the \"no-divider\" option,\nthen \"git interpret-trailers\" will hang indefinitively.\n\nThis is because when (a) is true, find_end_of_log_message() will invoke\nignored_log_message_bytes() with a len that is intended to make it\nignore the part of the commit message beyond the divider. However,\nignored_log_message_bytes() calls wt_status_locate_end(), and that\nfunction ignores the length restriction when it tries to locate the cut\nline. If it manages to find one, the returned cutoff value is greater\nthan len. At this point, ignored_log_message_bytes() goes into an\ninfinite loop, because it won't advance the string parsing beyond len,\nbut the exit condition expects to reach cutoff.\n\nIt seems sensible to expect that wt_status_locate_end() should honour\nthe length parameter passed in, and doing so fixes this issue.\n\nSigned-off-by: Florian Schmidt \nReviewed-by: Jonathan Davies \n---\n\nSide remark: Since strstr() doesn't consider len, and will always search\nup to a null byte, I now wonder whether it would be safer to create a\nnew strbuf that only contains the len bytes we want to operate on. If\nanybody ever thinks they can pass a non-null-terminated string into\nwt_status_locate_end() because they already provide a len parameter,\nthey will not have a good time. So it's that traded off against the\nslightly higher overhead of creating yet another buffer and copying a\npotentially large-ish commit message around.\n\n\n t/t7513-interpret-trailers.sh | 14 ++++++++++++++\n wt-status.c | 13 +++++++++----\n 2 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex ec9c6de114..3d3e13ccf8 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -1935,4 +1935,18 @@ test_expect_success 'suppressing --- does not disable cut-line handling' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'handling of --- lines in conjunction with cut-lines' '\n+\techo \"my-trailer: here\" >expected &&\n+\n+\tgit interpret-trailers --parse >actual <<-\\EOF &&\n+\tsubject\n+\n+\tmy-trailer: here\n+\t---\n+\t# ------------------------ >8 ------------------------\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex b5a29083df..51a84575ed 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1089,14 +1089,19 @@ size_t wt_status_locate_end(const char *s, size_t len)\n {\n \tconst char *p;\n \tstruct strbuf pattern = STRBUF_INIT;\n+\tsize_t result = len;\n \n \tstrbuf_addf(&pattern, \"\\n%c %s\", comment_line_char, cut_line);\n \tif (starts_with(s, pattern.buf + 1))\n-\t\tlen = 0;\n-\telse if ((p = strstr(s, pattern.buf)))\n-\t\tlen = p - s + 1;\n+\t\tresult = 0;\n+\telse if ((p = strstr(s, pattern.buf))) {\n+\t\tresult = p - s + 1;\n+\t\tif (result > len) {\n+\t\t\tresult = len;\n+\t\t}\n+\t}\n \tstrbuf_release(&pattern);\n-\treturn len;\n+\treturn result;\n }\n \n void wt_status_append_cut_line(struct strbuf *buf)\n-- \n2.42.0\n\n"},{"id":"490201","messageId":"xmqq34t1n91w.fsf@gitster.g","threadId":"61070","inReplyTo":"20240307183743.219951-1-flosch@nutanix.com","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-07T19:20:11Z","receivedAt":"2024-03-07T19:20:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Florian Schmidt writes:\n\n> Currently, if\n> (a) There is a \"---\" divider in a commit message,\n> (b) At some point beyond that divider, there is a cut-line (that is,\n> \"# ------------------------ >8 ------------------------\") in the\n> commit message,\n> (c) the user does not explicitly set the \"no-divider\" option,\n> then \"git interpret-trailers\" will hang indefinitively.\n\nYou do not have to say \"Currently, if\"; just \"If\" is sufficient.\nCf. Documentation/SubmittingPatches[[present-tense]]\n\n> This is because when (a) is true, find_end_of_log_message() will invoke\n> ignored_log_message_bytes() with a len that is intended to make it\n> ignore the part of the commit message beyond the divider. However,\n> ignored_log_message_bytes() calls wt_status_locate_end(), and that\n> function ignores the length restriction when it tries to locate the cut\n> line. If it manages to find one, the returned cutoff value is greater\n> than len. At this point, ignored_log_message_bytes() goes into an\n> infinite loop, because it won't advance the string parsing beyond len,\n> but the exit condition expects to reach cutoff.\n\nGood finding. \n\n> It seems sensible to expect that wt_status_locate_end() should honour\n> the length parameter passed in, and doing so fixes this issue.\n\nThanks. This is an ancient bug, not a retression from recent\nchanges to the trailer library [linusa CC'ed to save him from\nwasting his time wondering if he broke anything].\n\n> diff --git a/wt-status.c b/wt-status.c\n> index b5a29083df..51a84575ed 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1089,14 +1089,19 @@ size_t wt_status_locate_end(const char *s, size_t len)\n> {\n> \tconst char *p;\n> \tstruct strbuf pattern = STRBUF_INIT;\n> +\tsize_t result = len;\n> \n> \tstrbuf_addf(&pattern, \"\\n%c %s\", comment_line_char, cut_line);\n> \tif (starts_with(s, pattern.buf + 1))\n> -\t\tlen = 0;\n> -\telse if ((p = strstr(s, pattern.buf)))\n> -\t\tlen = p - s + 1;\n> +\t\tresult = 0;\n> +\telse if ((p = strstr(s, pattern.buf))) {\n> +\t\tresult = p - s + 1;\n> +\t\tif (result > len) {\n> +\t\t\tresult = len;\n> +\t\t}\n> +\t}\n> \tstrbuf_release(&pattern);\n> -\treturn len;\n> +\treturn result;\n> }\n\nLooks correct, but we probably can make the fix a lot more isolated\ninto a single block, like the attached patch. How does this look?\n\n wt-status.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git c/wt-status.c w/wt-status.c\nindex b5a29083df..511f37cfe0 100644\n--- c/wt-status.c\n+++ w/wt-status.c\n@@ -1093,8 +1093,11 @@ size_t wt_status_locate_end(const char *s, size_t len)\n \tstrbuf_addf(&pattern, \"\\n%c %s\", comment_line_char, cut_line);\n \tif (starts_with(s, pattern.buf + 1))\n \t\tlen = 0;\n-\telse if ((p = strstr(s, pattern.buf)))\n-\t\tlen = p - s + 1;\n+\telse if ((p = strstr(s, pattern.buf))) {\n+\t\tint newlen = p - s + 1;\n+\t\tif (newlen < len)\n+\t\t\tlen = newlen;\n+\t}\n \tstrbuf_release(&pattern);\n \treturn len;\n }\n"},{"id":"490202","messageId":"xmqqsf11ltrt.fsf@gitster.g","threadId":"61070","inReplyTo":"20240307183743.219951-1-flosch@nutanix.com","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-07T19:35:34Z","receivedAt":"2024-03-07T19:35:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Florian Schmidt writes:\n\n> Side remark: Since strstr() doesn't consider len, and will always search\n> up to a null byte, I now wonder whether it would be safer to create a\n> new strbuf that only contains the len bytes we want to operate on.\n\nThat is a valid concern in general, but does not seem to apply to\nthe current codebase. Thanks for being careful.\n\nTwo of the three callers of wt_status_locate_end() feed the pointer\ninto a piece of memory that is owned by strbuf, which guarantees\nthat the memory has an extra NUL to terminate it as a string even if\nyou did\n\n\tstrbuf buf = STRBUF_INIT;\n\tstrbuf_addch(&buf, 'A');\n\nThe other one is in commit.c:ignored_log_message_bytes() that still\ntakes as input, but again, two of its three callers call\nit with a pointer that points at the beginning of memory held by an\ninstance of strbuf.\n\nThat leaves us trailer.c:find_end_of_log_message() the only one to\nworry about, but it uses strlen() on the pointer before calling\nignored_log_message_bytes() so the region of the memory pointed at\nby the pointer is assumed to be NUL-terminated already, and\npresumably (I didn't follow the logic there too closely) the length\nis also computed within that NUL-terminated string.\n\n\n"},{"id":"490216","messageId":"xmqq7cidlqg5.fsf@gitster.g","threadId":"61070","inReplyTo":"xmqq34t1n91w.fsf@gitster.g","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-07T20:47:22Z","receivedAt":"2024-03-07T20:47:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Florian Schmidt \nDate: Thu, 7 Mar 2024 18:37:38 +0000\nSubject: [PATCH] wt-status: don't find scissors line beyond buf len\n\nIf\n\n (a) There is a \"---\" divider in a commit message,\n\n (b) At some point beyond that divider, there is a cut-line (that is,\n \"# ------------------------ >8 ------------------------\") in the\n commit message,\n\n (c) the user does not explicitly set the \"no-divider\" option,\n\nthen \"git interpret-trailers\" will hang indefinitively.\n\nThis is because when (a) is true, find_end_of_log_message() will invoke\nignored_log_message_bytes() with a len that is intended to make it\nignore the part of the commit message beyond the divider. However,\nignored_log_message_bytes() calls wt_status_locate_end(), and that\nfunction ignores the length restriction when it tries to locate the cut\nline. If it manages to find one, the returned cutoff value is greater\nthan len. At this point, ignored_log_message_bytes() goes into an\ninfinite loop, because it won't advance the string parsing beyond len,\nbut the exit condition expects to reach cutoff.\n\nMake wt_status_locate_end() honor the length parameter passed in, to\nfix this issue.\n\nIn general, if wt_status_locate_end() is given a piece of the memory\nthat lacks NUL at all, strstr() may continue across page boundaries\nand run into an unmapped page. For our current callers, this is not\na problem, as all of them except one uses a memory owned by a strbuf\n(which guarantees an implicit NUL-termination after its payload),\nand the one exeption in trailer.c:find_end_of_log_message() uses\nstrlen() to compute the length before calling this function.\n\nSigned-off-by: Florian Schmidt \nReviewed-by: Jonathan Davies \n[jc: tweaked the commit log message and the implementation a bit]\nSigned-off-by: Junio C Hamano \n---\n\n * So here is the version I queued. I have a new paragraph at the\n end of the log message to talk about use of strstr() and how it\n is OK in the current codebase.\n\n t/t7513-interpret-trailers.sh | 14 ++++++++++++++\n wt-status.c | 7 +++++--\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 6602790b5f..5efe70d675 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -1476,4 +1476,18 @@ test_expect_success 'suppress --- handling' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'handling of --- lines in conjunction with cut-lines' '\n+\techo \"my-trailer: here\" >expected &&\n+\n+\tgit interpret-trailers --parse >actual <<-\\EOF &&\n+\tsubject\n+\n+\tmy-trailer: here\n+\t---\n+\t# ------------------------ >8 ------------------------\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 40b59be478..16c1b9b7ee 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1007,8 +1007,11 @@ size_t wt_status_locate_end(const char *s, size_t len)\n \tstrbuf_addf(&pattern, \"\\n%c %s\", comment_line_char, cut_line);\n \tif (starts_with(s, pattern.buf + 1))\n \t\tlen = 0;\n-\telse if ((p = strstr(s, pattern.buf)))\n-\t\tlen = p - s + 1;\n+\telse if ((p = strstr(s, pattern.buf))) {\n+\t\tsize_t newlen = p - s + 1;\n+\t\tif (newlen < len)\n+\t\t\tlen = newlen;\n+\t}\n \tstrbuf_release(&pattern);\n \treturn len;\n }\n-- \n2.44.0-117-g43072b4ca1\n\n"},{"id":"490221","messageId":"CAPig+cTgusL7OH=5DJY9ef4YuLw5WBKgDFcbSu=QKFjjkforkw@mail.gmail.com","threadId":"61070","inReplyTo":"xmqq7cidlqg5.fsf@gitster.g","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-07T21:09:15Z","receivedAt":"2024-03-07T21:09:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 7, 2024 at 3:47 PM Junio C Hamano wrote:\n> * So here is the version I queued. I have a new paragraph at the\n> end of the log message to talk about use of strstr() and how it\n> is OK in the current codebase.\n> [jc: tweaked the commit log message and the implementation a bit]\n>\n> From: Florian Schmidt \n>\n> In general, if wt_status_locate_end() is given a piece of the memory\n> that lacks NUL at all, strstr() may continue across page boundaries\n> and run into an unmapped page. For our current callers, this is not\n> a problem, as all of them except one uses a memory owned by a strbuf\n> (which guarantees an implicit NUL-termination after its payload),\n> and the one exeption in trailer.c:find_end_of_log_message() uses\n> strlen() to compute the length before calling this function.\n\ns/exeption/exception/\n"},{"id":"490223","messageId":"f8de2b3a-9e12-49fe-a7d9-481317f10c4d@app.fastmail.com","threadId":"61070","inReplyTo":"xmqq7cidlqg5.fsf@gitster.g","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-07T21:15:12Z","receivedAt":"2024-03-07T21:15:35Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Thu, Mar 7, 2024, at 21:47, Junio C Hamano wrote:\n> Signed-off-by: Florian Schmidt \n> Reviewed-by: Jonathan Davies \n> [jc: tweaked the commit log message and the implementation a bit]\n\nJust a question. Given the imperative mood principle/rule, why are these\nbracket changelog lines always written in the past tense?\n\nCheers\n\n-- \nKristoffer Haugsbakk\n\n\n"},{"id":"490224","messageId":"xmqqo7bpka6e.fsf@gitster.g","threadId":"61070","inReplyTo":"f8de2b3a-9e12-49fe-a7d9-481317f10c4d@app.fastmail.com","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-07T21:24:09Z","receivedAt":"2024-03-07T21:24:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Thu, Mar 7, 2024, at 21:47, Junio C Hamano wrote:\n>> Signed-off-by: Florian Schmidt \n>> Reviewed-by: Jonathan Davies \n>> [jc: tweaked the commit log message and the implementation a bit]\n>\n> Just a question. Given the imperative mood principle/rule, why are these\n> bracket changelog lines always written in the past tense?\n\nThese are not giving orders to the code to become like so. The\ntrailer block records what happend to the patch in chronological\norder---think of those written there at one level higher level,\n\"meta\" comments.\n"},{"id":"490225","messageId":"CAPig+cRNa22A=fEmc__JvEjYiVF-QG8o7w0gukhbeL3e-PwVkA@mail.gmail.com","threadId":"61070","inReplyTo":"xmqqo7bpka6e.fsf@gitster.g","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-07T21:26:36Z","receivedAt":"2024-03-07T21:26:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 7, 2024 at 4:24 PM Junio C Hamano wrote:\n> \"Kristoffer Haugsbakk\" writes:\n> > On Thu, Mar 7, 2024, at 21:47, Junio C Hamano wrote:\n> >> [jc: tweaked the commit log message and the implementation a bit]\n> >\n> > Just a question. Given the imperative mood principle/rule, why are these\n> > bracket changelog lines always written in the past tense?\n>\n> These are not giving orders to the code to become like so. The\n> trailer block records what happend to the patch in chronological\n> order---think of those written there at one level higher level,\n> \"meta\" comments.\n\nAlso, they are not always written in past tense[*].\n\n[*]: https://lore.kernel.org/git/20240112171910.11131-1-ericsunshine@charter.net/\n"},{"id":"490226","messageId":"6acdc464-825e-48bd-a2d6-97671b58f879@app.fastmail.com","threadId":"61070","inReplyTo":"CAPig+cRNa22A=fEmc__JvEjYiVF-QG8o7w0gukhbeL3e-PwVkA@mail.gmail.com","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-07T21:30:33Z","receivedAt":"2024-03-07T21:31:05Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Thu, Mar 7, 2024 at 4:24 PM Junio C Hamano wrote:\n>> \"Kristoffer Haugsbakk\" writes:\n>> > On Thu, Mar 7, 2024, at 21:47, Junio C Hamano wrote:\n>> >> [jc: tweaked the commit log message and the implementation a bit]\n>> >\n>> > Just a question. Given the imperative mood principle/rule, why are these\n>> > bracket changelog lines always written in the past tense?\n>>\n>> These are not giving orders to the code to become like so. The\n>> trailer block records what happend to the patch in chronological\n>> order---think of those written there at one level higher level,\n>> \"meta\" comments.\n\nAnd when I think about it the trailers themselves are of course in the\npast tense…\n\nThanks guys\n\nOn Thu, Mar 7, 2024, at 22:26, Eric Sunshine wrote:\n>\n> Also, they are not always written in past tense[*].\n>\n> [*]:\n> https://lore.kernel.org/git/20240112171910.11131-1-ericsunshine@charter.net/\n\n-- \nKristoffer Haugsbakk\n"},{"id":"490234","messageId":"1ff36e64-b993-4cbb-ba0a-01aca5396ef6@nutanix.com","threadId":"61070","inReplyTo":"xmqq34t1n91w.fsf@gitster.g","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Florian Schmidt","fromEmail":"flosch@nutanix.com","sentAt":"2024-03-08T09:08:50Z","receivedAt":"2024-03-08T09:09:08Z","isPatch":true,"sender":{"key":"flosch@nutanix.com","avatar":"https://avatars.githubusercontent.com/u/25348638?v=4"},"body":"That's a quick review cycle, thanks!\n\nOn 07/03/2024 19:20, Junio C Hamano wrote:\n> You do not have to say \"Currently, if\"; just \"If\" is sufficient.\n> Cf. Documentation/SubmittingPatches[[present-tense]]\n\nack\n\n> Looks correct, but we probably can make the fix a lot more isolated\n> into a single block, like the attached patch. How does this look?\n> \n> wt-status.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git c/wt-status.c w/wt-status.c\n> index b5a29083df..511f37cfe0 100644\n> --- c/wt-status.c\n> +++ w/wt-status.c\n> @@ -1093,8 +1093,11 @@ size_t wt_status_locate_end(const char *s, size_t len)\n> \tstrbuf_addf(&pattern, \"\\n%c %s\", comment_line_char, cut_line);\n> \tif (starts_with(s, pattern.buf + 1))\n> \t\tlen = 0;\n> -\telse if ((p = strstr(s, pattern.buf)))\n> -\t\tlen = p - s + 1;\n> +\telse if ((p = strstr(s, pattern.buf))) {\n> +\t\tint newlen = p - s + 1;\n> +\t\tif (newlen < len)\n> +\t\t\tlen = newlen;\n> +\t}\n> \tstrbuf_release(&pattern);\n> \treturn len;\n> }\n\nThat looks good to me, thanks! For context, I had sent in the slightly \nlarger patch because at first look, I got confused by the fact that \nwt_status_locate_end takes len as a parameter, but then seemingly \ndoesn't read it at all and only writes to it. (Of course, if neither the \nif nor the if-else branch are hit, len is in fact read when it is \nreturned unchanged.) So I made the patch a bit larger in the hope that \nthe next cursory reader might not get confused.\nBut this option is a much more minimal patch, and functionally the same. \nSo it comes down to style, and you have a much better feeling for that \nin this code base, so I'm happy to go with this.\n\nDo you want me to send this version as a v2 to you + the list as per the \ndocumentation?\n\nCheers,\nflosch\n"},{"id":"490235","messageId":"d280a87b-e6ab-4f0d-b112-bbedc223c9fd@nutanix.com","threadId":"61070","inReplyTo":"xmqqsf11ltrt.fsf@gitster.g","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Florian Schmidt","fromEmail":"flosch@nutanix.com","sentAt":"2024-03-08T09:13:12Z","receivedAt":"2024-03-08T09:13:23Z","isPatch":true,"sender":{"key":"flosch@nutanix.com","avatar":"https://avatars.githubusercontent.com/u/25348638?v=4"},"body":"On 07/03/2024 19:35, Junio C Hamano wrote:\n> Florian Schmidt writes:\n> \n>> Side remark: Since strstr() doesn't consider len, and will always search\n>> up to a null byte, I now wonder whether it would be safer to create a\n>> new strbuf that only contains the len bytes we want to operate on.\n> \n> That is a valid concern in general, but does not seem to apply to\n> the current codebase. Thanks for being careful.\n\nThanks, that confirms my cursory look at the consumers of the function. \nIf you think that it's unlikely that in the future, a new user of this \nfunction would provide a non-terminated string, then there is no need \nfor action. I guess the aim is to use strbufs wherever suitable in the \nfirst place, anyway, and those won't have this issue?\n\nCheers,\nflosch\n"},{"id":"490250","messageId":"xmqqedckiw2r.fsf@gitster.g","threadId":"61070","inReplyTo":"1ff36e64-b993-4cbb-ba0a-01aca5396ef6@nutanix.com","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-08T15:26:20Z","receivedAt":"2024-03-08T15:26:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Florian Schmidt writes:\n\n> Do you want me to send this version as a v2 to you + the list as per\n> the documentation?\n\nIt would be the technically correct way to do so, but as a short-cut\nto reduce a round-trip, if you are happy with the version I queued\n(should be found by fetching the 'seen' branch from any of the\nmirrors), you can just say \"that looks fine\" and we can be done with\nthis patch.\n\nThanks.\n"},{"id":"490262","messageId":"08b9b37d-f0f8-4c1a-b72e-194202ff3d9f@nutanix.com","threadId":"61070","inReplyTo":"xmqqedckiw2r.fsf@gitster.g","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Florian Schmidt","fromEmail":"flosch@nutanix.com","sentAt":"2024-03-08T17:43:42Z","receivedAt":"2024-03-08T17:44:02Z","isPatch":true,"sender":{"key":"flosch@nutanix.com","avatar":"https://avatars.githubusercontent.com/u/25348638?v=4"},"body":"On 08/03/2024 15:26, Junio C Hamano wrote:\n> It would be the technically correct way to do so, but as a short-cut\n> to reduce a round-trip, if you are happy with the version I queued\n> (should be found by fetching the 'seen' branch from any of the\n> mirrors), you can just say \"that looks fine\" and we can be done with\n> this patch.\n\nI had a look, and it looks fine (though there's the one typo already \npointed out by Eric: s/exeption/exception/). You can go ahead and use \nthat version of the patch.\n\nCheers,\nflosch\n"},{"id":"492359","messageId":"owlyo7ans23q.fsf@fine.c.googlers.com","threadId":"61070","inReplyTo":"xmqq34t1n91w.fsf@gitster.g","subject":"Re: [PATCH] wt-status: Don't find scissors line beyond buf len","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-04-06T01:37:29Z","receivedAt":"2024-04-06T01:37:31Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Junio C Hamano writes:\n\n> Florian Schmidt writes:\n>\n>> It seems sensible to expect that wt_status_locate_end() should honour\n>> the length parameter passed in, and doing so fixes this issue.\n>\n> Thanks. This is an ancient bug, not a retression from recent\n> changes to the trailer library [linusa CC'ed to save him from\n> wasting his time wondering if he broke anything].\n\nAck. Very much appreciated!\n\nAlso, sorry for not responding sooner --- my email search queries so far\nwere using \"replied\" as a condition so I only saw this just now while\nsearching for the \"trailer\" term.\n\nCheers\n"}]}