[PATCH 0/3] a few more leak fixes

[PATCH 0/3] a few more leak fixes

[Jeff King]

I was really bothered that using clang with SANITIZE=leak found a leak
that gcc didn't. And then I was doubly bothered to find that there is
one that gcc finds that clang doesn't!

I don't think either of these are urgent or important leaks on their
own, but the flaky nature of the results makes it annoying while trying
to find and clean up other leaks. So here are some fixes. Patches 1 and
3 are those two cases, respectively. Patch 2 is a more interesting
leak-fix enabled by the infrastructure added in patch 1.

  [1/3]: decorate: add clear_decoration() function
  [2/3]: revision: clear decoration structs during release_revisions()
  [3/3]: daemon: free listen_addr before returning

 daemon.c                         | 37 ++++++++++++++++++--------------
 decorate.c                       | 15 +++++++++++++
 decorate.h                       | 10 +++++++++
 line-log.c                       | 10 +++++++++
 line-log.h                       |  2 ++
 revision.c                       |  9 ++++++++
 t/helper/test-example-decorate.c |  2 ++
 t/t4217-log-limit.sh             |  1 +
 t/t5811-proto-disable-git.sh     |  2 ++
 t/t9004-example.sh               |  2 ++
 10 files changed, 74 insertions(+), 16 deletions(-)

-Peff

[PATCH 1/3] decorate: add clear_decoration() function

[Jeff King]

There's not currently any way to free the resources associated with a
decoration struct. As a result, we have several memory leaks which
cannot easily be plugged.

Let's add a "clear" function and make use of it in the example code of
t9004. This removes the only leak from that script, so we can mark it as
passing the leak sanitizer.

Curiously this leak is found only when running SANITIZE=leak with clang,
but not with gcc.  But it is a bog-standard leak: we allocate some
memory in a local variable struct, and then exit main() without
releasing it. I'm not sure why gcc doesn't find it. After this
patch, both compilers report it as leak-free.

Note that the clear function takes a callback to free the individual
entries. That's not needed for our example (which is just decorating
with ints), but will be for real callers.

Signed-off-by: Jeff King <peff@peff.net>
---
 decorate.c                       | 15 +++++++++++++++
 decorate.h                       | 10 ++++++++++
 t/helper/test-example-decorate.c |  2 ++
 t/t9004-example.sh               |  2 ++
 4 files changed, 29 insertions(+)

diff --git a/decorate.c b/decorate.c
index a5c43c0c14..69aeb142b4 100644
--- a/decorate.c
+++ b/decorate.c
@@ -81,3 +81,18 @@ void *lookup_decoration(struct decoration *n, const struct object *obj)
 			j = 0;
 	}
 }
+
+void clear_decoration(struct decoration *n, void (*free_cb)(void *))
+{
+	if (free_cb) {
+		unsigned int i;
+		for (i = 0; i < n->size; i++) {
+			void *d = n->entries[i].decoration;
+			if (d)
+				free_cb(d);
+		}
+	}
+
+	FREE_AND_NULL(n->entries);
+	n->size = n->nr = 0;
+}
diff --git a/decorate.h b/decorate.h
index ee43dee1f0..cdeb17c9df 100644
--- a/decorate.h
+++ b/decorate.h
@@ -58,4 +58,14 @@ void *add_decoration(struct decoration *n, const struct object *obj, void *decor
  */
 void *lookup_decoration(struct decoration *n, const struct object *obj);
 
+/*
+ * Clear all decoration entries, releasing any memory used by the structure.
+ * If free_cb is not NULL, it is called for every decoration value currently
+ * stored.
+ *
+ * After clearing, the decoration struct can be used again. The "name" field is
+ * retained.
+ */
+void clear_decoration(struct decoration *n, void (*free_cb)(void *));
+
 #endif
diff --git a/t/helper/test-example-decorate.c b/t/helper/test-example-decorate.c
index 2ed910adaa..8f59f6be4c 100644
--- a/t/helper/test-example-decorate.c
+++ b/t/helper/test-example-decorate.c
@@ -72,5 +72,7 @@ int cmd__example_decorate(int argc UNUSED, const char **argv UNUSED)
 	if (objects_noticed != 2)
 		BUG("should have 2 objects");
 
+	clear_decoration(&n, NULL);
+
 	return 0;
 }
