* [PATCH 0/6] leaks: miscellaneous small leak fixes @ 2021-10-21 15:57 Ævar Arnfjörð Bjarmason 2021-10-21 15:57 ` [PATCH 1/6] grep: prefer "struct grep_opt" over its "void *" Ævar Arnfjörð Bjarmason ` (6 more replies) 0 siblings, 7 replies; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason A few small miscellaneous memory leak fixes. I'm currently waiting on the other in-flight leak fixes I've got to mark more tests as passing under SANITIZE=leak, these fixes don't make any tests pass by themselves, but in combination with later fixes I've got planned will make some tests scripts pass completely. Ævar Arnfjörð Bjarmason (6): grep: prefer "struct grep_opt" over its "void *" grep: use object_array_clear() in cmd_grep() clone: fix a memory leak of the "git_dir" variable submodule--helper: fix small memory leaks reflog: free() ref given to us by dwim_log() repack: stop leaking a "struct child_process" builtin/clone.c | 4 +++- builtin/grep.c | 5 +++-- builtin/reflog.c | 1 + builtin/repack.c | 4 +++- builtin/submodule--helper.c | 2 ++ 5 files changed, 12 insertions(+), 4 deletions(-) -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/6] grep: prefer "struct grep_opt" over its "void *" 2021-10-21 15:57 [PATCH 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 ` Ævar Arnfjörð Bjarmason 2021-10-21 23:52 ` Junio C Hamano 2021-10-21 15:57 ` [PATCH 2/6] grep: use object_array_clear() in cmd_grep() Ævar Arnfjörð Bjarmason ` (5 subsequent siblings) 6 siblings, 1 reply; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason Stylistically fix up code added in bfac23d9534 (grep: Fix two memory leaks, 2010-01-30). We usually don't use the "arg" at all once we've unpacked it into the struct we want, let's not do that here when we're freeing it. Perhaps it was thought that a cast to "void *" would otherwise be needed? Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/grep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 8af5249a7bb..fd184c182a3 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -199,8 +199,8 @@ static void *run(void *arg) grep_source_clear_data(&w->source); work_done(w); } - free_grep_patterns(arg); - free(arg); + free_grep_patterns(opt); + free(opt); return (void*) (intptr_t) hit; } -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/6] grep: prefer "struct grep_opt" over its "void *" 2021-10-21 15:57 ` [PATCH 1/6] grep: prefer "struct grep_opt" over its "void *" Ævar Arnfjörð Bjarmason @ 2021-10-21 23:52 ` Junio C Hamano 0 siblings, 0 replies; 20+ messages in thread From: Junio C Hamano @ 2021-10-21 23:52 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Taylor Blau Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Subject: Re: [PATCH 1/6] grep: prefer "struct grep_opt" over its "void *" I sense an incomplete sentence. "counterpart" or "equivalent" is missing at the tail, perhaps? > Stylistically fix up code added in bfac23d9534 (grep: Fix two memory > leaks, 2010-01-30). We usually don't use the "arg" at all once we've > unpacked it into the struct we want, let's not do that here when we're As we do not unpack and instead take it as a whole and cast, "we've unpacked it into" -> "we've casted it to". > freeing it. Perhaps it was thought that a cast to "void *" would > otherwise be needed? > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/grep.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/builtin/grep.c b/builtin/grep.c > index 8af5249a7bb..fd184c182a3 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -199,8 +199,8 @@ static void *run(void *arg) > grep_source_clear_data(&w->source); > work_done(w); > } > - free_grep_patterns(arg); > - free(arg); > + free_grep_patterns(opt); > + free(opt); > > return (void*) (intptr_t) hit; > } ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/6] grep: use object_array_clear() in cmd_grep() 2021-10-21 15:57 [PATCH 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason 2021-10-21 15:57 ` [PATCH 1/6] grep: prefer "struct grep_opt" over its "void *" Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 ` Ævar Arnfjörð Bjarmason 2021-10-21 23:56 ` Junio C Hamano 2021-10-21 23:58 ` Junio C Hamano 2021-10-21 15:57 ` [PATCH 3/6] clone: fix a memory leak of the "git_dir" variable Ævar Arnfjörð Bjarmason ` (4 subsequent siblings) 6 siblings, 2 replies; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason Free the "struct object_array" before exiting. This makes grep tests (e.g. "t7815-grep-binary.sh") a bit happer under SANITIZE=leak. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/grep.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/grep.c b/builtin/grep.c index fd184c182a3..555b2ab6008 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -1196,6 +1196,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) run_pager(&opt, prefix); clear_pathspec(&pathspec); free_grep_patterns(&opt); + object_array_clear(&list); free_repos(); return !hit; } -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/6] grep: use object_array_clear() in cmd_grep() 2021-10-21 15:57 ` [PATCH 2/6] grep: use object_array_clear() in cmd_grep() Ævar Arnfjörð Bjarmason @ 2021-10-21 23:56 ` Junio C Hamano 2021-10-21 23:58 ` Junio C Hamano 1 sibling, 0 replies; 20+ messages in thread From: Junio C Hamano @ 2021-10-21 23:56 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Taylor Blau Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Free the "struct object_array" before exiting. This makes grep tests > (e.g. "t7815-grep-binary.sh") a bit happer under SANITIZE=leak. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/grep.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/builtin/grep.c b/builtin/grep.c > index fd184c182a3..555b2ab6008 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -1196,6 +1196,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) > run_pager(&opt, prefix); > clear_pathspec(&pathspec); > free_grep_patterns(&opt); > + object_array_clear(&list); > free_repos(); > return !hit; > } OK. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/6] grep: use object_array_clear() in cmd_grep() 2021-10-21 15:57 ` [PATCH 2/6] grep: use object_array_clear() in cmd_grep() Ævar Arnfjörð Bjarmason 2021-10-21 23:56 ` Junio C Hamano @ 2021-10-21 23:58 ` Junio C Hamano 1 sibling, 0 replies; 20+ messages in thread From: Junio C Hamano @ 2021-10-21 23:58 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Taylor Blau Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Free the "struct object_array" before exiting. This makes grep tests > (e.g. "t7815-grep-binary.sh") a bit happer under SANITIZE=leak. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/grep.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/builtin/grep.c b/builtin/grep.c > index fd184c182a3..555b2ab6008 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -1196,6 +1196,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) > run_pager(&opt, prefix); > clear_pathspec(&pathspec); > free_grep_patterns(&opt); > + object_array_clear(&list); > free_repos(); > return !hit; > } Not a new issue introduced by this patch, but after run_pager(), it seems that opt.output_priv aka path_list is never cleared. Should this step do that, too, as the series never revisits this file? ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/6] clone: fix a memory leak of the "git_dir" variable 2021-10-21 15:57 [PATCH 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason 2021-10-21 15:57 ` [PATCH 1/6] grep: prefer "struct grep_opt" over its "void *" Ævar Arnfjörð Bjarmason 2021-10-21 15:57 ` [PATCH 2/6] grep: use object_array_clear() in cmd_grep() Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 ` Ævar Arnfjörð Bjarmason 2021-10-22 0:07 ` Junio C Hamano 2021-10-21 15:57 ` [PATCH 4/6] submodule--helper: fix small memory leaks Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 6 siblings, 1 reply; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason At this point in cmd_clone the "git_dir" is always either an xstrdup()'d string, or something we got from mkpathdup(). Let's free() it before we clobber it. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/clone.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/clone.c b/builtin/clone.c index 559acf9e036..fb377b27657 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1040,8 +1040,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix) init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL, INIT_DB_QUIET); - if (real_git_dir) + if (real_git_dir) { + free((char *)git_dir); git_dir = real_git_dir; + } /* * additional config can be injected with -c, make sure it's included -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 3/6] clone: fix a memory leak of the "git_dir" variable 2021-10-21 15:57 ` [PATCH 3/6] clone: fix a memory leak of the "git_dir" variable Ævar Arnfjörð Bjarmason @ 2021-10-22 0:07 ` Junio C Hamano 0 siblings, 0 replies; 20+ messages in thread From: Junio C Hamano @ 2021-10-22 0:07 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Taylor Blau Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > At this point in cmd_clone the "git_dir" is always either an > xstrdup()'d string, or something we got from mkpathdup(). Let's free() > it before we clobber it. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/clone.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/builtin/clone.c b/builtin/clone.c > index 559acf9e036..fb377b27657 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -1040,8 +1040,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL, > INIT_DB_QUIET); > > - if (real_git_dir) > + if (real_git_dir) { > + free((char *)git_dir); > git_dir = real_git_dir; > + } > > /* > * additional config can be injected with -c, make sure it's included I had to wonder if the old git_dir can still be pointed at by junk_git_dir. Much earlier than this point there is this: if (real_git_dir) { if (real_dest_exists) junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL; junk_git_dir = real_git_dir; } else { if (dest_exists) junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL; junk_git_dir = git_dir; } if (safe_create_leading_directories_const(git_dir) < 0) die(_("could not create leading directories of '%s'"), git_dir); Luckily, junk_git_dir gets git_dir only when !real_git_dir, so it is safe. real_git_dir can only be set via the --separate-git-dir command line option, so we are safe here. Thanks. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/6] submodule--helper: fix small memory leaks 2021-10-21 15:57 [PATCH 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2021-10-21 15:57 ` [PATCH 3/6] clone: fix a memory leak of the "git_dir" variable Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 ` Ævar Arnfjörð Bjarmason 2021-10-21 15:57 ` [PATCH 5/6] reflog: free() ref given to us by dwim_log() Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 6 siblings, 0 replies; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason Add a missing strbuf_release() and a clear_pathspec() to the submodule--helper. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/submodule--helper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6298cbdd4e5..a157656a48a 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -3220,6 +3220,7 @@ static void die_on_index_match(const char *path, int force) } free(ps_matched); } + clear_pathspec(&ps); } static void die_on_repo_without_commits(const char *path) @@ -3231,6 +3232,7 @@ static void die_on_repo_without_commits(const char *path) if (resolve_gitlink_ref(path, "HEAD", &oid) < 0) die(_("'%s' does not have a commit checked out"), path); } + strbuf_release(&sb); } static int module_add(int argc, const char **argv, const char *prefix) -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 5/6] reflog: free() ref given to us by dwim_log() 2021-10-21 15:57 [PATCH 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2021-10-21 15:57 ` [PATCH 4/6] submodule--helper: fix small memory leaks Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 ` Ævar Arnfjörð Bjarmason 2021-10-22 0:16 ` Junio C Hamano 2021-10-21 15:57 ` [PATCH 6/6] repack: stop leaking a "struct child_process" Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason 6 siblings, 1 reply; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason When dwim_log() returns the "ref" is always ether NULL or an xstrdup()'d string. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/reflog.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/reflog.c b/builtin/reflog.c index bd4c669918d..175c83e7cc2 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -653,6 +653,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix) should_expire_reflog_ent, reflog_expiry_cleanup, &cb); + free(ref); } return status; } -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 5/6] reflog: free() ref given to us by dwim_log() 2021-10-21 15:57 ` [PATCH 5/6] reflog: free() ref given to us by dwim_log() Ævar Arnfjörð Bjarmason @ 2021-10-22 0:16 ` Junio C Hamano 0 siblings, 0 replies; 20+ messages in thread From: Junio C Hamano @ 2021-10-22 0:16 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Taylor Blau Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > When dwim_log() returns the "ref" is always ether NULL or an > xstrdup()'d string. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/reflog.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/builtin/reflog.c b/builtin/reflog.c > index bd4c669918d..175c83e7cc2 100644 > --- a/builtin/reflog.c > +++ b/builtin/reflog.c > @@ -653,6 +653,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix) > should_expire_reflog_ent, > reflog_expiry_cleanup, > &cb); > + free(ref); OK. When dwim_log() returns false, it would not have touched &ref at all, so the "continue" (not shown in the context above) is not leaking, and this loop looks good. Thanks. > } > return status; > } ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 6/6] repack: stop leaking a "struct child_process" 2021-10-21 15:57 [PATCH 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason ` (4 preceding siblings ...) 2021-10-21 15:57 ` [PATCH 5/6] reflog: free() ref given to us by dwim_log() Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 ` Ævar Arnfjörð Bjarmason 2021-10-22 0:22 ` Junio C Hamano 2021-10-22 8:55 ` [PATCH v2 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason 6 siblings, 1 reply; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-21 15:57 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/repack.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/repack.c b/builtin/repack.c index 0b2d1e5d82b..50730517c7b 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -258,9 +258,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args, for_each_packed_object(write_oid, &cmd, FOR_EACH_OBJECT_PROMISOR_ONLY); - if (cmd.in == -1) + if (cmd.in == -1) { + child_process_clear(&cmd); /* No packed objects; cmd was never started */ return; + } close(cmd.in); -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] repack: stop leaking a "struct child_process" 2021-10-21 15:57 ` [PATCH 6/6] repack: stop leaking a "struct child_process" Ævar Arnfjörð Bjarmason @ 2021-10-22 0:22 ` Junio C Hamano 0 siblings, 0 replies; 20+ messages in thread From: Junio C Hamano @ 2021-10-22 0:22 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Taylor Blau Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/repack.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/builtin/repack.c b/builtin/repack.c > index 0b2d1e5d82b..50730517c7b 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -258,9 +258,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args, > for_each_packed_object(write_oid, &cmd, > FOR_EACH_OBJECT_PROMISOR_ONLY); > > - if (cmd.in == -1) > + if (cmd.in == -1) { > + child_process_clear(&cmd); > /* No packed objects; cmd was never started */ > return; > + } > > close(cmd.in); Not wrong per-se, but let's take the one that is part of Taylor's "plug pack bitmap leaks" series that plugs the same leak. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 0/6] leaks: miscellaneous small leak fixes 2021-10-21 15:57 [PATCH 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason ` (5 preceding siblings ...) 2021-10-21 15:57 ` [PATCH 6/6] repack: stop leaking a "struct child_process" Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 ` Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 1/6] grep: prefer "struct grep_opt" over its "void *" equivalent Ævar Arnfjörð Bjarmason ` (5 more replies) 6 siblings, 6 replies; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason A re-roll of these miscellaneous small leak fixes to a address comments on v1. I ejected the patch that conflicted with Taylor's version (sorry, didn't notice it), and fixed an additional leak in grep.c pointed out by Junio. Doing that allowed us to mark a test as passing under SANITIZE=leak, with only the minor change of replacing a "git checkout" with "git reset --hard" (as "checkout" happens to leak currently, but I've also got a pending patch for that). Ævar Arnfjörð Bjarmason (6): grep: prefer "struct grep_opt" over its "void *" equivalent grep: use object_array_clear() in cmd_grep() grep: fix a "path_list" memory leak clone: fix a memory leak of the "git_dir" variable submodule--helper: fix small memory leaks reflog: free() ref given to us by dwim_log() builtin/clone.c | 4 +++- builtin/grep.c | 14 ++++++++------ builtin/reflog.c | 1 + builtin/submodule--helper.c | 2 ++ t/t7811-grep-open.sh | 3 ++- 5 files changed, 16 insertions(+), 8 deletions(-) Range-diff against v1: 1: 2bdd21e4e59 ! 1: 66c838fd800 grep: prefer "struct grep_opt" over its "void *" @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## Commit message ## - grep: prefer "struct grep_opt" over its "void *" + grep: prefer "struct grep_opt" over its "void *" equivalent Stylistically fix up code added in bfac23d9534 (grep: Fix two memory leaks, 2010-01-30). We usually don't use the "arg" at all once we've - unpacked it into the struct we want, let's not do that here when we're + casted it to the struct we want, let's not do that here when we're freeing it. Perhaps it was thought that a cast to "void *" would otherwise be needed? 2: 727fdb27a2a = 2: 033ca3f7b4f grep: use object_array_clear() in cmd_grep() -: ----------- > 3: 8e941e40711 grep: fix a "path_list" memory leak 3: 86d928ae2f9 = 4: 0d0e6359cf4 clone: fix a memory leak of the "git_dir" variable 4: 9c3c0529ad0 = 5: a529c04a29a submodule--helper: fix small memory leaks 5: 85b7b7aef37 = 6: 6ea5e611ae0 reflog: free() ref given to us by dwim_log() 6: 526d5649156 < -: ----------- repack: stop leaking a "struct child_process" -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/6] grep: prefer "struct grep_opt" over its "void *" equivalent 2021-10-22 8:55 ` [PATCH v2 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 ` Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 2/6] grep: use object_array_clear() in cmd_grep() Ævar Arnfjörð Bjarmason ` (4 subsequent siblings) 5 siblings, 0 replies; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason Stylistically fix up code added in bfac23d9534 (grep: Fix two memory leaks, 2010-01-30). We usually don't use the "arg" at all once we've casted it to the struct we want, let's not do that here when we're freeing it. Perhaps it was thought that a cast to "void *" would otherwise be needed? Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/grep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 8af5249a7bb..fd184c182a3 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -199,8 +199,8 @@ static void *run(void *arg) grep_source_clear_data(&w->source); work_done(w); } - free_grep_patterns(arg); - free(arg); + free_grep_patterns(opt); + free(opt); return (void*) (intptr_t) hit; } -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 2/6] grep: use object_array_clear() in cmd_grep() 2021-10-22 8:55 ` [PATCH v2 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 1/6] grep: prefer "struct grep_opt" over its "void *" equivalent Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 ` Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 3/6] grep: fix a "path_list" memory leak Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 5 siblings, 0 replies; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason Free the "struct object_array" before exiting. This makes grep tests (e.g. "t7815-grep-binary.sh") a bit happer under SANITIZE=leak. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/grep.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/grep.c b/builtin/grep.c index fd184c182a3..555b2ab6008 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -1196,6 +1196,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) run_pager(&opt, prefix); clear_pathspec(&pathspec); free_grep_patterns(&opt); + object_array_clear(&list); free_repos(); return !hit; } -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 3/6] grep: fix a "path_list" memory leak 2021-10-22 8:55 ` [PATCH v2 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 1/6] grep: prefer "struct grep_opt" over its "void *" equivalent Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 2/6] grep: use object_array_clear() in cmd_grep() Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 ` Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 4/6] clone: fix a memory leak of the "git_dir" variable Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 5 siblings, 0 replies; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason Free the "path_list" used in builtin/grep.c, it was declared as STRING_LIST_INIT_NODUP, let's change it to a STRING_LIST_INIT_DUP since an early user in cmd_grep() appends a string passed via parse-options.c to it, which needs to be duplicated. Let's then convert the remaining callers to use string_list_append_nodup() instead, allowing us to free the list. This makes all the tests in t7811-grep-open.sh pass, 6/10 would fail before this change. The only remaining failure would have been due to a stray "git checkout" (which still leaks memory). In this case we can use a "git reset --hard" instead, so let's do that, and move the test_when_finished() above the code that would modify the relevant file. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/grep.c | 9 +++++---- t/t7811-grep-open.sh | 3 ++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 555b2ab6008..9e34a820ad4 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -401,7 +401,7 @@ static void append_path(struct grep_opt *opt, const void *data, size_t len) if (len == 1 && *(const char *)data == '\0') return; - string_list_append(path_list, xstrndup(data, len)); + string_list_append_nodup(path_list, xstrndup(data, len)); } static void run_pager(struct grep_opt *opt, const char *prefix) @@ -839,7 +839,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) struct grep_opt opt; struct object_array list = OBJECT_ARRAY_INIT; struct pathspec pathspec; - struct string_list path_list = STRING_LIST_INIT_NODUP; + struct string_list path_list = STRING_LIST_INIT_DUP; int i; int dummy; int use_index = 1; @@ -1159,8 +1159,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix) strbuf_addf(&buf, "+/%s%s", strcmp("less", pager) ? "" : "*", opt.pattern_list->pattern); - string_list_append(&path_list, - strbuf_detach(&buf, NULL)); + string_list_append_nodup(&path_list, + strbuf_detach(&buf, NULL)); } } @@ -1195,6 +1195,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) if (hit && show_in_pager) run_pager(&opt, prefix); clear_pathspec(&pathspec); + string_list_clear(&path_list, 0); free_grep_patterns(&opt); object_array_clear(&list); free_repos(); diff --git a/t/t7811-grep-open.sh b/t/t7811-grep-open.sh index a98785da795..1dd07141a7d 100755 --- a/t/t7811-grep-open.sh +++ b/t/t7811-grep-open.sh @@ -3,6 +3,7 @@ test_description='git grep --open-files-in-pager ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-pager.sh unset PAGER GIT_PAGER @@ -114,8 +115,8 @@ test_expect_success 'modified file' ' unrelated EOF + test_when_finished "git reset --hard" && echo "enum grep_pat_token" >unrelated && - test_when_finished "git checkout HEAD unrelated" && GIT_PAGER=./less git grep -F -O "enum grep_pat_token" >out && test_cmp expect actual && test_must_be_empty out -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 4/6] clone: fix a memory leak of the "git_dir" variable 2021-10-22 8:55 ` [PATCH v2 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2021-10-22 8:55 ` [PATCH v2 3/6] grep: fix a "path_list" memory leak Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 ` Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 5/6] submodule--helper: fix small memory leaks Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 6/6] reflog: free() ref given to us by dwim_log() Ævar Arnfjörð Bjarmason 5 siblings, 0 replies; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason At this point in cmd_clone the "git_dir" is always either an xstrdup()'d string, or something we got from mkpathdup(). Let's free() it before we clobber it. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/clone.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/clone.c b/builtin/clone.c index 559acf9e036..fb377b27657 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1040,8 +1040,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix) init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL, INIT_DB_QUIET); - if (real_git_dir) + if (real_git_dir) { + free((char *)git_dir); git_dir = real_git_dir; + } /* * additional config can be injected with -c, make sure it's included -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 5/6] submodule--helper: fix small memory leaks 2021-10-22 8:55 ` [PATCH v2 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2021-10-22 8:55 ` [PATCH v2 4/6] clone: fix a memory leak of the "git_dir" variable Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 ` Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 6/6] reflog: free() ref given to us by dwim_log() Ævar Arnfjörð Bjarmason 5 siblings, 0 replies; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason Add a missing strbuf_release() and a clear_pathspec() to the submodule--helper. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/submodule--helper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6298cbdd4e5..a157656a48a 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -3220,6 +3220,7 @@ static void die_on_index_match(const char *path, int force) } free(ps_matched); } + clear_pathspec(&ps); } static void die_on_repo_without_commits(const char *path) @@ -3231,6 +3232,7 @@ static void die_on_repo_without_commits(const char *path) if (resolve_gitlink_ref(path, "HEAD", &oid) < 0) die(_("'%s' does not have a commit checked out"), path); } + strbuf_release(&sb); } static int module_add(int argc, const char **argv, const char *prefix) -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 6/6] reflog: free() ref given to us by dwim_log() 2021-10-22 8:55 ` [PATCH v2 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason ` (4 preceding siblings ...) 2021-10-22 8:55 ` [PATCH v2 5/6] submodule--helper: fix small memory leaks Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 ` Ævar Arnfjörð Bjarmason 5 siblings, 0 replies; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 8:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Taylor Blau, Ævar Arnfjörð Bjarmason When dwim_log() returns the "ref" is always ether NULL or an xstrdup()'d string. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/reflog.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/reflog.c b/builtin/reflog.c index bd4c669918d..175c83e7cc2 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -653,6 +653,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix) should_expire_reflog_ent, reflog_expiry_cleanup, &cb); + free(ref); } return status; } -- 2.33.1.1494.g88b39a443e1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-10-22 8:56 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-21 15:57 [PATCH 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason 2021-10-21 15:57 ` [PATCH 1/6] grep: prefer "struct grep_opt" over its "void *" Ævar Arnfjörð Bjarmason 2021-10-21 23:52 ` Junio C Hamano 2021-10-21 15:57 ` [PATCH 2/6] grep: use object_array_clear() in cmd_grep() Ævar Arnfjörð Bjarmason 2021-10-21 23:56 ` Junio C Hamano 2021-10-21 23:58 ` Junio C Hamano 2021-10-21 15:57 ` [PATCH 3/6] clone: fix a memory leak of the "git_dir" variable Ævar Arnfjörð Bjarmason 2021-10-22 0:07 ` Junio C Hamano 2021-10-21 15:57 ` [PATCH 4/6] submodule--helper: fix small memory leaks Ævar Arnfjörð Bjarmason 2021-10-21 15:57 ` [PATCH 5/6] reflog: free() ref given to us by dwim_log() Ævar Arnfjörð Bjarmason 2021-10-22 0:16 ` Junio C Hamano 2021-10-21 15:57 ` [PATCH 6/6] repack: stop leaking a "struct child_process" Ævar Arnfjörð Bjarmason 2021-10-22 0:22 ` Junio C Hamano 2021-10-22 8:55 ` [PATCH v2 0/6] leaks: miscellaneous small leak fixes Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 1/6] grep: prefer "struct grep_opt" over its "void *" equivalent Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 2/6] grep: use object_array_clear() in cmd_grep() Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 3/6] grep: fix a "path_list" memory leak Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 4/6] clone: fix a memory leak of the "git_dir" variable Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 5/6] submodule--helper: fix small memory leaks Ævar Arnfjörð Bjarmason 2021-10-22 8:55 ` [PATCH v2 6/6] reflog: free() ref given to us by dwim_log() Ævar Arnfjörð Bjarmason
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.