[PATCH 0/4] fix a leak with excludes_file

[PATCH 0/4] fix a leak with excludes_file

[Rubén Justo]

Having ...
	[core]
        	excludesFile = /some/global/path/.gitignore

... this triggers a leak:

	$ GIT_EDITOR=: git rebase -i HEAD
	Successfully rebased and updated detached HEAD.

	=================================================================
	==...==ERROR: LeakSanitizer: detected memory leaks

	Direct leak of 60 byte(s) in 1 object(s) allocated from:
	    #0  ... in realloc 
	    #1  ... in xrealloc wrapper.c:137
	    #2  ... in strbuf_grow strbuf.c:112
	    #3  ... in strbuf_add strbuf.c:311
	    #4  ... in strbuf_addstr strbuf.h:310
	    #5  ... in interpolate_path path.c:771
	    #6  ... in git_config_pathname config.c:1352
	    #7  ... in git_default_core_config config.c:1588
	    #8  ... in git_default_config config.c:1791
	    #9  ... in rebase_config builtin/rebase.c:801
	    #10 ... in configset_iter config.c:2161
	    #11 ... in repo_config config.c:2540
	    #12 ... in git_config config.c:2663
	    #13 ... in cmd_rebase builtin/rebase.c:1187
	    #14 ... in run_builtin git.c:469
	    #15 ... in handle_builtin git.c:724
	    #16 ... in run_argv git.c:788
	    #17 ... in cmd_main git.c:923
	    #18 ... in main common-main.c:62

It happens because we parse twice the configuration: 

	$ GIT_EDITOR=: gdb --ex "break git_config" --ex "run" --args git rebase -i HEAD
	(gdb) bt
	#0  git_config () at config.c:2663
	#1  ... in cmd_rebase () at builtin/rebase.c:1187
	#2  ... in run_builtin () at git.c:469
	#3  ... in handle_builtin () at git.c:724
	#4  ... in run_argv () at git.c:788
	#5  ... in cmd_main () at git.c:923
	#6  ... in main () at common-main.c:62
	(gdb) c
	(gdb) bt
	#0  git_config () at config.c:2663
	#1  ... in sequencer_init_config () at sequencer.c:291
	#2  ... in get_replay_opts () at builtin/rebase.c:161
	#3  ... in do_interactive_rebase () at builtin/rebase.c:271
	#4  ... in run_sequencer_rebase () at builtin/rebase.c:339
	#5  ... in run_specific_rebase () at builtin/rebase.c:705
	#6  ... in cmd_rebase () at builtin/rebase.c:1830
	#7  ... in run_builtin () at git.c:469
	#8  ... in handle_builtin () at git.c:724
	#9  ... in run_argv () at git.c:788
	#10 ... in cmd_main () at git.c:923
	#11 ... in main () at common-main.c:62

We call twice to git_config(): first to get the main git-branch(1) options,
and second to get the ones related to the sequencer.

Due to how git_config() works the global configuration is parsed twice,
therefore if core.excludesFile is set, it will be allocated twice.

A free() before the git_config_pathname() can be a simpler fix, but I
think this series offers a better approach, perhaps applicable to other
potential similar leaks.

Rubén Justo (4):
  path.c: introduce strbuf_interpolate_path
  config.c: introduce git_config_strbuf_pathname
  environment.c: convert excludes_file to struct strbuf
  t7300: mark as leak-free

 config.c         | 12 +++++++++++-
 config.h         |  2 ++
 dir.c            | 13 +++++++++----
 environment.c    |  2 +-
 environment.h    |  2 +-
 path.c           | 20 ++++++++++++++------
 path.h           |  1 +
 t/t7300-clean.sh |  1 +
 8 files changed, 40 insertions(+), 13 deletions(-)

-- 
2.44.0.697.g9b33b46f29

[PATCH 1/4] path.c: introduce strbuf_interpolate_path

[Rubén Justo]

Factorize interpolate_path to have a similar function that uses a
strbuf, instead of allocating a new string, to return the interpolated
path.