diff --git a/t/t9004-example.sh b/t/t9004-example.sh
index 7e8894a4a7..590aab0304 100755
--- a/t/t9004-example.sh
+++ b/t/t9004-example.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='check that example code compiles and runs'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'decorate' '
-- 
2.42.0.836.g4b88bf80c5

[PATCH 2/3] revision: clear decoration structs during release_revisions()

[Jeff King]

The point of release_revisions() is to free memory associated with the
rev_info struct, but we have several "struct decoration" members that
are left untouched. Since the previous commit introduced a function to
do that, we can just call it.

We do have to provide some specialized callbacks to map the void
pointers onto real ones (the alternative would be casting the existing
function pointers; this generally works because "void *" is usually
interchangeable with a struct pointer, but it is technically forbidden
by the standard).

Since the line-log code does not expose the type it stores in the
decoration (nor of course the function to free it), I put this behind a
generic line_log_free() entry point. It's possible we may need to add
more line-log specific bits anyway (running t4211 shows a number of
other leaks in the line-log code).

While this doubtless cleans up many leaks triggered by the test suite,
the only script which becomes leak-free is t4217, as it does very little
beyond a simple traversal (its existing leak was from the use of
--children, which is now fixed).

Signed-off-by: Jeff King <peff@peff.net>
---
 line-log.c           | 10 ++++++++++
 line-log.h           |  2 ++
 revision.c           |  9 +++++++++
 t/t4217-log-limit.sh |  1 +
 4 files changed, 22 insertions(+)

diff --git a/line-log.c b/line-log.c
index 790ab73212..24a1ecb677 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1327,3 +1327,13 @@ int line_log_filter(struct rev_info *rev)
 
 	return 0;
 }
+
+static void free_void_line_log_data(void *data)
+{
+	free_line_log_data(data);
+}
+
+void line_log_free(struct rev_info *rev)
+{
+	clear_decoration(&rev->line_log_data, free_void_line_log_data);
+}
diff --git a/line-log.h b/line-log.h
index adff361b1b..4291da8d79 100644
--- a/line-log.h
+++ b/line-log.h
@@ -60,4 +60,6 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev,
 
 int line_log_print(struct rev_info *rev, struct commit *commit);
 
+void line_log_free(struct rev_info *rev);
+
 #endif /* LINE_LOG_H */
diff --git a/revision.c b/revision.c
index e789834dd1..219dc76716 100644
--- a/revision.c
+++ b/revision.c
@@ -3083,6 +3083,11 @@ static void release_revisions_mailmap(struct string_list *mailmap)
 
 static void release_revisions_topo_walk_info(struct topo_walk_info *info);
 
+static void free_void_commit_list(void *list)
+{
+	free_commit_list(list);
+}
+
 void release_revisions(struct rev_info *revs)
 {
 	free_commit_list(revs->commits);
@@ -3100,6 +3105,10 @@ void release_revisions(struct rev_info *revs)
 	diff_free(&revs->pruning);
 	reflog_walk_info_release(revs->reflog_info);
 	release_revisions_topo_walk_info(revs->topo_walk_info);
+	clear_decoration(&revs->children, free_void_commit_list);
+	clear_decoration(&revs->merge_simplification, free);
+	clear_decoration(&revs->treesame, free);
+	line_log_free(revs);
 }
 
 static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
diff --git a/t/t4217-log-limit.sh b/t/t4217-log-limit.sh
index 6e01e2629c..613f0710e9 100755
--- a/t/t4217-log-limit.sh
+++ b/t/t4217-log-limit.sh
@@ -2,6 +2,7 @@
 
 test_description='git log with filter options limiting the output'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup test' '
-- 
2.42.0.836.g4b88bf80c5

[PATCH 3/3] daemon: free listen_addr before returning

[Jeff King]

