git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Johannes Schindelin" <johannes.schindelin@gmx.de>
Subject: [PATCH] unleak paths allocated in "diff --no-index"
Date: Fri, 02 Sep 2022 14:27:27 -0700	[thread overview]
Message-ID: <xmqqilm579hc.fsf@gitster.g> (raw)

"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>
---

 * Because we never pass NULL to the new helper, !path check is not
   strictly necessary, but it is conventional for free()-like
   functions to take NULL and safely become a no-op.

 diff-no-index.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git c/diff-no-index.c w/diff-no-index.c
index 9a8b09346b..2770b7d15a 100644
--- c/diff-no-index.c
+++ w/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 ||
+	    (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);
 		paths[i] = p;
 	}
 
@@ -294,13 +306,18 @@ 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);
+	free_allocated_path(paths[0]);
+	free_allocated_path(paths[1]);
 
 	/*
 	 * The return code for --no-index imitates diff(1):

             reply	other threads:[~2022-09-02 21:27 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-02 21:27 Junio C Hamano [this message]
2022-09-02 23:49 ` [PATCH v2] diff --no-index: unleak paths[] elements Junio C Hamano
2022-09-03  6:00   ` René Scharfe
2022-09-05 20:26     ` Junio C Hamano
2022-09-06 12:31       ` [PATCH 1/2] diff-no-index: release strbuf on queue error René Scharfe
2022-09-07  6:01         ` René Scharfe
2022-09-06 12:31       ` [PATCH 2/2] diff-no-index: release prefixed filenames René Scharfe
2022-09-07 10:03         ` Johannes Schindelin
2022-09-07 11:19           ` René Scharfe
2022-09-07 11:36       ` [PATCH v2 1/2] diff-no-index: release strbuf on queue error René Scharfe
2022-09-07 11:37       ` [PATCH v2 2/2] diff-no-index: release prefixed filenames René Scharfe
2022-09-07 11:45       ` [PATCH v2 3/2] diff-no-index: simplify argv index calculation René Scharfe
2022-09-07 19:37         ` Junio C Hamano
2022-09-05 10:03   ` [PATCH v2] diff --no-index: unleak paths[] elements Johannes Schindelin
2022-09-05 11:01     ` Ævar Arnfjörð Bjarmason
2022-09-07 10:06       ` Johannes Schindelin
2022-09-07 12:31         ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqilm579hc.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).