{"thread":{"id":"58393","subject":"[PATCH] unleak paths allocated in \"diff --no-index\"","startedAt":"2022-09-02T21:27:34Z","lastAt":"2022-09-07T19:37:12Z","messageCount":17,"participants":["Junio C Hamano","René Scharfe","Johannes Schindelin","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"462559","messageId":"xmqqilm579hc.fsf@gitster.g","threadId":"58393","inReplyTo":null,"subject":"[PATCH] unleak paths allocated in \"diff --no-index\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-02T21:27:27Z","receivedAt":"2022-09-02T21:27:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git diff --no-index\" codepath starts with the two elements in\nargv[] and munges them into two paths to be compared, stored in a\nseparate path[] arrays.  The munging is implemented in a rather\nhaphazard way, sometimes overwriting old version with a new copy,\nand sometimes a constant string assigned to path[], making it\nimpossible to release the resources properly:\n\n * A single dash \"-\" from the command line is a special signal that\n   the standard input is used for the side to be compared, and is\n   internally replaced with a copy of string \"-\" at a known address.\n\n * When run in a subdirectory, full paths to the two paths are\n   allocated and placed in path[].\n\n * After the above happens, when comparing a file with a directory,\n   the directory side is replaced with the path to a file in the\n   directory with the same name as the file.\n\nThis was perfectly fine for just two strings that are pathnames used\nduring the lifetime of the program and cleaned up upon program exit,\nbut it gets in the way when leak sanitizer is in effect.  The third\nstep can be losing the full path that was allocated in the second\nstep, but it is not easy to tell if its input is an allocated piece\nof memory to begin with.\n\nLoosen the earlier two steps a bit so that elements of the path[]\narray that come to the directory/file comparison code are either the\nsingleton \"-\" or an allocated piece of memory.  Use that knowledge\nin the third step to release an allocated piece of memory when it\nreplaces the path to a directory with the path to a file in that\ndirectory, and also at the end to release the two elements of the\npath[] array as needed.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Because we never pass NULL to the new helper, !path check is not\n   strictly necessary, but it is conventional for free()-like\n   functions to take NULL and safely become a no-op.\n\n diff-no-index.c | 19 ++++++++++++++++++-\n 1 file changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git c/diff-no-index.c w/diff-no-index.c\nindex 9a8b09346b..2770b7d15a 100644\n--- c/diff-no-index.c\n+++ w/diff-no-index.c\n@@ -208,6 +208,14 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi\n \tstrbuf_addstr(path, tail ? tail + 1 : file);\n }\n \n+static void free_allocated_path(const char *path)\n+{\n+\tif (!path ||\n+\t    (path == file_from_standard_input))\n+\t\treturn;\n+\tfree((char *)path);\n+}\n+\n /*\n  * DWIM \"diff D F\" into \"diff D/F F\" and \"diff F D\" into \"diff F D/F\"\n  * Note that we append the basename of F to D/, so \"diff a/b/file D\"\n@@ -226,9 +234,11 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n \t\treturn;\n \tif (isdir0) {\n \t\tappend_basename(replacement, path[0], path[1]);\n+\t\tfree_allocated_path(path[0]);\n \t\tpath[0] = replacement->buf;\n \t} else {\n \t\tappend_basename(replacement, path[1], path[0]);\n+\t\tfree_allocated_path(path[1]);\n \t\tpath[1] = replacement->buf;\n \t}\n }\n@@ -274,6 +284,8 @@ int diff_no_index(struct rev_info *revs,\n \t\t\tp = file_from_standard_input;\n \t\telse if (prefix)\n \t\t\tp = prefix_filename(prefix, p);\n+\t\telse\n+\t\t\tp = xstrdup(p);\n \t\tpaths[i] = p;\n \t}\n \n@@ -294,13 +306,18 @@ int diff_no_index(struct rev_info *revs,\n \tsetup_diff_pager(&revs->diffopt);\n \trevs->diffopt.flags.exit_with_status = 1;\n \n-\tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n+\tif (queue_diff(&revs->diffopt, paths[0], paths[1])) {\n+\t\tfree_allocated_path(paths[0]);\n+\t\tfree_allocated_path(paths[1]);\n \t\treturn 1;\n+\t}\n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n \n \tstrbuf_release(&replacement);\n+\tfree_allocated_path(paths[0]);\n+\tfree_allocated_path(paths[1]);\n \n \t/*\n \t * The return code for --no-index imitates diff(1):\n"},{"id":"462563","messageId":"xmqqilm51gn6.fsf@gitster.g","threadId":"58393","inReplyTo":"xmqqilm579hc.fsf@gitster.g","subject":"[PATCH v2] diff --no-index: unleak paths[] elements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-02T23:49:17Z","receivedAt":"2022-09-02T23:49:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git diff --no-index\" codepath starts with the two elements in\nargv[] and munges them into two paths to be compared, stored in a\nseparate path[] arrays.  The munging is implemented in a rather\nhaphazard way, sometimes overwriting old version with a new copy,\nand sometimes a constant string assigned to path[], making it\nimpossible to release the resources properly:\n\n * A single dash \"-\" from the command line is a special signal that\n   the standard input is used for the side to be compared, and is\n   internally replaced with a copy of string \"-\" at a known address.\n\n * When run in a subdirectory, full paths to the two paths are\n   allocated and placed in path[].\n\n * After the above happens, when comparing a file with a directory,\n   the directory side is replaced with the path to a file in the\n   directory with the same name as the file.\n\nThis was perfectly fine for just two strings that are pathnames used\nduring the lifetime of the program and cleaned up upon program exit,\nbut it gets in the way when leak sanitizer is in effect.  The third\nstep can be losing the full path that was allocated in the second\nstep, but it is not easy to tell if its input is an allocated piece\nof memory to begin with.\n\nLoosen the earlier two steps a bit so that elements of the path[]\narray that come to the directory/file comparison code are either the\nsingleton \"-\" or an allocated piece of memory.  Use that knowledge\nin the third step to release an allocated piece of memory when it\nreplaces the path to a directory with the path to a file in that\ndirectory, and also at the end to release the two elements of the\npath[] array as needed.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * The previous one allowed strbuf_release() to free replacement.buf\n   which may be used in path[0] or path[1] potentially leading to\n   double freeing.  The kosher way may be to use strbuf_detach() in\n   fixup_paths(), but this is a simpler fix, it is getting late in\n   the day, and I am getting sick of fighting the leak-checker, so...\n\n diff-no-index.c | 24 ++++++++++++++++++++++--\n 1 file changed, 22 insertions(+), 2 deletions(-)\n\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 9a8b09346b..77a126469b 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -208,6 +208,14 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi\n \tstrbuf_addstr(path, tail ? tail + 1 : file);\n }\n \n+static void free_allocated_path(const char *path)\n+{\n+\tif (!path ||\n+\t    (path == file_from_standard_input))\n+\t\treturn;\n+\tfree((char *)path);\n+}\n+\n /*\n  * DWIM \"diff D F\" into \"diff D/F F\" and \"diff F D\" into \"diff F D/F\"\n  * Note that we append the basename of F to D/, so \"diff a/b/file D\"\n@@ -226,9 +234,11 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n \t\treturn;\n \tif (isdir0) {\n \t\tappend_basename(replacement, path[0], path[1]);\n+\t\tfree_allocated_path(path[0]);\n \t\tpath[0] = replacement->buf;\n \t} else {\n \t\tappend_basename(replacement, path[1], path[0]);\n+\t\tfree_allocated_path(path[1]);\n \t\tpath[1] = replacement->buf;\n \t}\n }\n@@ -274,6 +284,8 @@ int diff_no_index(struct rev_info *revs,\n \t\t\tp = file_from_standard_input;\n \t\telse if (prefix)\n \t\t\tp = prefix_filename(prefix, p);\n+\t\telse\n+\t\t\tp = xstrdup(p);\n \t\tpaths[i] = p;\n \t}\n \n@@ -294,13 +306,21 @@ int diff_no_index(struct rev_info *revs,\n \tsetup_diff_pager(&revs->diffopt);\n \trevs->diffopt.flags.exit_with_status = 1;\n \n-\tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n+\tif (queue_diff(&revs->diffopt, paths[0], paths[1])) {\n+\t\tfree_allocated_path(paths[0]);\n+\t\tfree_allocated_path(paths[1]);\n \t\treturn 1;\n+\t}\n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n \n-\tstrbuf_release(&replacement);\n+\t/*\n+\t * do not strbuf_release(&replacement), as it is in paths[]\n+\t * when replacement was actually used.\n+\t */\n+\tfree_allocated_path(paths[0]);\n+\tfree_allocated_path(paths[1]);\n \n \t/*\n \t * The return code for --no-index imitates diff(1):\n-- \n2.37.3-661-g73a641a77a\n\n"},{"id":"462576","messageId":"181c029b-8b36-4b04-30f9-97a3f252bfbc@web.de","threadId":"58393","inReplyTo":"xmqqilm51gn6.fsf@gitster.g","subject":"Re: [PATCH v2] diff --no-index: unleak paths[] elements","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-03T06:00:23Z","receivedAt":"2022-09-03T06:00:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.09.22 um 01:49 schrieb Junio C Hamano:\n> \"git diff --no-index\" codepath starts with the two elements in\n> argv[] and munges them into two paths to be compared, stored in a\n> separate path[] arrays.  The munging is implemented in a rather\n> haphazard way, sometimes overwriting old version with a new copy,\n> and sometimes a constant string assigned to path[], making it\n> impossible to release the resources properly:\n>\n>  * A single dash \"-\" from the command line is a special signal that\n>    the standard input is used for the side to be compared, and is\n>    internally replaced with a copy of string \"-\" at a known address.\n>\n>  * When run in a subdirectory, full paths to the two paths are\n>    allocated and placed in path[].\n>\n>  * After the above happens, when comparing a file with a directory,\n>    the directory side is replaced with the path to a file in the\n>    directory with the same name as the file.\n>\n> This was perfectly fine for just two strings that are pathnames used\n> during the lifetime of the program and cleaned up upon program exit,\n> but it gets in the way when leak sanitizer is in effect.  The third\n> step can be losing the full path that was allocated in the second\n> step, but it is not easy to tell if its input is an allocated piece\n> of memory to begin with.\n>\n> Loosen the earlier two steps a bit so that elements of the path[]\n> array that come to the directory/file comparison code are either the\n> singleton \"-\" or an allocated piece of memory.  Use that knowledge\n> in the third step to release an allocated piece of memory when it\n> replaces the path to a directory with the path to a file in that\n> directory, and also at the end to release the two elements of the\n> path[] array as needed.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  * The previous one allowed strbuf_release() to free replacement.buf\n>    which may be used in path[0] or path[1] potentially leading to\n>    double freeing.  The kosher way may be to use strbuf_detach() in\n>    fixup_paths(), but this is a simpler fix, it is getting late in\n>    the day, and I am getting sick of fighting the leak-checker, so...\n>\n>  diff-no-index.c | 24 ++++++++++++++++++++++--\n>  1 file changed, 22 insertions(+), 2 deletions(-)\n>\n>\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 9a8b09346b..77a126469b 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -208,6 +208,14 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi\n>  \tstrbuf_addstr(path, tail ? tail + 1 : file);\n>  }\n>\n> +static void free_allocated_path(const char *path)\n> +{\n> +\tif (!path ||\n\nHow can path be NULL?  And if it was, why shield free(3) from it?\n\n> +\t    (path == file_from_standard_input))\n> +\t\treturn;\n> +\tfree((char *)path);\n> +}\n> +\n>  /*\n>   * DWIM \"diff D F\" into \"diff D/F F\" and \"diff F D\" into \"diff F D/F\"\n>   * Note that we append the basename of F to D/, so \"diff a/b/file D\"\n> @@ -226,9 +234,11 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n>  \t\treturn;\n>  \tif (isdir0) {\n>  \t\tappend_basename(replacement, path[0], path[1]);\n> +\t\tfree_allocated_path(path[0]);\n>  \t\tpath[0] = replacement->buf;\n>  \t} else {\n>  \t\tappend_basename(replacement, path[1], path[0]);\n> +\t\tfree_allocated_path(path[1]);\n>  \t\tpath[1] = replacement->buf;\n>  \t}\n>  }\n> @@ -274,6 +284,8 @@ int diff_no_index(struct rev_info *revs,\n>  \t\t\tp = file_from_standard_input;\n>  \t\telse if (prefix)\n>  \t\t\tp = prefix_filename(prefix, p);\n> +\t\telse\n> +\t\t\tp = xstrdup(p);\n\nprefix_filename(NULL, p) is basically the same as xstrdup(p), so those\ntwo conditional branches could be joined.\n\n>  \t\tpaths[i] = p;\n>  \t}\n>\n> @@ -294,13 +306,21 @@ int diff_no_index(struct rev_info *revs,\n>  \tsetup_diff_pager(&revs->diffopt);\n>  \trevs->diffopt.flags.exit_with_status = 1;\n>\n> -\tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n> +\tif (queue_diff(&revs->diffopt, paths[0], paths[1])) {\n> +\t\tfree_allocated_path(paths[0]);\n> +\t\tfree_allocated_path(paths[1]);\n>  \t\treturn 1;\n> +\t}\n>  \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n>  \tdiffcore_std(&revs->diffopt);\n>  \tdiff_flush(&revs->diffopt);\n>\n> -\tstrbuf_release(&replacement);\n> +\t/*\n> +\t * do not strbuf_release(&replacement), as it is in paths[]\n> +\t * when replacement was actually used.\n> +\t */\n> +\tfree_allocated_path(paths[0]);\n> +\tfree_allocated_path(paths[1]);\n>\n>  \t/*\n>  \t * The return code for --no-index imitates diff(1):\n\nPerhaps avoid the need for that comment by moving that strbuf to where\nit's used and have it spend its full lifecycle there?  Something like:\n\n---\n diff-no-index.c | 50 ++++++++++++++++++++++---------------------------\n 1 file changed, 22 insertions(+), 28 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 77a126469b..9f8b78f173 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -196,18 +196,6 @@ static int queue_diff(struct diff_options *o,\n \t}\n }\n\n-/* append basename of F to D */\n-static void append_basename(struct strbuf *path, const char *dir, const char *file)\n-{\n-\tconst char *tail = strrchr(file, '/');\n-\n-\tstrbuf_addstr(path, dir);\n-\twhile (path->len && path->buf[path->len - 1] == '/')\n-\t\tpath->len--;\n-\tstrbuf_addch(path, '/');\n-\tstrbuf_addstr(path, tail ? tail + 1 : file);\n-}\n-\n static void free_allocated_path(const char *path)\n {\n \tif (!path ||\n@@ -216,12 +204,28 @@ static void free_allocated_path(const char *path)\n \tfree((char *)path);\n }\n\n+/* append basename of F to D */\n+static void append_basename(const char **dir, const char *file)\n+{\n+\tconst char *tail = strrchr(file, '/');\n+\tstruct strbuf path = STRBUF_INIT;\n+\n+\tstrbuf_addstr(&path, *dir);\n+\twhile (path.len && path.buf[path.len - 1] == '/')\n+\t\tpath.len--;\n+\tstrbuf_addch(&path, '/');\n+\tstrbuf_addstr(&path, tail ? tail + 1 : file);\n+\n+\tfree_allocated_path(*dir);\n+\t*dir = strbuf_detach(&path, NULL);\n+}\n+\n /*\n  * DWIM \"diff D F\" into \"diff D/F F\" and \"diff F D\" into \"diff F D/F\"\n  * Note that we append the basename of F to D/, so \"diff a/b/file D\"\n  * becomes \"diff a/b/file D/file\", not \"diff a/b/file D/a/b/file\".\n  */\n-static void fixup_paths(const char **path, struct strbuf *replacement)\n+static void fixup_paths(const char **path)\n {\n \tunsigned int isdir0, isdir1;\n\n@@ -232,15 +236,10 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n \tisdir1 = is_directory(path[1]);\n \tif (isdir0 == isdir1)\n \t\treturn;\n-\tif (isdir0) {\n-\t\tappend_basename(replacement, path[0], path[1]);\n-\t\tfree_allocated_path(path[0]);\n-\t\tpath[0] = replacement->buf;\n-\t} else {\n-\t\tappend_basename(replacement, path[1], path[0]);\n-\t\tfree_allocated_path(path[1]);\n-\t\tpath[1] = replacement->buf;\n-\t}\n+\tif (isdir0)\n+\t\tappend_basename(&path[0], path[1]);\n+\telse\n+\t\tappend_basename(&path[1], path[0]);\n }\n\n static const char * const diff_no_index_usage[] = {\n@@ -254,7 +253,6 @@ int diff_no_index(struct rev_info *revs,\n {\n \tint i, no_index;\n \tconst char *paths[2];\n-\tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n \tstruct option no_index_options[] = {\n \t\tOPT_BOOL_F(0, \"no-index\", &no_index, \"\",\n@@ -289,7 +287,7 @@ int diff_no_index(struct rev_info *revs,\n \t\tpaths[i] = p;\n \t}\n\n-\tfixup_paths(paths, &replacement);\n+\tfixup_paths(paths);\n\n \trevs->diffopt.skip_stat_unmatch = 1;\n \tif (!revs->diffopt.output_format)\n@@ -315,10 +313,6 @@ int diff_no_index(struct rev_info *revs,\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n\n-\t/*\n-\t * do not strbuf_release(&replacement), as it is in paths[]\n-\t * when replacement was actually used.\n-\t */\n \tfree_allocated_path(paths[0]);\n \tfree_allocated_path(paths[1]);\n\n--\n2.37.2\n\n"},{"id":"462621","messageId":"01pn98p2-qppn-260p-8o80-n5483p41859o@tzk.qr","threadId":"58393","inReplyTo":"xmqqilm51gn6.fsf@gitster.g","subject":"Re: [PATCH v2] diff --no-index: unleak paths[] elements","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-05T10:03:42Z","receivedAt":"2022-09-05T10:03:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 2 Sep 2022, Junio C Hamano wrote:\n\n> \"git diff --no-index\" codepath starts with the two elements in\n> argv[] and munges them into two paths to be compared, stored in a\n> separate path[] arrays.  The munging is implemented in a rather\n> haphazard way, sometimes overwriting old version with a new copy,\n> and sometimes a constant string assigned to path[], making it\n> impossible to release the resources properly:\n>\n>  * A single dash \"-\" from the command line is a special signal that\n>    the standard input is used for the side to be compared, and is\n>    internally replaced with a copy of string \"-\" at a known address.\n>\n>  * When run in a subdirectory, full paths to the two paths are\n>    allocated and placed in path[].\n>\n>  * After the above happens, when comparing a file with a directory,\n>    the directory side is replaced with the path to a file in the\n>    directory with the same name as the file.\n>\n> This was perfectly fine for just two strings that are pathnames used\n> during the lifetime of the program and cleaned up upon program exit,\n> but it gets in the way when leak sanitizer is in effect.  The third\n> step can be losing the full path that was allocated in the second\n> step, but it is not easy to tell if its input is an allocated piece\n> of memory to begin with.\n>\n> Loosen the earlier two steps a bit so that elements of the path[]\n> array that come to the directory/file comparison code are either the\n> singleton \"-\" or an allocated piece of memory.  Use that knowledge\n> in the third step to release an allocated piece of memory when it\n> replaces the path to a directory with the path to a file in that\n> directory, and also at the end to release the two elements of the\n> path[] array as needed.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  * The previous one allowed strbuf_release() to free replacement.buf\n>    which may be used in path[0] or path[1] potentially leading to\n>    double freeing.  The kosher way may be to use strbuf_detach() in\n>    fixup_paths(), but this is a simpler fix, it is getting late in\n>    the day, and I am getting sick of fighting the leak-checker, so...\n\nI wonder whether a much better way would be to first fix the code to\nalways release `replacement`, like so:\n\n-- snip --\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 9a8b09346bd..87047605385 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -242,7 +242,7 @@ int diff_no_index(struct rev_info *revs,\n \t\t  int implicit_no_index,\n \t\t  int argc, const char **argv)\n {\n-\tint i, no_index;\n+\tint i, no_index, ret;\n \tconst char *paths[2];\n \tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n@@ -294,17 +294,23 @@ int diff_no_index(struct rev_info *revs,\n \tsetup_diff_pager(&revs->diffopt);\n \trevs->diffopt.flags.exit_with_status = 1;\n\n-\tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n-\t\treturn 1;\n+\tif (queue_diff(&revs->diffopt, paths[0], paths[1])) {\n+\t\tret = 1;\n+\t\tgoto out;\n+\t}\n+\n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n\n-\tstrbuf_release(&replacement);\n-\n \t/*\n \t * The return code for --no-index imitates diff(1):\n \t * 0 = no changes, 1 = changes, else error\n \t */\n-\treturn diff_result_code(&revs->diffopt, 0);\n+\tret = diff_result_code(&revs->diffopt, 0);\n+\n+out:\n+\tstrbuf_release(&replacement);\n+\n+\treturn ret;\n }\n-- snap --\n\nAfter that, the proposed diff could be replaced by this diff:\n\n-- snip --\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 87047605385..d350e4381bc 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -244,6 +244,7 @@ int diff_no_index(struct rev_info *revs,\n {\n \tint i, no_index, ret;\n \tconst char *paths[2];\n+\tstruct string_list to_release = STRING_LIST_INIT_DUP;\n \tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n \tstruct option no_index_options[] = {\n@@ -274,6 +275,12 @@ int diff_no_index(struct rev_info *revs,\n \t\t\tp = file_from_standard_input;\n \t\telse if (prefix)\n \t\t\tp = prefix_filename(prefix, p);\n+\t\telse {\n+\t\t\tchar *dup = xstrdup(p);\n+\n+\t\t\tp = dup;\n+\t\t\tstring_list_append_nodup(&to_release, dup);\n+\t\t}\n \t\tpaths[i] = p;\n \t}\n\n@@ -310,6 +317,7 @@ int diff_no_index(struct rev_info *revs,\n \tret = diff_result_code(&revs->diffopt, 0);\n\n out:\n+\tstring_list_clear(&to_release, 1);\n \tstrbuf_release(&replacement);\n\n \treturn ret;\n\n-- snap --\n\nThat approach has the distinct advantage of making it very easy to reason\nabout the code.\n\nWhat do you think?\n\nCiao,\nDscho\n\n>\n>  diff-no-index.c | 24 ++++++++++++++++++++++--\n>  1 file changed, 22 insertions(+), 2 deletions(-)\n>\n>\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 9a8b09346b..77a126469b 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -208,6 +208,14 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi\n>  \tstrbuf_addstr(path, tail ? tail + 1 : file);\n>  }\n>\n> +static void free_allocated_path(const char *path)\n> +{\n> +\tif (!path ||\n> +\t    (path == file_from_standard_input))\n> +\t\treturn;\n> +\tfree((char *)path);\n> +}\n> +\n>  /*\n>   * DWIM \"diff D F\" into \"diff D/F F\" and \"diff F D\" into \"diff F D/F\"\n>   * Note that we append the basename of F to D/, so \"diff a/b/file D\"\n> @@ -226,9 +234,11 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n>  \t\treturn;\n>  \tif (isdir0) {\n>  \t\tappend_basename(replacement, path[0], path[1]);\n> +\t\tfree_allocated_path(path[0]);\n>  \t\tpath[0] = replacement->buf;\n>  \t} else {\n>  \t\tappend_basename(replacement, path[1], path[0]);\n> +\t\tfree_allocated_path(path[1]);\n>  \t\tpath[1] = replacement->buf;\n>  \t}\n>  }\n> @@ -274,6 +284,8 @@ int diff_no_index(struct rev_info *revs,\n>  \t\t\tp = file_from_standard_input;\n>  \t\telse if (prefix)\n>  \t\t\tp = prefix_filename(prefix, p);\n> +\t\telse\n> +\t\t\tp = xstrdup(p);\n>  \t\tpaths[i] = p;\n>  \t}\n>\n> @@ -294,13 +306,21 @@ int diff_no_index(struct rev_info *revs,\n>  \tsetup_diff_pager(&revs->diffopt);\n>  \trevs->diffopt.flags.exit_with_status = 1;\n>\n> -\tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n> +\tif (queue_diff(&revs->diffopt, paths[0], paths[1])) {\n> +\t\tfree_allocated_path(paths[0]);\n> +\t\tfree_allocated_path(paths[1]);\n>  \t\treturn 1;\n> +\t}\n>  \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n>  \tdiffcore_std(&revs->diffopt);\n>  \tdiff_flush(&revs->diffopt);\n>\n> -\tstrbuf_release(&replacement);\n> +\t/*\n> +\t * do not strbuf_release(&replacement), as it is in paths[]\n> +\t * when replacement was actually used.\n> +\t */\n> +\tfree_allocated_path(paths[0]);\n> +\tfree_allocated_path(paths[1]);\n>\n>  \t/*\n>  \t * The return code for --no-index imitates diff(1):\n> --\n> 2.37.3-661-g73a641a77a\n>\n>\n"},{"id":"462627","messageId":"220905.86zgfekrqq.gmgdl@evledraar.gmail.com","threadId":"58393","inReplyTo":"01pn98p2-qppn-260p-8o80-n5483p41859o@tzk.qr","subject":"Re: [PATCH v2] diff --no-index: unleak paths[] elements","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-05T11:01:51Z","receivedAt":"2022-09-05T11:04:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 05 2022, Johannes Schindelin wrote:\n\n> Hi Junio,\n>\n> On Fri, 2 Sep 2022, Junio C Hamano wrote:\n>\n>> \"git diff --no-index\" codepath starts with the two elements in\n>> argv[] and munges them into two paths to be compared, stored in a\n>> separate path[] arrays.  The munging is implemented in a rather\n>> haphazard way, sometimes overwriting old version with a new copy,\n>> and sometimes a constant string assigned to path[], making it\n>> impossible to release the resources properly:\n>>\n>>  * A single dash \"-\" from the command line is a special signal that\n>>    the standard input is used for the side to be compared, and is\n>>    internally replaced with a copy of string \"-\" at a known address.\n>>\n>>  * When run in a subdirectory, full paths to the two paths are\n>>    allocated and placed in path[].\n>>\n>>  * After the above happens, when comparing a file with a directory,\n>>    the directory side is replaced with the path to a file in the\n>>    directory with the same name as the file.\n>>\n>> This was perfectly fine for just two strings that are pathnames used\n>> during the lifetime of the program and cleaned up upon program exit,\n>> but it gets in the way when leak sanitizer is in effect.  The third\n>> step can be losing the full path that was allocated in the second\n>> step, but it is not easy to tell if its input is an allocated piece\n>> of memory to begin with.\n>>\n>> Loosen the earlier two steps a bit so that elements of the path[]\n>> array that come to the directory/file comparison code are either the\n>> singleton \"-\" or an allocated piece of memory.  Use that knowledge\n>> in the third step to release an allocated piece of memory when it\n>> replaces the path to a directory with the path to a file in that\n>> directory, and also at the end to release the two elements of the\n>> path[] array as needed.\n>>\n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>> ---\n>>\n>>  * The previous one allowed strbuf_release() to free replacement.buf\n>>    which may be used in path[0] or path[1] potentially leading to\n>>    double freeing.  The kosher way may be to use strbuf_detach() in\n>>    fixup_paths(), but this is a simpler fix, it is getting late in\n>>    the day, and I am getting sick of fighting the leak-checker, so...\n>\n> I wonder whether a much better way would be to first fix the code to\n> always release `replacement`, like so:\n>\n> -- snip --\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 9a8b09346bd..87047605385 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -242,7 +242,7 @@ int diff_no_index(struct rev_info *revs,\n>  \t\t  int implicit_no_index,\n>  \t\t  int argc, const char **argv)\n>  {\n> -\tint i, no_index;\n> +\tint i, no_index, ret;\n>  \tconst char *paths[2];\n>  \tstruct strbuf replacement = STRBUF_INIT;\n>  \tconst char *prefix = revs->prefix;\n> @@ -294,17 +294,23 @@ int diff_no_index(struct rev_info *revs,\n>  \tsetup_diff_pager(&revs->diffopt);\n>  \trevs->diffopt.flags.exit_with_status = 1;\n>\n> -\tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n> -\t\treturn 1;\n> +\tif (queue_diff(&revs->diffopt, paths[0], paths[1])) {\n> +\t\tret = 1;\n> +\t\tgoto out;\n> +\t}\n> +\n>  \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n>  \tdiffcore_std(&revs->diffopt);\n>  \tdiff_flush(&revs->diffopt);\n>\n> -\tstrbuf_release(&replacement);\n> -\n>  \t/*\n>  \t * The return code for --no-index imitates diff(1):\n>  \t * 0 = no changes, 1 = changes, else error\n>  \t */\n> -\treturn diff_result_code(&revs->diffopt, 0);\n> +\tret = diff_result_code(&revs->diffopt, 0);\n> +\n> +out:\n> +\tstrbuf_release(&replacement);\n> +\n> +\treturn ret;\n>  }\n> -- snap --\n>\n> After that, the proposed diff could be replaced by this diff:\n>\n> -- snip --\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 87047605385..d350e4381bc 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -244,6 +244,7 @@ int diff_no_index(struct rev_info *revs,\n>  {\n>  \tint i, no_index, ret;\n>  \tconst char *paths[2];\n> +\tstruct string_list to_release = STRING_LIST_INIT_DUP;\n>  \tstruct strbuf replacement = STRBUF_INIT;\n>  \tconst char *prefix = revs->prefix;\n>  \tstruct option no_index_options[] = {\n> @@ -274,6 +275,12 @@ int diff_no_index(struct rev_info *revs,\n>  \t\t\tp = file_from_standard_input;\n>  \t\telse if (prefix)\n>  \t\t\tp = prefix_filename(prefix, p);\n> +\t\telse {\n> +\t\t\tchar *dup = xstrdup(p);\n> +\n> +\t\t\tp = dup;\n> +\t\t\tstring_list_append_nodup(&to_release, dup);\n> +\t\t}\n>  \t\tpaths[i] = p;\n>  \t}\n>\n> @@ -310,6 +317,7 @@ int diff_no_index(struct rev_info *revs,\n>  \tret = diff_result_code(&revs->diffopt, 0);\n>\n>  out:\n> +\tstring_list_clear(&to_release, 1);\n>  \tstrbuf_release(&replacement);\n>\n>  \treturn ret;\n>\n> -- snap --\n>\n> That approach has the distinct advantage of making it very easy to reason\n> about the code.\n>\n> What do you think?\n>\n> Ciao,\n> Dscho\n>\n>>\n>>  diff-no-index.c | 24 ++++++++++++++++++++++--\n>>  1 file changed, 22 insertions(+), 2 deletions(-)\n>>\n>>\n>> diff --git a/diff-no-index.c b/diff-no-index.c\n>> index 9a8b09346b..77a126469b 100644\n>> --- a/diff-no-index.c\n>> +++ b/diff-no-index.c\n>> @@ -208,6 +208,14 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi\n>>  \tstrbuf_addstr(path, tail ? tail + 1 : file);\n>>  }\n>>\n>> +static void free_allocated_path(const char *path)\n>> +{\n>> +\tif (!path ||\n>> +\t    (path == file_from_standard_input))\n>> +\t\treturn;\n>> +\tfree((char *)path);\n>> +}\n>> +\n>>  /*\n>>   * DWIM \"diff D F\" into \"diff D/F F\" and \"diff F D\" into \"diff F D/F\"\n>>   * Note that we append the basename of F to D/, so \"diff a/b/file D\"\n>> @@ -226,9 +234,11 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n>>  \t\treturn;\n>>  \tif (isdir0) {\n>>  \t\tappend_basename(replacement, path[0], path[1]);\n>> +\t\tfree_allocated_path(path[0]);\n>>  \t\tpath[0] = replacement->buf;\n>>  \t} else {\n>>  \t\tappend_basename(replacement, path[1], path[0]);\n>> +\t\tfree_allocated_path(path[1]);\n>>  \t\tpath[1] = replacement->buf;\n>>  \t}\n>>  }\n>> @@ -274,6 +284,8 @@ int diff_no_index(struct rev_info *revs,\n>>  \t\t\tp = file_from_standard_input;\n>>  \t\telse if (prefix)\n>>  \t\t\tp = prefix_filename(prefix, p);\n>> +\t\telse\n>> +\t\t\tp = xstrdup(p);\n>>  \t\tpaths[i] = p;\n>>  \t}\n>>\n>> @@ -294,13 +306,21 @@ int diff_no_index(struct rev_info *revs,\n>>  \tsetup_diff_pager(&revs->diffopt);\n>>  \trevs->diffopt.flags.exit_with_status = 1;\n>>\n>> -\tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n>> +\tif (queue_diff(&revs->diffopt, paths[0], paths[1])) {\n>> +\t\tfree_allocated_path(paths[0]);\n>> +\t\tfree_allocated_path(paths[1]);\n>>  \t\treturn 1;\n>> +\t}\n>>  \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n>>  \tdiffcore_std(&revs->diffopt);\n>>  \tdiff_flush(&revs->diffopt);\n>>\n>> -\tstrbuf_release(&replacement);\n>> +\t/*\n>> +\t * do not strbuf_release(&replacement), as it is in paths[]\n>> +\t * when replacement was actually used.\n>> +\t */\n>> +\tfree_allocated_path(paths[0]);\n>> +\tfree_allocated_path(paths[1]);\n>>\n>>  \t/*\n>>  \t * The return code for --no-index imitates diff(1):\n>> --\n>> 2.37.3-661-g73a641a77a\n>>\n>>\n\nThat looks like a much better approach, I'd think you'd want this on\ntop, because:\n\n * We entirely avoid playing cames with the string_list \"dup\" and then\n   \"nodup\" append. In some cases we have to, but in this case we can\n   just get the pointer to the member we just created, and avoid the\n   explicit xstrdup() in the caller.\n * The free_util=1 in your code isn't needed/is a bug, we make no use of\n   \"util\" here, so it should be free_util=0\n * It avoids the \"add braces to all if/else arms\" part of\n   CodingGuidelines.\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex d350e4381bc..2861319c0e5 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -275,12 +275,8 @@ int diff_no_index(struct rev_info *revs,\n \t\t\tp = file_from_standard_input;\n \t\telse if (prefix)\n \t\t\tp = prefix_filename(prefix, p);\n-\t\telse {\n-\t\t\tchar *dup = xstrdup(p);\n-\n-\t\t\tp = dup;\n-\t\t\tstring_list_append_nodup(&to_release, dup);\n-\t\t}\n+\t\telse\n+\t\t\tp = string_list_append(&to_release, p)->string;\n \t\tpaths[i] = p;\n \t}\n \n@@ -317,7 +313,7 @@ int diff_no_index(struct rev_info *revs,\n \tret = diff_result_code(&revs->diffopt, 0);\n \n out:\n-\tstring_list_clear(&to_release, 1);\n+\tstring_list_clear(&to_release, 0);\n \tstrbuf_release(&replacement);\n \n \treturn ret;\n"},{"id":"462643","messageId":"xmqq8rmx1saz.fsf@gitster.g","threadId":"58393","inReplyTo":"181c029b-8b36-4b04-30f9-97a3f252bfbc@web.de","subject":"Re: [PATCH v2] diff --no-index: unleak paths[] elements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-05T20:26:28Z","receivedAt":"2022-09-05T20:26:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Am 03.09.22 um 01:49 schrieb Junio C Hamano:\n>> \"git diff --no-index\" codepath starts with the two elements in\n>> argv[] and munges them into two paths to be compared, stored in a\n>> separate path[] arrays.  The munging is implemented in a rather\n>> haphazard way, sometimes overwriting old version with a new copy,\n>> and sometimes a constant string assigned to path[], making it\n>> impossible to release the resources properly:\n>>\n>>  * A single dash \"-\" from the command line is a special signal that\n>>    the standard input is used for the side to be compared, and is\n>>    internally replaced with a copy of string \"-\" at a known address.\n>>\n>>  * When run in a subdirectory, full paths to the two paths are\n>>    allocated and placed in path[].\n>>\n>>  * After the above happens, when comparing a file with a directory,\n>>    the directory side is replaced with the path to a file in the\n>>    directory with the same name as the file.\n>>\n>> This was perfectly fine for just two strings that are pathnames used\n>> during the lifetime of the program and cleaned up upon program exit,\n>> but it gets in the way when leak sanitizer is in effect.  The third\n>> step can be losing the full path that was allocated in the second\n>> step, but it is not easy to tell if its input is an allocated piece\n>> of memory to begin with.\n>>\n>> Loosen the earlier two steps a bit so that elements of the path[]\n>> array that come to the directory/file comparison code are either the\n>> singleton \"-\" or an allocated piece of memory.  Use that knowledge\n>> in the third step to release an allocated piece of memory when it\n>> replaces the path to a directory with the path to a file in that\n>> directory, and also at the end to release the two elements of the\n>> path[] array as needed.\n>>\n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>> ---\n>>\n>>  * The previous one allowed strbuf_release() to free replacement.buf\n>>    which may be used in path[0] or path[1] potentially leading to\n>>    double freeing.  The kosher way may be to use strbuf_detach() in\n>>    fixup_paths(), but this is a simpler fix, it is getting late in\n>>    the day, and I am getting sick of fighting the leak-checker, so...\n>>\n>>  diff-no-index.c | 24 ++++++++++++++++++++++--\n>>  1 file changed, 22 insertions(+), 2 deletions(-)\n>>\n>>\n>> diff --git a/diff-no-index.c b/diff-no-index.c\n>> index 9a8b09346b..77a126469b 100644\n>> --- a/diff-no-index.c\n>> +++ b/diff-no-index.c\n>> @@ -208,6 +208,14 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi\n>>  \tstrbuf_addstr(path, tail ? tail + 1 : file);\n>>  }\n>>\n>> +static void free_allocated_path(const char *path)\n>> +{\n>> +\tif (!path ||\n>\n> How can path be NULL?  And if it was, why shield free(3) from it?\n\nSee the comment under three-dashes of the first iteration.\n\n>> +\t/*\n>> +\t * do not strbuf_release(&replacement), as it is in paths[]\n>> +\t * when replacement was actually used.\n>> +\t */\n>> +\tfree_allocated_path(paths[0]);\n>> +\tfree_allocated_path(paths[1]);\n>>\n>>  \t/*\n>>  \t * The return code for --no-index imitates diff(1):\n>\n> Perhaps avoid the need for that comment by moving that strbuf to where\n> it's used and have it spend its full lifecycle there?  Something like:\n\nYup, that is what I said in the comment under three-dashes (with the\nreason why I didn't bother).\n\nQuite honestly I am sick of fighting the overzealous leak-checker so\nI'd very much appreciate if somebody else pick this up and run with\nit.\n\nThanks.\n\n"},{"id":"462647","messageId":"a3e73848-4262-1875-5ba1-085d31297933@web.de","threadId":"58393","inReplyTo":"xmqq8rmx1saz.fsf@gitster.g","subject":"[PATCH 1/2] diff-no-index: release strbuf on queue error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-06T12:31:28Z","receivedAt":"2022-09-06T12:32:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The strbuf is small and we are about to exit, so we could leave its\ncleanup to the OS.  If we release it explicitly at all, however, then we\nshould do it on early exit as well.  Move it to a new cleanup section at\nthe end and make sure all execution paths go through it.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n diff-no-index.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 9a8b09346b..a3683d8a04 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -243,6 +243,7 @@ int diff_no_index(struct rev_info *revs,\n \t\t  int argc, const char **argv)\n {\n \tint i, no_index;\n+\tint ret = 1;\n \tconst char *paths[2];\n \tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n@@ -295,16 +296,18 @@ int diff_no_index(struct rev_info *revs,\n \trevs->diffopt.flags.exit_with_status = 1;\n\n \tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n-\t\treturn 1;\n+\t\tgoto out;\n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n\n-\tstrbuf_release(&replacement);\n-\n \t/*\n \t * The return code for --no-index imitates diff(1):\n \t * 0 = no changes, 1 = changes, else error\n \t */\n-\treturn diff_result_code(&revs->diffopt, 0);\n+\tret = diff_result_code(&revs->diffopt, 0);\n+\n+out:\n+\tstrbuf_release(&replacement);\n+\treturn ret;\n }\n--\n2.37.2\n"},{"id":"462648","messageId":"df5f8305-79d5-2c12-bdf0-961428c0bdd1@web.de","threadId":"58393","inReplyTo":"xmqq8rmx1saz.fsf@gitster.g","subject":"[PATCH 2/2] diff-no-index: release prefixed filenames","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-06T12:31:32Z","receivedAt":"2022-09-06T12:32:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Callers of prefix_filename() are responsible for freeing its result.\nRemember them and release them to appease leak checkers.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n diff-no-index.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex a3683d8a04..35809f26d7 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -245,6 +245,7 @@ int diff_no_index(struct rev_info *revs,\n \tint i, no_index;\n \tint ret = 1;\n \tconst char *paths[2];\n+\tchar *to_free[2] = { 0 };\n \tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n \tstruct option no_index_options[] = {\n@@ -274,7 +275,7 @@ int diff_no_index(struct rev_info *revs,\n \t\t\t */\n \t\t\tp = file_from_standard_input;\n \t\telse if (prefix)\n-\t\t\tp = prefix_filename(prefix, p);\n+\t\t\tp = to_free[i] = prefix_filename(prefix, p);\n \t\tpaths[i] = p;\n \t}\n\n@@ -308,6 +309,8 @@ int diff_no_index(struct rev_info *revs,\n \tret = diff_result_code(&revs->diffopt, 0);\n\n out:\n+\tfor (i = 0; i < 2; i++)\n+\t\tfree(to_free[i]);\n \tstrbuf_release(&replacement);\n \treturn ret;\n }\n--\n2.37.2\n"},{"id":"462683","messageId":"3a2609e3-453a-441b-346d-dcc2877d159f@web.de","threadId":"58393","inReplyTo":"a3e73848-4262-1875-5ba1-085d31297933@web.de","subject":"Re: [PATCH 1/2] diff-no-index: release strbuf on queue error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-07T06:01:27Z","receivedAt":"2022-09-07T06:01:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.09.22 um 14:31 schrieb René Scharfe:\n> The strbuf is small and we are about to exit, so we could leave its\n> cleanup to the OS.  If we release it explicitly at all, however, then we\n> should do it on early exit as well.  Move it to a new cleanup section at\n> the end and make sure all execution paths go through it.\n>\n\nOh, forgot:\n\nSuggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  diff-no-index.c | 11 +++++++----\n>  1 file changed, 7 insertions(+), 4 deletions(-)\n>\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 9a8b09346b..a3683d8a04 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -243,6 +243,7 @@ int diff_no_index(struct rev_info *revs,\n>  \t\t  int argc, const char **argv)\n>  {\n>  \tint i, no_index;\n> +\tint ret = 1;\n>  \tconst char *paths[2];\n>  \tstruct strbuf replacement = STRBUF_INIT;\n>  \tconst char *prefix = revs->prefix;\n> @@ -295,16 +296,18 @@ int diff_no_index(struct rev_info *revs,\n>  \trevs->diffopt.flags.exit_with_status = 1;\n>\n>  \tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n> -\t\treturn 1;\n> +\t\tgoto out;\n>  \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n>  \tdiffcore_std(&revs->diffopt);\n>  \tdiff_flush(&revs->diffopt);\n>\n> -\tstrbuf_release(&replacement);\n> -\n>  \t/*\n>  \t * The return code for --no-index imitates diff(1):\n>  \t * 0 = no changes, 1 = changes, else error\n>  \t */\n> -\treturn diff_result_code(&revs->diffopt, 0);\n> +\tret = diff_result_code(&revs->diffopt, 0);\n> +\n> +out:\n> +\tstrbuf_release(&replacement);\n> +\treturn ret;\n>  }\n> --\n> 2.37.2\n"},{"id":"462698","messageId":"n575rrn9-5p4o-r40o-snsq-197p242q10p5@tzk.qr","threadId":"58393","inReplyTo":"df5f8305-79d5-2c12-bdf0-961428c0bdd1@web.de","subject":"Re: [PATCH 2/2] diff-no-index: release prefixed filenames","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-07T10:03:51Z","receivedAt":"2022-09-07T10:04:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Tue, 6 Sep 2022, René Scharfe wrote:\n\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index a3683d8a04..35809f26d7 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -245,6 +245,7 @@ int diff_no_index(struct rev_info *revs,\n>  \tint i, no_index;\n>  \tint ret = 1;\n>  \tconst char *paths[2];\n> +\tchar *to_free[2] = { 0 };\n>  \tstruct strbuf replacement = STRBUF_INIT;\n>  \tconst char *prefix = revs->prefix;\n>  \tstruct option no_index_options[] = {\n> @@ -274,7 +275,7 @@ int diff_no_index(struct rev_info *revs,\n>  \t\t\t */\n>  \t\t\tp = file_from_standard_input;\n>  \t\telse if (prefix)\n> -\t\t\tp = prefix_filename(prefix, p);\n> +\t\t\tp = to_free[i] = prefix_filename(prefix, p);\n>  \t\tpaths[i] = p;\n>  \t}\n>\n> @@ -308,6 +309,8 @@ int diff_no_index(struct rev_info *revs,\n>  \tret = diff_result_code(&revs->diffopt, 0);\n>\n>  out:\n> +\tfor (i = 0; i < 2; i++)\n> +\t\tfree(to_free[i]);\n\nHeh. That's long-hand for\n\n\tfree(to_free[0]);\n\tfree(to_free[1]);\n\nIf you do want to have that loop, please replace the hard-coded 2 by\n`ARRAY_SIZE(to_free)`.\n\nOtherwise, both patches look fine to me.\n\nThanks!\nDscho\n\n>  \tstrbuf_release(&replacement);\n>  \treturn ret;\n>  }\n> --\n> 2.37.2\n>\n"},{"id":"462699","messageId":"36q9s463-rno9-9rs1-7qsq-5oqq23o30o0r@tzk.qr","threadId":"58393","inReplyTo":"220905.86zgfekrqq.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] diff --no-index: unleak paths[] elements","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-07T10:06:33Z","receivedAt":"2022-09-07T10:06:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Mon, 5 Sep 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> On Mon, Sep 05 2022, Johannes Schindelin wrote:\n>\n> [...]\n> > +\tstring_list_clear(&to_release, 1);\n> [...]\n>\n>  * The free_util=1 in your code isn't needed/is a bug, we make no use of\n>    \"util\" here, so it should be free_util=0\n\nCalling it a bug is a bit strong, and misses the reason why I did it:\nfuture-proofing.\n\nIn any case, René took up Junio's ask and provided a new iteration that\nside-steps this concern altogether, therefore the point is now moot.\n\nCiao,\nJohannes\n"},{"id":"462702","messageId":"90d7af20-0563-3e09-f7dc-f69f8d473928@web.de","threadId":"58393","inReplyTo":"n575rrn9-5p4o-r40o-snsq-197p242q10p5@tzk.qr","subject":"Re: [PATCH 2/2] diff-no-index: release prefixed filenames","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-07T11:19:54Z","receivedAt":"2022-09-07T11:20:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.09.22 um 12:03 schrieb Johannes Schindelin:\n> Hi René,\n>\n> On Tue, 6 Sep 2022, René Scharfe wrote:\n>\n>> diff --git a/diff-no-index.c b/diff-no-index.c\n>> index a3683d8a04..35809f26d7 100644\n>> --- a/diff-no-index.c\n>> +++ b/diff-no-index.c\n>> @@ -245,6 +245,7 @@ int diff_no_index(struct rev_info *revs,\n>>  \tint i, no_index;\n>>  \tint ret = 1;\n>>  \tconst char *paths[2];\n>> +\tchar *to_free[2] = { 0 };\n>>  \tstruct strbuf replacement = STRBUF_INIT;\n>>  \tconst char *prefix = revs->prefix;\n>>  \tstruct option no_index_options[] = {\n>> @@ -274,7 +275,7 @@ int diff_no_index(struct rev_info *revs,\n>>  \t\t\t */\n>>  \t\t\tp = file_from_standard_input;\n>>  \t\telse if (prefix)\n>> -\t\t\tp = prefix_filename(prefix, p);\n>> +\t\t\tp = to_free[i] = prefix_filename(prefix, p);\n>>  \t\tpaths[i] = p;\n>>  \t}\n>>\n>> @@ -308,6 +309,8 @@ int diff_no_index(struct rev_info *revs,\n>>  \tret = diff_result_code(&revs->diffopt, 0);\n>>\n>>  out:\n>> +\tfor (i = 0; i < 2; i++)\n>> +\t\tfree(to_free[i]);\n>\n> Heh. That's long-hand for\n>\n> \tfree(to_free[0]);\n> \tfree(to_free[1]);\n\nHad that before, but it's repetitive and more importantly this loop\nmatches the first one.\n\n> If you do want to have that loop, please replace the hard-coded 2 by\n> `ARRAY_SIZE(to_free)`.\n\nThe two is hard-coded in other places explicitly as well and implied in\nfixup_paths().  The root cause is not any array size but the design\ndecision to require exactly two things to compare.  A reader would need\nto know that.  We could sure use ARRAY_SIZE(paths) in the declaration\nof to_free and ARRAY_SIZE(to_free) in the loop to at least not add more\ninstances of that magic number and make the code understandable without\nseeing the bigger picture.\n\n> Otherwise, both patches look fine to me.\n>\n> Thanks!\n> Dscho\n>\n>>  \tstrbuf_release(&replacement);\n>>  \treturn ret;\n>>  }\n>> --\n>> 2.37.2\n>>\n"},{"id":"462703","messageId":"1aee1020-2b8a-906b-8e25-051dd6b48fb9@web.de","threadId":"58393","inReplyTo":"xmqq8rmx1saz.fsf@gitster.g","subject":"[PATCH v2 1/2] diff-no-index: release strbuf on queue error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-07T11:36:53Z","receivedAt":"2022-09-07T11:37:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The strbuf is small and we are about to exit, so we could leave its\ncleanup to the OS.  If we release it explicitly at all, however, then we\nshould do it on early exit as well.  Move the strbuf_release call to a\nnew cleanup section at the end and make sure all execution paths go\nthrough it.\n\nSuggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges:\n* Add Suggested-by.\n* Clarify message (s/Move it/Move the strbuf_release call/).\n\n diff-no-index.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 9a8b09346b..a3683d8a04 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -243,6 +243,7 @@ int diff_no_index(struct rev_info *revs,\n \t\t  int argc, const char **argv)\n {\n \tint i, no_index;\n+\tint ret = 1;\n \tconst char *paths[2];\n \tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n@@ -295,16 +296,18 @@ int diff_no_index(struct rev_info *revs,\n \trevs->diffopt.flags.exit_with_status = 1;\n\n \tif (queue_diff(&revs->diffopt, paths[0], paths[1]))\n-\t\treturn 1;\n+\t\tgoto out;\n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n\n-\tstrbuf_release(&replacement);\n-\n \t/*\n \t * The return code for --no-index imitates diff(1):\n \t * 0 = no changes, 1 = changes, else error\n \t */\n-\treturn diff_result_code(&revs->diffopt, 0);\n+\tret = diff_result_code(&revs->diffopt, 0);\n+\n+out:\n+\tstrbuf_release(&replacement);\n+\treturn ret;\n }\n--\n2.37.2\n"},{"id":"462704","messageId":"49a40ff2-757a-5d7b-4146-7e8b35884ae8@web.de","threadId":"58393","inReplyTo":"xmqq8rmx1saz.fsf@gitster.g","subject":"[PATCH v2 2/2] diff-no-index: release prefixed filenames","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-07T11:37:02Z","receivedAt":"2022-09-07T11:37:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Callers of prefix_filename() are responsible for freeing its result.\nRemember the returned strings and release them to appease leak checkers.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges:\n* Use ARRAY_SIZE twice instead of hard-code 2.\n* Clarify message (s/Remember them/Remember the returned strings/).\n\n diff-no-index.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex a3683d8a04..a18f6c3c63 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -245,6 +245,7 @@ int diff_no_index(struct rev_info *revs,\n \tint i, no_index;\n \tint ret = 1;\n \tconst char *paths[2];\n+\tchar *to_free[ARRAY_SIZE(paths)] = { 0 };\n \tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n \tstruct option no_index_options[] = {\n@@ -274,7 +275,7 @@ int diff_no_index(struct rev_info *revs,\n \t\t\t */\n \t\t\tp = file_from_standard_input;\n \t\telse if (prefix)\n-\t\t\tp = prefix_filename(prefix, p);\n+\t\t\tp = to_free[i] = prefix_filename(prefix, p);\n \t\tpaths[i] = p;\n \t}\n\n@@ -308,6 +309,8 @@ int diff_no_index(struct rev_info *revs,\n \tret = diff_result_code(&revs->diffopt, 0);\n\n out:\n+\tfor (i = 0; i < ARRAY_SIZE(to_free); i++)\n+\t\tfree(to_free[i]);\n \tstrbuf_release(&replacement);\n \treturn ret;\n }\n--\n2.37.2\n"},{"id":"462705","messageId":"9ea33608-3a6c-c0ef-9bb3-4bb535009aeb@web.de","threadId":"58393","inReplyTo":"xmqq8rmx1saz.fsf@gitster.g","subject":"[PATCH v2 3/2] diff-no-index: simplify argv index calculation","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-07T11:45:42Z","receivedAt":"2022-09-07T11:45:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since 16bb3d714d (diff --no-index: use parse_options() instead of\ndiff_opt_parse(), 2019-03-24) argc must be 2 if we reach the loop, i.e.\nargc - 2 == 0.  Remove that inconsequential term.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nBonus patch \"while at it\", would have saved me from going \"huh?\".\nGenerated using -U8 for easier review.\n\n diff-no-index.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex a18f6c3c63..18edbdf4b5 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -262,17 +262,17 @@ int diff_no_index(struct rev_info *revs,\n \tif (argc != 2) {\n \t\tif (implicit_no_index)\n \t\t\twarning(_(\"Not a git repository. Use --no-index to \"\n \t\t\t\t  \"compare two paths outside a working tree\"));\n \t\tusage_with_options(diff_no_index_usage, options);\n \t}\n \tFREE_AND_NULL(options);\n \tfor (i = 0; i < 2; i++) {\n-\t\tconst char *p = argv[argc - 2 + i];\n+\t\tconst char *p = argv[i];\n \t\tif (!strcmp(p, \"-\"))\n \t\t\t/*\n \t\t\t * stdin should be spelled as \"-\"; if you have\n \t\t\t * path that is \"-\", spell it as \"./-\".\n \t\t\t */\n \t\t\tp = file_from_standard_input;\n \t\telse if (prefix)\n \t\t\tp = to_free[i] = prefix_filename(prefix, p);\n--\n2.37.2\n"},{"id":"462710","messageId":"220907.86mtbbl5ua.gmgdl@evledraar.gmail.com","threadId":"58393","inReplyTo":"36q9s463-rno9-9rs1-7qsq-5oqq23o30o0r@tzk.qr","subject":"Re: [PATCH v2] diff --no-index: unleak paths[] elements","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-07T12:31:25Z","receivedAt":"2022-09-07T12:36:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 07 2022, Johannes Schindelin wrote:\n\n> Hi Ævar,\n>\n> On Mon, 5 Sep 2022, Ævar Arnfjörð Bjarmason wrote:\n>\n>> On Mon, Sep 05 2022, Johannes Schindelin wrote:\n>>\n>> [...]\n>> > +\tstring_list_clear(&to_release, 1);\n>> [...]\n>>\n>>  * The free_util=1 in your code isn't needed/is a bug, we make no use of\n>>    \"util\" here, so it should be free_util=0\n>\n> Calling it a bug is a bit strong[...]\n\nYes, FWIW I meant that in the sense of \"the author probably didn't mean\nthis\" or \"it was copy/pasted\", but it has no effect currently, as we'll\nalways have NULL \"util\" members.\n\n> , and misses the reason why I did it: future-proofing.\n\nI didn't think it was intentional, but obviously you know better about\nthe intent.\n\n\"Future proofing\" seems like a bad reason for that API use however. If\nyou look at the various \"util\" users some of them want to free() it, and\nsome don't. If you forget to free() the worst you'll have is a memory\nleak, but if you have some boilerplate free() there you'll probably get\na segfault.\n\nWithout knowing what the future code looks like we don't know whether it\nwould make any sense to free() that util use.\n\n> In any case, René took up Junio's ask and provided a new iteration that\n> side-steps this concern altogether, therefore the point is now moot.\n\nIndeed, that patch LGTM.\n"},{"id":"462752","messageId":"xmqqr10nt1qz.fsf@gitster.g","threadId":"58393","inReplyTo":"9ea33608-3a6c-c0ef-9bb3-4bb535009aeb@web.de","subject":"Re: [PATCH v2 3/2] diff-no-index: simplify argv index calculation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-07T19:37:08Z","receivedAt":"2022-09-07T19:37:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Since 16bb3d714d (diff --no-index: use parse_options() instead of\n> diff_opt_parse(), 2019-03-24) argc must be 2 if we reach the loop, i.e.\n> argc - 2 == 0.  Remove that inconsequential term.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Bonus patch \"while at it\", would have saved me from going \"huh?\".\n> Generated using -U8 for easier review.\n\nAll three patches made sense to me.  Thanks.\n\n>\n>  diff-no-index.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index a18f6c3c63..18edbdf4b5 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -262,17 +262,17 @@ int diff_no_index(struct rev_info *revs,\n>  \tif (argc != 2) {\n>  \t\tif (implicit_no_index)\n>  \t\t\twarning(_(\"Not a git repository. Use --no-index to \"\n>  \t\t\t\t  \"compare two paths outside a working tree\"));\n>  \t\tusage_with_options(diff_no_index_usage, options);\n>  \t}\n>  \tFREE_AND_NULL(options);\n>  \tfor (i = 0; i < 2; i++) {\n> -\t\tconst char *p = argv[argc - 2 + i];\n> +\t\tconst char *p = argv[i];\n>  \t\tif (!strcmp(p, \"-\"))\n>  \t\t\t/*\n>  \t\t\t * stdin should be spelled as \"-\"; if you have\n>  \t\t\t * path that is \"-\", spell it as \"./-\".\n>  \t\t\t */\n>  \t\t\tp = file_from_standard_input;\n>  \t\telse if (prefix)\n>  \t\t\tp = to_free[i] = prefix_filename(prefix, p);\n> --\n> 2.37.2\n"}]}