We build up a string list of listen addresses from the command-line
arguments, but never free it. This causes t5811 to complain of a leak
(though curiously it seems to do so only when compiled with gcc, not
with clang).

To handle this correctly, we have to do a little refactoring:

  - there are two exit points from the main function, depending on
    whether we are entering the main loop or serving a single client
    (since rather than a traditional fork model, we re-exec ourselves
    with the extra "--serve" argument to accommodate Windows).

    We don't need --listen at all in the --serve case, of course, but it
    is passed along by the parent daemon, which simply copies all of the
    command-line options it got.

  - we just "return serve()" to run the main loop, giving us no chance
    to do any cleanup

So let's use a "ret" variable to store the return code, and give
ourselves a single exit point at the end. That gives us one place to do
cleanup.

Note that this code also uses the "use a no-dup string-list, but
allocate strings we add to it" trick, meaning string_list_clear() will
not realize it should free them. We can fix this by switching to a "dup"
string-list, but using the "append_nodup" function to add to it (this is
preferable to tweaking the strdup_strings flag before clearing, as it
puts all the subtle memory-ownership code together).

Signed-off-by: Jeff King <peff@peff.net>
---
Diff is a little messy due to indentation. Viewing with "-w" makes it
more clear, or just viewing the post-image.

 daemon.c                     | 37 ++++++++++++++++++++----------------
 t/t5811-proto-disable-git.sh |  2 ++
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/daemon.c b/daemon.c
index f5e597114b..17d331b2f3 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1243,19 +1243,20 @@ static int serve(struct string_list *listen_addr, int listen_port,
 int cmd_main(int argc, const char **argv)
 {
 	int listen_port = 0;
-	struct string_list listen_addr = STRING_LIST_INIT_NODUP;
+	struct string_list listen_addr = STRING_LIST_INIT_DUP;
 	int serve_mode = 0, inetd_mode = 0;
 	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
 	int detach = 0;
 	struct credentials *cred = NULL;
 	int i;
+	int ret;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		const char *v;
 
 		if (skip_prefix(arg, "--listen=", &v)) {
-			string_list_append(&listen_addr, xstrdup_tolower(v));
+			string_list_append_nodup(&listen_addr, xstrdup_tolower(v));
 			continue;
 		}
 		if (skip_prefix(arg, "--port=", &v)) {
@@ -1437,22 +1438,26 @@ int cmd_main(int argc, const char **argv)
 			die_errno("failed to redirect stderr to /dev/null");
 	}
 
-	if (inetd_mode || serve_mode)
-		return execute();
+	if (inetd_mode || serve_mode) {
+		ret = execute();
+	} else {
+		if (detach) {
+			if (daemonize())
+				die("--detach not supported on this platform");
+		}
 
-	if (detach) {
-		if (daemonize())
-			die("--detach not supported on this platform");
-	}
+		if (pid_file)
+			write_file(pid_file, "%"PRIuMAX, (uintmax_t) getpid());
 
-	if (pid_file)
-		write_file(pid_file, "%"PRIuMAX, (uintmax_t) getpid());
+		/* prepare argv for serving-processes */
+		strvec_push(&cld_argv, argv[0]); /* git-daemon */
+		strvec_push(&cld_argv, "--serve");
+		for (i = 1; i < argc; ++i)
+			strvec_push(&cld_argv, argv[i]);
 
-	/* prepare argv for serving-processes */
-	strvec_push(&cld_argv, argv[0]); /* git-daemon */
-	strvec_push(&cld_argv, "--serve");
-	for (i = 1; i < argc; ++i)
-		strvec_push(&cld_argv, argv[i]);
+		ret = serve(&listen_addr, listen_port, cred);
+	}
 
-	return serve(&listen_addr, listen_port, cred);
+	string_list_clear(&listen_addr, 0);
+	return ret;
 }
diff --git a/t/t5811-proto-disable-git.sh b/t/t5811-proto-disable-git.sh
index 8ac6b2a1d0..ed773e7432 100755
--- a/t/t5811-proto-disable-git.sh
+++ b/t/t5811-proto-disable-git.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test disabling of git-over-tcp in clone/fetch'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-proto-disable.sh"
 . "$TEST_DIRECTORY/lib-git-daemon.sh"
-- 
2.42.0.836.g4b88bf80c5

Re: [PATCH 2/3] revision: clear decoration structs during release_revisions()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

Wow, nested maze of callbacks make my head spin ;-) but they all
look reasonable.  Thanks.

