Re: [PATCH v2] diff --no-index: unleak paths[] elements

["René Scharfe" <l.s.r@web.de>]

Am 03.09.22 um 01:49 schrieb Junio C Hamano:
> "git diff --no-index" codepath starts with the two elements in
> argv[] and munges them into two paths to be compared, stored in a
> separate path[] arrays.  The munging is implemented in a rather
> haphazard way, sometimes overwriting old version with a new copy,
> and sometimes a constant string assigned to path[], making it
> impossible to release the resources properly:
>
>  * A single dash "-" from the command line is a special signal that
>    the standard input is used for the side to be compared, and is
>    internally replaced with a copy of string "-" at a known address.
>
>  * When run in a subdirectory, full paths to the two paths are
>    allocated and placed in path[].
>
>  * After the above happens, when comparing a file with a directory,
>    the directory side is replaced with the path to a file in the
>    directory with the same name as the file.
>
> This was perfectly fine for just two strings that are pathnames used
> during the lifetime of the program and cleaned up upon program exit,
> but it gets in the way when leak sanitizer is in effect.  The third
> step can be losing the full path that was allocated in the second
> step, but it is not easy to tell if its input is an allocated piece
> of memory to begin with.
>
> Loosen the earlier two steps a bit so that elements of the path[]
> array that come to the directory/file comparison code are either the
> singleton "-" or an allocated piece of memory.  Use that knowledge
> in the third step to release an allocated piece of memory when it
> replaces the path to a directory with the path to a file in that
> directory, and also at the end to release the two elements of the
> path[] array as needed.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * The previous one allowed strbuf_release() to free replacement.buf
>    which may be used in path[0] or path[1] potentially leading to
>    double freeing.  The kosher way may be to use strbuf_detach() in
>    fixup_paths(), but this is a simpler fix, it is getting late in
>    the day, and I am getting sick of fighting the leak-checker, so...
>
>  diff-no-index.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 9a8b09346b..77a126469b 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -208,6 +208,14 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi
>  	strbuf_addstr(path, tail ? tail + 1 : file);
>  }
>
> +static void free_allocated_path(const char *path)
> +{
> +	if (!path ||

How can path be NULL?  And if it was, why shield free(3) from it?

> +	    (path == file_from_standard_input))
> +		return;
> +	free((char *)path);
> +}
> +
>  /*
>   * DWIM "diff D F" into "diff D/F F" and "diff F D" into "diff F D/F"
>   * Note that we append the basename of F to D/, so "diff a/b/file D"
> @@ -226,9 +234,11 @@ static void fixup_paths(const char **path, struct strbuf *replacement)
>  		return;
>  	if (isdir0) {
>  		append_basename(replacement, path[0], path[1]);
> +		free_allocated_path(path[0]);
>  		path[0] = replacement->buf;
>  	} else {
>  		append_basename(replacement, path[1], path[0]);
> +		free_allocated_path(path[1]);
>  		path[1] = replacement->buf;
>  	}
>  }
> @@ -274,6 +284,8 @@ int diff_no_index(struct rev_info *revs,
>  			p = file_from_standard_input;
>  		else if (prefix)
>  			p = prefix_filename(prefix, p);
> +		else
> +			p = xstrdup(p);

prefix_filename(NULL, p) is basically the same as xstrdup(p), so those
two conditional branches could be joined.

>  		paths[i] = p;
>  	}
>
> @@ -294,13 +306,21 @@ int diff_no_index(struct rev_info *revs,
>  	setup_diff_pager(&revs->diffopt);
>  	revs->diffopt.flags.exit_with_status = 1;
>
> -	if (queue_diff(&revs->diffopt, paths[0], paths[1]))
> +	if (queue_diff(&revs->diffopt, paths[0], paths[1])) {
> +		free_allocated_path(paths[0]);
> +		free_allocated_path(paths[1]);
>  		return 1;
> +	}
>  	diff_set_mnemonic_prefix(&revs->diffopt, "1/", "2/");
>  	diffcore_std(&revs->diffopt);
>  	diff_flush(&revs->diffopt);
>
> -	strbuf_release(&replacement);
> +	/*
> +	 * do not strbuf_release(&replacement), as it is in paths[]
> +	 * when replacement was actually used.
> +	 */
> +	free_allocated_path(paths[0]);
> +	free_allocated_path(paths[1]);
>
>  	/*
>  	 * The return code for --no-index imitates diff(1):

Perhaps avoid the need for that comment by moving that strbuf to where
it's used and have it spend its full lifecycle there?  Something like:

---
 diff-no-index.c | 50 ++++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index 77a126469b..9f8b78f173 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -196,18 +196,6 @@ static int queue_diff(struct diff_options *o,
 	}
 }

-/* append basename of F to D */
-static void append_basename(struct strbuf *path, const char *dir, const char *file)
-{
-	const char *tail = strrchr(file, '/');
-
-	strbuf_addstr(path, dir);
-	while (path->len && path->buf[path->len - 1] == '/')
-		path->len--;
-	strbuf_addch(path, '/');
-	strbuf_addstr(path, tail ? tail + 1 : file);
-}
-
 static void free_allocated_path(const char *path)
 {
 	if (!path ||
@@ -216,12 +204,28 @@ static void free_allocated_path(const char *path)
 	free((char *)path);
 }

+/* append basename of F to D */
+static void append_basename(const char **dir, const char *file)
+{
+	const char *tail = strrchr(file, '/');
+	struct strbuf path = STRBUF_INIT;
+
+	strbuf_addstr(&path, *dir);
+	while (path.len && path.buf[path.len - 1] == '/')
+		path.len--;
+	strbuf_addch(&path, '/');
+	strbuf_addstr(&path, tail ? tail + 1 : file);
+
+	free_allocated_path(*dir);
+	*dir = strbuf_detach(&path, NULL);
+}
+
 /*
  * DWIM "diff D F" into "diff D/F F" and "diff F D" into "diff F D/F"
  * Note that we append the basename of F to D/, so "diff a/b/file D"
  * becomes "diff a/b/file D/file", not "diff a/b/file D/a/b/file".
  */
-static void fixup_paths(const char **path, struct strbuf *replacement)
+static void fixup_paths(const char **path)
 {
 	unsigned int isdir0, isdir1;

@@ -232,15 +236,10 @@ static void fixup_paths(const char **path, struct strbuf *replacement)
 	isdir1 = is_directory(path[1]);
 	if (isdir0 == isdir1)
 		return;
-	if (isdir0) {
-		append_basename(replacement, path[0], path[1]);
-		free_allocated_path(path[0]);
-		path[0] = replacement->buf;
-	} else {
-		append_basename(replacement, path[1], path[0]);
-		free_allocated_path(path[1]);
-		path[1] = replacement->buf;
-	}
+	if (isdir0)
+		append_basename(&path[0], path[1]);
+	else
+		append_basename(&path[1], path[0]);
 }

 static const char * const diff_no_index_usage[] = {
@@ -254,7 +253,6 @@ int diff_no_index(struct rev_info *revs,
 {
 	int i, no_index;
 	const char *paths[2];
-	struct strbuf replacement = STRBUF_INIT;
 	const char *prefix = revs->prefix;
 	struct option no_index_options[] = {
 		OPT_BOOL_F(0, "no-index", &no_index, "",
@@ -289,7 +287,7 @@ int diff_no_index(struct rev_info *revs,
 		paths[i] = p;
 	}

-	fixup_paths(paths, &replacement);
+	fixup_paths(paths);

 	revs->diffopt.skip_stat_unmatch = 1;
 	if (!revs->diffopt.output_format)
@@ -315,10 +313,6 @@ int diff_no_index(struct rev_info *revs,
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);

-	/*
-	 * do not strbuf_release(&replacement), as it is in paths[]
-	 * when replacement was actually used.
-	 */
 	free_allocated_path(paths[0]);
 	free_allocated_path(paths[1]);

--
2.37.2