* [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
* [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
* [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
* [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
* [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 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
* 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
* 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
* 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
* 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
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).