> diff --git a/line-log.c b/line-log.c
> index 790ab73212..24a1ecb677 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -1327,3 +1327,13 @@ int line_log_filter(struct rev_info *rev)
>  
>  	return 0;
>  }
> +
> +static void free_void_line_log_data(void *data)
> +{
> +	free_line_log_data(data);
> +}
> +
> +void line_log_free(struct rev_info *rev)
> +{
> +	clear_decoration(&rev->line_log_data, free_void_line_log_data);
> +}
> diff --git a/line-log.h b/line-log.h
> index adff361b1b..4291da8d79 100644
> --- a/line-log.h
> +++ b/line-log.h
> @@ -60,4 +60,6 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev,
>  
>  int line_log_print(struct rev_info *rev, struct commit *commit);
>  
> +void line_log_free(struct rev_info *rev);
> +
>  #endif /* LINE_LOG_H */
> diff --git a/revision.c b/revision.c
> index e789834dd1..219dc76716 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -3083,6 +3083,11 @@ static void release_revisions_mailmap(struct string_list *mailmap)
>  
>  static void release_revisions_topo_walk_info(struct topo_walk_info *info);
>  
> +static void free_void_commit_list(void *list)
> +{
> +	free_commit_list(list);
> +}
> +
>  void release_revisions(struct rev_info *revs)
>  {
>  	free_commit_list(revs->commits);
> @@ -3100,6 +3105,10 @@ void release_revisions(struct rev_info *revs)
>  	diff_free(&revs->pruning);
>  	reflog_walk_info_release(revs->reflog_info);
>  	release_revisions_topo_walk_info(revs->topo_walk_info);
> +	clear_decoration(&revs->children, free_void_commit_list);
> +	clear_decoration(&revs->merge_simplification, free);
> +	clear_decoration(&revs->treesame, free);
> +	line_log_free(revs);
>  }
>  
>  static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
> diff --git a/t/t4217-log-limit.sh b/t/t4217-log-limit.sh
> index 6e01e2629c..613f0710e9 100755
> --- a/t/t4217-log-limit.sh
> +++ b/t/t4217-log-limit.sh
> @@ -2,6 +2,7 @@
>  
>  test_description='git log with filter options limiting the output'
>  
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
>  test_expect_success 'setup test' '

Re: [PATCH 2/3] revision: clear decoration structs during release_revisions()

[Jeff King]

On Thu, Oct 05, 2023 at 04:00:54PM -0700, Junio C Hamano wrote:

> Wow, nested maze of callbacks make my head spin ;-) but they all
> look reasonable.  Thanks.

Yeah, I don't love those one-liner callbacks just to handle the cast.

The other alternative is to write some kind of for_each_decoration()
macro, but I think it ends up in the usual macro hell (requiring the
caller to provide iterator variables, hanging half-open braces, and so
on). It might be worth it if iterating could be used in other places,
but I don't think it is.

So I tried to choose the lesser of two evils. :)

-Peff

Re: [PATCH 2/3] revision: clear decoration structs during release_revisions()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Oct 05, 2023 at 04:00:54PM -0700, Junio C Hamano wrote:
>
>> Wow, nested maze of callbacks make my head spin ;-) but they all
>> look reasonable.  Thanks.
>
> Yeah, I don't love those one-liner callbacks just to handle the cast.
>
> The other alternative is to write some kind of for_each_decoration()
> macro, but I think it ends up in the usual macro hell (requiring the
> caller to provide iterator variables, hanging half-open braces, and so
> on). It might be worth it if iterating could be used in other places,
> but I don't think it is.
>
> So I tried to choose the lesser of two evils. :)

Oh, I am happy with the result.