From 771868243cf0b6f7edcec6e672d20faa4e9b50be Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 5 Oct 2023 17:29:02 -0400 Subject: [PATCH 1/3] decorate: add clear_decoration() function 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 Signed-off-by: Junio C Hamano --- 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' ' From 8ef8da484272587c5c30810e5fcb03b4048a1221 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 5 Oct 2023 17:30:14 -0400 Subject: [PATCH 2/3] revision: clear decoration structs during release_revisions() 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 Signed-off-by: Junio C Hamano --- 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 2f4c53ea20..0ae1c76db3 100644 --- a/revision.c +++ b/revision.c @@ -3076,6 +3076,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); @@ -3093,6 +3098,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' ' From badf2fe1c31f939cac5ea229bba8de273af132d9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 5 Oct 2023 17:33:26 -0400 Subject: [PATCH 3/3] daemon: free listen_addr before returning 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 Signed-off-by: Junio C Hamano --- daemon.c | 39 ++++++++++++++++++++---------------- t/t5811-proto-disable-git.sh | 2 ++ 2 files changed, 24 insertions(+), 17 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()); + + /* 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); } - 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]); - - 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"