It will allow us to avoid some allocs and also some frees, which we will
take advantage of in the next commits.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 path.c | 20 ++++++++++++++------
 path.h |  1 +
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/path.c b/path.c
index 8bb223c92c..7a1a1a9bc0 100644
--- a/path.c
+++ b/path.c
@@ -737,8 +737,16 @@ static struct passwd *getpw_str(const char *username, size_t len)
 char *interpolate_path(const char *path, int real_home)
 {
 	struct strbuf user_path = STRBUF_INIT;
+
+	return strbuf_interpolate_path(path, real_home, &user_path);
+}
+
+char *strbuf_interpolate_path(const char *path, int real_home, struct strbuf* dst)
+{
 	const char *to_copy = path;
 
+	strbuf_reset(dst);
+
 	if (!path)
 		goto return_null;
 
@@ -754,9 +762,9 @@ char *interpolate_path(const char *path, int real_home)
 			if (!home)
 				goto return_null;
 			if (real_home)
-				strbuf_add_real_path(&user_path, home);
+				strbuf_add_real_path(dst, home);
 			else
-				strbuf_addstr(&user_path, home);
+				strbuf_addstr(dst, home);
 #ifdef GIT_WINDOWS_NATIVE
 			convert_slashes(user_path.buf);
 #endif
@@ -764,14 +772,14 @@ char *interpolate_path(const char *path, int real_home)
 			struct passwd *pw = getpw_str(username, username_len);
 			if (!pw)
 				goto return_null;
-			strbuf_addstr(&user_path, pw->pw_dir);
+			strbuf_addstr(dst, pw->pw_dir);
 		}
 		to_copy = first_slash;
 	}
-	strbuf_addstr(&user_path, to_copy);
-	return strbuf_detach(&user_path, NULL);
+	strbuf_addstr(dst, to_copy);
+	return dst->buf;
 return_null:
-	strbuf_release(&user_path);
+	strbuf_release(dst);
 	return NULL;
 }
 
diff --git a/path.h b/path.h
index e053effef2..da7e5384a3 100644
--- a/path.h
+++ b/path.h
@@ -185,6 +185,7 @@ int calc_shared_perm(int mode);
 int adjust_shared_perm(const char *path);
 
 char *interpolate_path(const char *path, int real_home);
+char *strbuf_interpolate_path(const char *path, int real_home, struct strbuf *dst);
 const char *enter_repo(const char *path, int strict);
 const char *remove_leading_path(const char *in, const char *prefix);
 const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
-- 
2.44.0.697.g9b33b46f29

[PATCH 2/4] config.c: introduce git_config_strbuf_pathname

[Rubén Justo]

Add a new git_config_strbuf_pathname function, similar to
git_config_pathname, that works with a strbuf relying on the recently
introduced strbuf_interpolate_path.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 config.c | 10 ++++++++++
 config.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/config.c b/config.c
index eebce8c7e0..9beeb63b50 100644
--- a/config.c
+++ b/config.c
@@ -1355,6 +1355,16 @@ int git_config_pathname(const char **dest, const char *var, const char *value)
 	return 0;
 }
 
