{"thread":{"id":"56243","subject":"[PATCH 0/3] some small range-diff read_patches() fixes","startedAt":"2021-08-09T22:45:47Z","lastAt":"2021-08-14T21:56:03Z","messageCount":6,"participants":["Jeff King","Derrick Stolee","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"432342","messageId":"YRGwGDvOte/o9vkd@coredump.intra.peff.net","threadId":"56243","inReplyTo":null,"subject":"[PATCH 0/3] some small range-diff read_patches() fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-09T22:45:44Z","receivedAt":"2021-08-09T22:45:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Amidst all the talk of clang4 in another thread, I noticed that Debian\nunstable recently shipped a clang-14 package. So I tried it out, and it\ndoes find one small cleanup. And then looking at the surrounding code\nhelped me find 2 more. :)\n\n  [1/3]: range-diff: drop useless \"offset\" variable from read_patches()\n  [2/3]: range-diff: handle unterminated lines in read_patches()\n  [3/3]: range-diff: use ssize_t for parsed \"len\" in read_patches()\n\n range-diff.c | 29 +++++++++++++----------------\n 1 file changed, 13 insertions(+), 16 deletions(-)\n\n-Peff\n"},{"id":"432343","messageId":"YRGwjgAIyLPb7g50@coredump.intra.peff.net","threadId":"56243","inReplyTo":"YRGwGDvOte/o9vkd@coredump.intra.peff.net","subject":"[PATCH 1/3] range-diff: drop useless \"offset\" variable from read_patches()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-09T22:47:42Z","receivedAt":"2021-08-09T22:47:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"offset\" variable was was introduced in 44b67cb62b (range-diff:\nsplit lines manually, 2019-07-11), but it has never done anything\nuseful. We use it to count up the number of bytes we've consumed, but we\nnever look at the result. It was probably copied accidentally from an\nalmost-identical loop in apply.c:find_header() (and the point of that\ncommit was to make use of the parse_git_diff_header() function which\nunderlies both).\n\nBecause the variable was set but not used, most compilers didn't seem to\nnotice, but the upcoming clang-14 does complain about it, via its\n-Wunused-but-set-variable warning.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe for-loop with an empty initializer and a doubled post-loop operation\nis a little funny to see. I didn't see an easy way to make it less ugly\n(pushing the line/size initialization into the for() would work, but the\nresulting line is awfully long).\n\n range-diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex e9479794b4..551600c774 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -49,7 +49,7 @@ static int read_patches(const char *range, struct string_list *list,\n \tstruct patch_util *util = NULL;\n \tint in_header = 1;\n \tchar *line, *current_filename = NULL;\n-\tint offset, len;\n+\tint len;\n \tsize_t size;\n \n \tstrvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\", \"--no-merges\",\n@@ -86,7 +86,7 @@ static int read_patches(const char *range, struct string_list *list,\n \n \tline = contents.buf;\n \tsize = contents.len;\n-\tfor (offset = 0; size > 0; offset += len, size -= len, line += len) {\n+\tfor (; size > 0; size -= len, line += len) {\n \t\tconst char *p;\n \n \t\tlen = find_end_of_line(line, size);\n-- \n2.33.0.rc1.475.g023efe0ae4\n\n"},{"id":"432344","messageId":"YRGwx5gkIXYyzMsm@coredump.intra.peff.net","threadId":"56243","inReplyTo":"YRGwGDvOte/o9vkd@coredump.intra.peff.net","subject":"[PATCH 2/3] range-diff: handle unterminated lines in read_patches()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-09T22:48:39Z","receivedAt":"2021-08-09T22:48:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When parsing our buffer of output from git-log, we have a\nfind_end_of_line() helper that finds the next newline, and gives us the\nnumber of bytes to move past it, or the size of the whole remaining\nbuffer if there is no newline.\n\nBut trying to handle both those cases leads to some oddities:\n\n  - we try to overwrite the newline with NUL in the caller, by writing\n    over line[len-1]. This is at best redundant, since the helper will\n    already have done so if it saw a newline. But if it didn't see a\n    newline, it's actively wrong; we'll overwrite the byte at the end of\n    the (unterminated) line.\n\n    We could solve this just dropping the extra NUL assignment in the\n    caller and just letting the helper do the right thing. But...\n\n  - if we see a \"diff --git\" line, we'll restore the newline on top of\n    the NUL byte, so we can pass the string to parse_git_diff_header().\n    But if there was no newline in the first place, we can't do this.\n    There's no place to put it (the current code writes a newline\n    over whatever byte we obliterated earlier). The best we can do is\n    feed the complete remainder of the buffer to the function (which is,\n    in fact, a string, by virtue of being a strbuf).\n\nTo solve this, the caller needs to know whether we actually found a\nnewline or not. We could modify find_end_of_line() to return that\ninformation, but we can further observe that it has only one caller.\nSo let's just inline it in that caller.\n\nNobody seems to have noticed this case, probably because git-log would\nnever produce input that doesn't end with a newline. Arguably we could\njust return an error as soon as we see that the output does not end in a\nnewline. But the code to do so actually ends up _longer_, mostly because\nof the cleanup we have to do in handling the error.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI had initially hoped to just delete the redundant line, but the details\nand the explanation got surprisingly tricky (especially for something\nthat we don't expect to see in practice anyway). Sorry. :)\n\n range-diff.c | 25 +++++++++++--------------\n 1 file changed, 11 insertions(+), 14 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 551600c774..088db1b1ce 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -26,17 +26,6 @@ struct patch_util {\n \tstruct object_id oid;\n };\n \n-static size_t find_end_of_line(char *buffer, unsigned long size)\n-{\n-\tchar *eol = memchr(buffer, '\\n', size);\n-\n-\tif (!eol)\n-\t\treturn size;\n-\n-\t*eol = '\\0';\n-\treturn eol + 1 - buffer;\n-}\n-\n /*\n  * Reads the patches into a string list, with the `util` field being populated\n  * as struct object_id (will need to be free()d).\n@@ -88,9 +77,16 @@ static int read_patches(const char *range, struct string_list *list,\n \tsize = contents.len;\n \tfor (; size > 0; size -= len, line += len) {\n \t\tconst char *p;\n+\t\tchar *eol;\n+\n+\t\teol = memchr(line, '\\n', size);\n+\t\tif (eol) {\n+\t\t\t*eol = '\\0';\n+\t\t\tlen = eol + 1 - line;\n+\t\t} else {\n+\t\t\tlen = size;\n+\t\t}\n \n-\t\tlen = find_end_of_line(line, size);\n-\t\tline[len - 1] = '\\0';\n \t\tif (skip_prefix(line, \"commit \", &p)) {\n \t\t\tif (util) {\n \t\t\t\tstring_list_append(list, buf.buf)->util = util;\n@@ -132,7 +128,8 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t\tstrbuf_addch(&buf, '\\n');\n \t\t\tif (!util->diff_offset)\n \t\t\t\tutil->diff_offset = buf.len;\n-\t\t\tline[len - 1] = '\\n';\n+\t\t\tif (eol)\n+\t\t\t\t*eol = '\\n';\n \t\t\torig_len = len;\n \t\t\tlen = parse_git_diff_header(&root, &linenr, 0, line,\n \t\t\t\t\t\t    len, size, &patch);\n-- \n2.33.0.rc1.475.g023efe0ae4\n\n"},{"id":"432345","messageId":"YRGw0FH3XciRGq5x@coredump.intra.peff.net","threadId":"56243","inReplyTo":"YRGwGDvOte/o9vkd@coredump.intra.peff.net","subject":"[PATCH 3/3] range-diff: use ssize_t for parsed \"len\" in read_patches()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-09T22:48:48Z","receivedAt":"2021-08-09T22:48:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As we iterate through the buffer containing git-log output, parsing\nlines, we use an \"int\" to store the size of an individual line. This\nshould be a size_t, as we have no guarantee that there is not a\nmalicious 2GB+ commit-message line in the output.\n\nOverflowing this integer probably doesn't do anything _too_ terrible. We\nare not using the value to size a buffer, so the worst case is probably\nan out-of-bounds read from before the array. But it's easy enough to\nfix.\n\nNote that we have to use ssize_t here, since we also store the length\nresult from parse_git_diff_header(), which may return a negative value\nfor error. That function actually returns an int itself, which has a\nsimilar overflow problem, but I'll leave that for another day. Much\nof the apply.c code uses ints and should be converted as a whole; in the\nmeantime, a negative return from parse_git_diff_header() will be\ninterpreted as an error, and we'll bail (so we can't handle such a case,\nbut given that it's likely to be malicious anyway, the important thing\nis we don't have any memory errors).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n range-diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 088db1b1ce..e731525e66 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -38,7 +38,7 @@ static int read_patches(const char *range, struct string_list *list,\n \tstruct patch_util *util = NULL;\n \tint in_header = 1;\n \tchar *line, *current_filename = NULL;\n-\tint len;\n+\tssize_t len;\n \tsize_t size;\n \n \tstrvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\", \"--no-merges\",\n-- \n2.33.0.rc1.475.g023efe0ae4\n"},{"id":"432380","messageId":"65b00c6a-ef88-29c3-157b-24ad9f4d8a8c@gmail.com","threadId":"56243","inReplyTo":"YRGwGDvOte/o9vkd@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] some small range-diff read_patches() fixes","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-10T14:47:38Z","receivedAt":"2021-08-10T14:47:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/9/2021 6:45 PM, Jeff King wrote:\n> Amidst all the talk of clang4 in another thread, I noticed that Debian\n> unstable recently shipped a clang-14 package. So I tried it out, and it\n> does find one small cleanup. And then looking at the surrounding code\n> helped me find 2 more. :)\n> \n>   [1/3]: range-diff: drop useless \"offset\" variable from read_patches()\n>   [2/3]: range-diff: handle unterminated lines in read_patches()\n>   [3/3]: range-diff: use ssize_t for parsed \"len\" in read_patches()\n\nI gave these a read. The code diffs are obviously correct and the\nexplanations are well motivated. Thanks.\n\n-Stolee\n"},{"id":"432749","messageId":"nycvar.QRO.7.76.6.2108142348390.59@tvgsbejvaqbjf.bet","threadId":"56243","inReplyTo":"65b00c6a-ef88-29c3-157b-24ad9f4d8a8c@gmail.com","subject":"Re: [PATCH 0/3] some small range-diff read_patches() fixes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-08-14T21:48:56Z","receivedAt":"2021-08-14T21:56:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 10 Aug 2021, Derrick Stolee wrote:\n\n> On 8/9/2021 6:45 PM, Jeff King wrote:\n> > Amidst all the talk of clang4 in another thread, I noticed that Debian\n> > unstable recently shipped a clang-14 package. So I tried it out, and it\n> > does find one small cleanup. And then looking at the surrounding code\n> > helped me find 2 more. :)\n> >\n> >   [1/3]: range-diff: drop useless \"offset\" variable from read_patches()\n> >   [2/3]: range-diff: handle unterminated lines in read_patches()\n> >   [3/3]: range-diff: use ssize_t for parsed \"len\" in read_patches()\n>\n> I gave these a read. The code diffs are obviously correct and the\n> explanations are well motivated. Thanks.\n\nSame here. Thanks, both,\nDscho\n"}]}