diff mbox series

[v2,3/3] config.c: free(expanded) before die(), work around GCC oddity

Message ID patch-v2-3.3-d812358e331-20211021T195133Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series refs.c + config.c: plug memory leaks | expand

Commit Message

Ævar Arnfjörð Bjarmason Oct. 21, 2021, 7:54 p.m. UTC
On my GCC version (10.2.1-6), but not the clang I have available t0017
will fail under SANITIZE=leak on optimization levels higher than -O0,
which is annoying when combined with the change in 956d2e4639b (tests:
add a test mode for SANITIZE=leak, run it in CI, 2021-09-23).

We really do have a memory leak here in either case, as e.g. running
the pre-image under valgrind(1) will reveal. It's documented
SANITIZE=leak (and "address", which exhibits the same behavior) might
interact with compiler optimization in this way in some cases, and
since this function is called recursively it's going to be especially
interesting as an optimization target.

Let's work around this issue by freeing the "expanded" memory before
we call die(), using the "goto cleanup" pattern introduced in the
preceding commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 config.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Junio C Hamano Oct. 21, 2021, 11:32 p.m. UTC | #1
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  cleanup:
>  	strbuf_release(&buf);
>  	free(expanded);
> -	return ret;
> +	if (!die_depth)
> +		return ret;
> +	die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,
> +	    !cf ? "<unknown>" : cf->name ? cf->name : "the command line");
>  }

Yuck.  With or without compiler bugs, this code is too ugly to live,
isn't it?
diff mbox series

Patch

diff --git a/config.c b/config.c
index c5873f3a706..ab40decaeba 100644
--- a/config.c
+++ b/config.c
@@ -132,6 +132,7 @@  static int handle_path_include(const char *path, struct config_include_data *inc
 	int ret = 0;
 	struct strbuf buf = STRBUF_INIT;
 	char *expanded;
+	int die_depth = 0;
 
 	if (!path)
 		return config_error_nonbool("include.path");
@@ -161,18 +162,20 @@  static int handle_path_include(const char *path, struct config_include_data *inc
 	}
 
 	if (!access_or_die(path, R_OK, 0)) {
-		if (++inc->depth > MAX_INCLUDE_DEPTH)
-			die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,
-			    !cf ? "<unknown>" :
-			    cf->name ? cf->name :
-			    "the command line");
+		if (++inc->depth > MAX_INCLUDE_DEPTH) {
+			die_depth = 1;
+			goto cleanup;
+		}
 		ret = git_config_from_file(git_config_include, path, inc);
 		inc->depth--;
 	}
 cleanup:
 	strbuf_release(&buf);
 	free(expanded);
-	return ret;
+	if (!die_depth)
+		return ret;
+	die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,
+	    !cf ? "<unknown>" : cf->name ? cf->name : "the command line");
 }
 
 static void add_trailing_starstar_for_dir(struct strbuf *pat)