+int git_config_strbuf_pathname(struct strbuf *dest, const char *var, const char *value)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	strbuf_interpolate_path(value, 0, dest);
+	if (!dest->len)
+		die(_("failed to expand user dir in: '%s'"), value);
+	return 0;
+}
+
 int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *value)
 {
 	if (!value)
diff --git a/config.h b/config.h
index f4966e3749..e405f1c140 100644
--- a/config.h
+++ b/config.h
@@ -22,6 +22,7 @@
  */
 
 struct object_id;
+struct strbuf;
 
 /* git_config_parse_key() returns these negated: */
 #define CONFIG_INVALID_KEY 1
@@ -287,6 +288,7 @@ int git_config_string(const char **, const char *, const char *);
  * user's home directory when found at the beginning of the path.
  */
 int git_config_pathname(const char **, const char *, const char *);
+int git_config_strbuf_pathname(struct strbuf *, const char *, const char *);
 
 int git_config_expiry_date(timestamp_t *, const char *, const char *);
 int git_config_color(char *, const char *, const char *);
-- 
2.44.0.697.g9b33b46f29

[PATCH 3/4] environment.c: convert excludes_file to struct strbuf

[Rubén Justo]

Make excludes_file a strbuf, so that we don't have to worry about freeing
it if it is set multiple times.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 config.c      |  2 +-
 dir.c         | 13 +++++++++----
 environment.c |  2 +-
 environment.h |  2 +-
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/config.c b/config.c
index 9beeb63b50..bc64af8efa 100644
--- a/config.c
+++ b/config.c
@@ -1595,7 +1595,7 @@ static int git_default_core_config(const char *var, const char *value,
 		return git_config_string(&askpass_program, var, value);
 
 	if (!strcmp(var, "core.excludesfile"))
-		return git_config_pathname(&excludes_file, var, value);
+		return git_config_strbuf_pathname(&excludes_file, var, value);
 
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
diff --git a/dir.c b/dir.c
index 20ebe4cba2..e31ccd8e48 100644
--- a/dir.c
+++ b/dir.c
@@ -3386,10 +3386,15 @@ void setup_standard_excludes(struct dir_struct *dir)
 	dir->exclude_per_dir = ".gitignore";
 
 	/* core.excludesfile defaulting to $XDG_CONFIG_HOME/git/ignore */
-	if (!excludes_file)
-		excludes_file = xdg_config_home("ignore");
-	if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
-		add_patterns_from_file_1(dir, excludes_file,
+	if (!excludes_file.len) {
+		char *str = xdg_config_home("ignore");
+		if (str) {
+			strbuf_addstr(&excludes_file, str);
+			free(str);
+		}
+	}
+	if (excludes_file.len && !access_or_warn(excludes_file.buf, R_OK, 0))
+		add_patterns_from_file_1(dir, excludes_file.buf,
 					 dir->untracked ? &dir->internal.ss_excludes_file : NULL);
 
 	/* per repository user preference */
diff --git a/environment.c b/environment.c
index a73ba9c12c..b4c66e7153 100644
--- a/environment.c
+++ b/environment.c
@@ -60,7 +60,7 @@ size_t delta_base_cache_limit = 96 * 1024 * 1024;
 unsigned long big_file_threshold = 512 * 1024 * 1024;
 const char *editor_program;
 const char *askpass_program;
-const char *excludes_file;
+struct strbuf excludes_file = STRBUF_INIT;
 enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
 enum eol core_eol = EOL_UNSET;
 int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
diff --git a/environment.h b/environment.h
index 05fd94d7be..114e3dde99 100644
--- a/environment.h
+++ b/environment.h
@@ -222,7 +222,7 @@ extern const char *git_log_output_encoding;
 
 extern const char *editor_program;
 extern const char *askpass_program;
-extern const char *excludes_file;
+extern struct strbuf excludes_file;
 
 /*
  * Should we print an ellipsis after an abbreviated SHA-1 value
-- 
2.44.0.697.g9b33b46f29

[PATCH 4/4] t7300: mark as leak-free

[Rubén Justo]

Since the previous commit this test does not leak.

Mark it with TEST_PASSES_SANITIZE_LEAK=true so to make the leak
checkers happy.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t7300-clean.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 1f7201eb60..0aae0dee67 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -5,6 +5,7 @@
 
 test_description='git clean basic tests'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 git config clean.requireForce no
-- 
2.44.0.697.g9b33b46f29

Re: [PATCH 0/4] fix a leak with excludes_file

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> We call twice to git_config(): first to get the main git-branch(1) options,
> and second to get the ones related to the sequencer.

The diagnosis of the control flow that leads to overwriting the
value assigned earlier to "excludes_file" is quite correct, but
I find that your fix is somehow not addressing the real issue.

We used to assume that excludes_file *always* points at a piece of
memory that somebody else owns.  But seeing the two places that
assign to the variable, both actually give the variable a piece of
memory allocated for the caller.

 - The one in config.c that reads core.excludesfile via
   git_config_pathname(), which calls interpolate_path(), whose
   return value is always allocated.

 - The one in dir.c that sets up standard excludes uses
   xdg_config_home() that returns a value obtained from mkpathdup().

The core of the fix ought to be just like this, but to lose the cast
that discards the constness away, the fallouts will be large (if you
remove "const" from the first parameter to git_config_pathname(),
the compiler will tell you where the fallouts are).

We may want to eventually fix the type of the first parameter of the
git_config_pathmame() function, but I somehow doubt that your
approach gets us any closer to that future.  Adding a parallel API
next to the existing git_config_pathname() and interpolate_path()
and convert only these callers that touch excludes_file would not
help other callers that hold the pointer git_config_pathname()
returned.

 config.c         | 4 +++-
 t/t7300-clean.sh | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git c/config.c w/config.c
index eebce8c7e0..ae3652b08f 100644
--- c/config.c
+++ w/config.c
@@ -1584,8 +1584,10 @@ static int git_default_core_config(const char *var, const char *value,
 	if (!strcmp(var, "core.askpass"))
 		return git_config_string(&askpass_program, var, value);
 
-	if (!strcmp(var, "core.excludesfile"))
+	if (!strcmp(var, "core.excludesfile")) {
+		free((char *)excludes_file);
 		return git_config_pathname(&excludes_file, var, value);
+	}
 
 	if (!strcmp(var, "core.whitespace")) {
 		if (!value)
diff --git c/t/t7300-clean.sh w/t/t7300-clean.sh
index 1f7201eb60..0aae0dee67 100755
--- c/t/t7300-clean.sh
+++ w/t/t7300-clean.sh
@@ -5,6 +5,7 @@
 
 test_description='git clean basic tests'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 git config clean.requireForce no

Re: [PATCH 0/4] fix a leak with excludes_file

[Rubén Justo]

On Sat, Apr 06, 2024 at 10:53:09AM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > We call twice to git_config(): first to get the main git-branch(1) options,
> > and second to get the ones related to the sequencer.

Obviously I meant git-rebase(1) -- or any other command that uses the
sequencer under the hood.

> Adding a parallel API
> next to the existing git_config_pathname() and interpolate_path()
> and convert only these callers that touch excludes_file would not
> help other callers that hold the pointer git_config_pathname()
> returned.

It does not have to be like that.  We may no longer need the current
and problematic git_config_pathname().  However I did not want to go
that far in this series.

>  config.c         | 4 +++-
>  t/t7300-clean.sh | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git c/config.c w/config.c
> index eebce8c7e0..ae3652b08f 100644
> --- c/config.c
> +++ w/config.c
> @@ -1584,8 +1584,10 @@ static int git_default_core_config(const char *var, const char *value,
>  	if (!strcmp(var, "core.askpass"))
>  		return git_config_string(&askpass_program, var, value);
>  
> -	if (!strcmp(var, "core.excludesfile"))
> +	if (!strcmp(var, "core.excludesfile")) {
> +		free((char *)excludes_file);

Aaah, you prefer this :-( ...  this free is what I was referring to, in
the message you are replying to, as the simpler fix ...

It obviously plugs the leak and so my itch is gone;  therefore OK by me
to the series I have seen that you have already sent with this.

Still, I find the approach in this series more interesting than the
simple free.  The strbuf saves us from subsequent free+alloc's, for
exactly the same "core.excludesFile".  The strbuf states clearly, IMHO,
who owns the pointer.  And other niceties.

Re: [PATCH 0/4] fix a leak with excludes_file

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> It does not have to be like that.  We may no longer need the current
> and problematic git_config_pathname().  However I did not want to go
> that far in this series.

True, but that one and only true interface we will end up with MUST
NOT be strbuf based one, and that is why I said the patch as posted
will not take us into a better future.

And as a stop-gap measure, ...

>> -	if (!strcmp(var, "core.excludesfile"))
>> +	if (!strcmp(var, "core.excludesfile")) {
>> +		free((char *)excludes_file);
>
> Aaah, you prefer this :-( ...  this free is what I was referring to, in
> the message you are replying to, as the simpler fix ...

... obviously this would be much more preferrable.  And when the one
and only true interface comes, we only need to drop the cast from
around here.

Thanks.

Re: [PATCH 0/4] fix a leak with excludes_file

[Rubén Justo]

On Mon, Apr 08, 2024 at 10:36:30AM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > It does not have to be like that.  We may no longer need the current
> > and problematic git_config_pathname().  However I did not want to go
> > that far in this series.
> 
> True, but that one and only true interface we will end up with MUST
> NOT be strbuf based one, and that is why I said the patch as posted
> will not take us into a better future.

It doesn't have to be having a strbuf-based-only interface, either.

It is about introducing an interface where callers:

	- can take advantage of a not-always-allocate interface, and

	- can stop worrying about freeing previous values, when reusing
	  variables.

This latter fixes the leak while the former introduces a nice "if you
set n-times the same value, we'll allocate once".