[In-Reply-To
<a74bbcae7363df03bf8e93167d9274d16dc807f3.1615747662.git.gitgitgadget@xxxxxxxxx>,
but intentionally breaking threading for a new topic]
On Sun, Mar 14 2021, Andrzej Hunt via GitGitGadget wrote:
> Most of these pointers can safely be freed when cmd_clone() completes,
> therefore we make sure to free them. The one exception is that we
> have to UNLEAK(repo) because it can point either to argv[0], or a
> malloc'd string returned by absolute_pathdup().
I ran into this when manually checking with valgrind and discovered that
you need SANITIZERS for -DSUPPRESS_ANNOTATED_LEAKS to squash it.
I wonder if that shouldn't be in DEVOPTS (or even a default under
DEVELOPER=1). I.e. you don't need any other special compile flags, just
a compiled git that you then run under valgrind to spot this.
> builtin/clone.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 51e844a2de0a..952fe3d8fc88 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -964,10 +964,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> {
> int is_bundle = 0, is_local;
> const char *repo_name, *repo, *work_tree, *git_dir;
> - char *path, *dir, *display_repo = NULL;
> + char *path = NULL, *dir, *display_repo = NULL;
> int dest_exists, real_dest_exists = 0;
> const struct ref *refs, *remote_head;
> - const struct ref *remote_head_points_at;
> + struct ref *remote_head_points_at = NULL;
> const struct ref *our_head_points_at;
> struct ref *mapped_refs;
> const struct ref *ref;
> @@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> repo_name = argv[0];
>
> path = get_repo_path(repo_name, &is_bundle);
> - if (path)
> + if (path) {
> + FREE_AND_NULL(path);
> repo = absolute_pathdup(repo_name);
> - else if (strchr(repo_name, ':')) {
> + } else if (strchr(repo_name, ':')) {
> repo = repo_name;
> display_repo = transport_anonymize_url(repo);
> } else
In this case it seems better to just have a :
int repo_heap = 0;
Then set "repo_heap = 1" in that absolute_pathdup(repo_name) branch,
and...
> @@ -1393,6 +1394,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> strbuf_release(&reflog_msg);
> strbuf_release(&branch_top);
> strbuf_release(&key);
> + free_refs(mapped_refs);
> + free_refs(remote_head_points_at);
> + free(dir);
> + free(path);
> + UNLEAK(repo);
Here do:
if (repo_heap)
free(repo);
But maybe there's some other out of the box way to make leak checking
Just Work without special flags in this case. I'm just noting this one
because it ended up being the only one that leaked unless I compiled
with -DSUPPRESS_ANNOTATED_LEAKS. I was fixing some leaks in the bundle
code.
Anyway, getting to the "default tests" point. I fixed a memory leak, and
wanted to it tested that the specific command doesn't leak in git's
default tests.
Do we have such a thing, if not why not?
The closest I got to getting this was:
GIT_VALGRIND_MODE=memcheck GIT_VALGRIND_OPTIONS="--leak-check=full --errors-for-leak-kinds=definite --error-exitcode=123" <SOME TEST> --valgrind
But as t/README notes it implies --verbose so we can't currently run it
under the test harness (although I have out-of-tree patches to fix that
in general).
It seems pretty straightforward to turn that specific thing into a test
with a prereq to detect if valgrind works in that mode at all, and then
do (in some dedicated test file):
# Exit/skip if we can't setup valgrind, then setup relevant
# valgrind options (maybe needing to re-source test-lib.sh, ew!)
test_expect_successs 'ls-heads should not leak' '
git bundle ls-heads a.bdl
'
But from what I've found so far no such thing exists, and it seems to
the extent that this is checked it's run manually as a one-off (see git
log --grep=valgrind), but we don't explicitly test for this
anywhere. Have I missed something?
