From patchwork Thu Oct 5 21:29:02 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13410794 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0249BE92FC6 for ; Thu, 5 Oct 2023 21:29:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231572AbjJEV3H (ORCPT ); Thu, 5 Oct 2023 17:29:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50410 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229778AbjJEV3E (ORCPT ); Thu, 5 Oct 2023 17:29:04 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 62F9F9E for ; Thu, 5 Oct 2023 14:29:03 -0700 (PDT) Received: (qmail 27243 invoked by uid 109); 5 Oct 2023 21:29:03 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 05 Oct 2023 21:29:03 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 29425 invoked by uid 111); 5 Oct 2023 21:29:04 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 05 Oct 2023 17:29:04 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 5 Oct 2023 17:29:02 -0400 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 1/3] decorate: add clear_decoration() function Message-ID: <20231005212902.GA986467@coredump.intra.peff.net> References: <20231005212802.GA982892@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231005212802.GA982892@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- 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 patchwork Thu Oct 5 21:30:14 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13410817 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1B7D6E92FC0 for ; Thu, 5 Oct 2023 21:30:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231359AbjJEVaS (ORCPT ); Thu, 5 Oct 2023 17:30:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45120 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229552AbjJEVaQ (ORCPT ); Thu, 5 Oct 2023 17:30:16 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BCBE795 for ; Thu, 5 Oct 2023 14:30:15 -0700 (PDT) Received: (qmail 27261 invoked by uid 109); 5 Oct 2023 21:30:15 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 05 Oct 2023 21:30:15 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 29452 invoked by uid 111); 5 Oct 2023 21:30:17 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 05 Oct 2023 17:30:17 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 5 Oct 2023 17:30:14 -0400 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 2/3] revision: clear decoration structs during release_revisions() Message-ID: <20231005213014.GB986467@coredump.intra.peff.net> References: <20231005212802.GA982892@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231005212802.GA982892@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- 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' ' From patchwork Thu Oct 5 21:33:26 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13410818 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 68A6DE92FC6 for ; Thu, 5 Oct 2023 21:33:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231786AbjJEVdb (ORCPT ); Thu, 5 Oct 2023 17:33:31 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38562 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229750AbjJEVda (ORCPT ); Thu, 5 Oct 2023 17:33:30 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 407AAE4 for ; Thu, 5 Oct 2023 14:33:27 -0700 (PDT) Received: (qmail 27319 invoked by uid 109); 5 Oct 2023 21:33:27 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 05 Oct 2023 21:33:27 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 29460 invoked by uid 111); 5 Oct 2023 21:33:29 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 05 Oct 2023 17:33:29 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 5 Oct 2023 17:33:26 -0400 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 3/3] daemon: free listen_addr before returning Message-ID: <20231005213326.GC986467@coredump.intra.peff.net> References: <20231005212802.GA982892@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231005212802.GA982892@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- 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"