All of lore.kernel.org
 help / color / mirror / Atom feed
* UNLEAK(), leak checking in the default tests etc.
@ 2021-06-09 14:38 Ævar Arnfjörð Bjarmason
  2021-06-09 17:44 ` Andrzej Hunt
                   ` (2 more replies)
  0 siblings, 3 replies; 125+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-09 14:38 UTC (permalink / raw)
  To: Andrzej Hunt
  Cc: git, Andrzej Hunt, Jeff King, Lénaïc Huard, Derrick Stolee


[In-Reply-To
<a74bbcae7363df03bf8e93167d9274d16dc807f3.1615747662.git.gitgitgadget@gmail.com>,
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?

^ permalink raw reply	[flat|nested] 125+ messages in thread

end of thread, other threads:[~2022-02-16 17:45 UTC | newest]

Thread overview: 125+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-09 14:38 UNLEAK(), leak checking in the default tests etc Ævar Arnfjörð Bjarmason
2021-06-09 17:44 ` Andrzej Hunt
2021-06-09 20:36   ` Felipe Contreras
2021-06-10 10:46   ` Jeff King
2021-06-10 10:56   ` Ævar Arnfjörð Bjarmason
2021-06-10 13:38     ` Jeff King
2021-06-10 15:32       ` Andrzej Hunt
2021-06-10 16:36         ` Jeff King
2021-06-11 15:44           ` Andrzej Hunt
2021-06-10 19:01 ` SZEDER Gábor
2021-07-14  0:11 ` [PATCH 0/4] add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-07-14  0:11   ` [PATCH 1/4] tests: " Ævar Arnfjörð Bjarmason
2021-07-14  3:23     ` Đoàn Trần Công Danh
2021-07-14  0:11   ` [PATCH 2/4] SANITIZE tests: fix memory leaks in t13*config*, add to whitelist Ævar Arnfjörð Bjarmason
2021-07-14  0:11   ` [PATCH 3/4] SANITIZE tests: fix memory leaks in t5701*, " Ævar Arnfjörð Bjarmason
2021-07-14  0:11   ` [PATCH 4/4] SANITIZE tests: fix leak in mailmap.c Ævar Arnfjörð Bjarmason
2021-07-14  2:19     ` Eric Sunshine
2021-07-14 17:23   ` [PATCH v2 0/4] add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-07-14 17:23     ` [PATCH v2 1/4] tests: " Ævar Arnfjörð Bjarmason
2021-07-14 18:42       ` Andrzej Hunt
2021-07-14 22:39         ` Ævar Arnfjörð Bjarmason
2021-07-15 21:14         ` Jeff King
2021-07-15 21:06       ` Jeff King
2021-07-16 14:46         ` Ævar Arnfjörð Bjarmason
2021-07-16 18:09           ` Jeff King
2021-07-16 18:45             ` Jeff King
2021-07-16 18:56             ` Ævar Arnfjörð Bjarmason
2021-07-16 19:22               ` Jeff King
2021-07-14 17:23     ` [PATCH v2 2/4] SANITIZE tests: fix memory leaks in t13*config*, add to whitelist Ævar Arnfjörð Bjarmason
2021-07-14 18:57       ` Andrzej Hunt
2021-07-14 22:56         ` Ævar Arnfjörð Bjarmason
2021-07-15 21:42         ` Jeff King
2021-07-16  5:18           ` Andrzej Hunt
2021-07-16 21:20             ` Jeff King
2021-07-16  7:46           ` Ævar Arnfjörð Bjarmason
2021-07-16 21:16             ` Jeff King
2021-08-31 12:47               ` Ævar Arnfjörð Bjarmason
2021-09-01  7:53                 ` Jeff King
2021-09-01 11:45                   ` Ævar Arnfjörð Bjarmason
2021-07-14 17:23     ` [PATCH v2 3/4] SANITIZE tests: fix memory leaks in t5701*, " Ævar Arnfjörð Bjarmason
2021-07-15 17:37       ` Andrzej Hunt
2021-07-15 21:43       ` Jeff King
2021-08-31 13:46       ` [PATCH] protocol-caps.c: fix memory leak in send_info() Ævar Arnfjörð Bjarmason
2021-08-31 15:32         ` Bruno Albuquerque
2021-08-31 18:15           ` Junio C Hamano
     [not found]         ` <CAPeR6H69a_HMwWnpHzssaCm_ow=ic7AnzMdZVQJQ2ECRDaWzaA@mail.gmail.com>
2021-08-31 20:08           ` Ævar Arnfjörð Bjarmason
2021-07-14 17:23     ` [PATCH v2 4/4] SANITIZE tests: fix leak in mailmap.c Ævar Arnfjörð Bjarmason
2021-08-31 13:42       ` [PATCH] mailmap.c: fix a memory leak in free_mailap_{info,entry}() Ævar Arnfjörð Bjarmason
2021-08-31 16:22         ` Eric Sunshine
2021-08-31 19:38         ` Jeff King
2021-08-31 19:46           ` Junio C Hamano
2021-07-15 17:37     ` [PATCH v2 0/4] add a test mode for SANITIZE=leak, run it in CI Andrzej Hunt
2021-08-31 13:35     ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
2021-09-01  9:56       ` Jeff King
2021-09-01 10:42         ` Jeff King
2021-09-02 12:25         ` Ævar Arnfjörð Bjarmason
2021-09-03 11:13           ` Jeff King
2021-09-07 15:33       ` [PATCH v4 0/3] " Ævar Arnfjörð Bjarmason
2021-09-07 15:33         ` [PATCH v4 1/3] Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS Ævar Arnfjörð Bjarmason
2021-09-07 15:33         ` [PATCH v4 2/3] CI: refactor "if" to "case" statement Ævar Arnfjörð Bjarmason
2021-09-07 15:33         ` [PATCH v4 3/3] tests: add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-09-07 16:29           ` Eric Sunshine
2021-09-07 16:51           ` Jeff King
2021-09-07 16:44         ` [PATCH v4 0/3] " Jeff King
2021-09-07 18:22           ` Junio C Hamano
2021-09-07 21:30         ` [PATCH v5 " Ævar Arnfjörð Bjarmason
2021-09-07 21:30           ` [PATCH v5 1/3] Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS Ævar Arnfjörð Bjarmason
2021-09-07 21:30           ` [PATCH v5 2/3] CI: refactor "if" to "case" statement Ævar Arnfjörð Bjarmason
2021-09-07 21:30           ` [PATCH v5 3/3] tests: add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-09-08  4:46             ` Eric Sunshine
2021-09-16  3:56             ` [PATCH] fixup! " Carlo Marcelo Arenas Belón
2021-09-16  6:14               ` Ævar Arnfjörð Bjarmason
2021-09-08 11:02           ` [PATCH v5 0/3] " Junio C Hamano
2021-09-08 12:03             ` Ævar Arnfjörð Bjarmason
2021-09-09 23:10               ` Emily Shaffer
2021-09-16 10:48           ` [PATCH v6 0/2] " Ævar Arnfjörð Bjarmason
2021-09-16 10:48             ` [PATCH v6 1/2] Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS Ævar Arnfjörð Bjarmason
2021-09-16 10:48             ` [PATCH v6 2/2] tests: add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-09-19  8:03             ` [PATCH v7 0/2] " Ævar Arnfjörð Bjarmason
2021-09-19  8:03               ` [PATCH v7 1/2] Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS Ævar Arnfjörð Bjarmason
2021-09-19  8:03               ` [PATCH v7 2/2] tests: add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-09-22 11:17                 ` [PATCH] fixup! " Carlo Marcelo Arenas Belón
2021-09-23  1:50                   ` Ævar Arnfjörð Bjarmason
2021-09-23  9:20               ` [PATCH v8 0/2] " Ævar Arnfjörð Bjarmason
2021-09-23  9:20                 ` [PATCH v8 1/2] Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS Ævar Arnfjörð Bjarmason
2021-09-23  9:20                 ` [PATCH v8 2/2] tests: add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-11-03 22:44                   ` Re* " Junio C Hamano
2021-11-03 23:57                     ` Junio C Hamano
2021-11-04 10:06                     ` Ævar Arnfjörð Bjarmason
2021-11-16 18:31                       ` [PATCH] t0006: date_mode can leak .strftime_fmt member Ævar Arnfjörð Bjarmason
2021-11-16 19:04                         ` Junio C Hamano
2021-11-16 19:31                         ` Jeff King
2022-02-02 21:03                           ` [PATCH 0/5] date.[ch] API: split from cache.h, add API docs, stop leaking memory Ævar Arnfjörð Bjarmason
2022-02-02 21:03                             ` [PATCH 1/5] cache.h: remove always unused show_date_human() declaration Ævar Arnfjörð Bjarmason
2022-02-02 21:03                             ` [PATCH 2/5] date API: create a date.h, split from cache.h Ævar Arnfjörð Bjarmason
2022-02-02 21:19                               ` Ævar Arnfjörð Bjarmason
2022-02-15  3:04                               ` Junio C Hamano
2022-02-02 21:03                             ` [PATCH 3/5] date API: provide and use a DATE_MODE_INIT Ævar Arnfjörð Bjarmason
2022-02-02 21:03                             ` [PATCH 4/5] date API: add basic API docs Ævar Arnfjörð Bjarmason
2022-02-15  2:14                               ` Junio C Hamano
2022-02-02 21:03                             ` [PATCH 5/5] date API: add and use a date_mode_release() Ævar Arnfjörð Bjarmason
2022-02-15  0:28                               ` Junio C Hamano
2022-02-04 23:53                             ` [PATCH v2 0/5] date.[ch] API: split from cache.h, add API docs, stop leaking memory Ævar Arnfjörð Bjarmason
2022-02-04 23:53                               ` [PATCH v2 1/5] cache.h: remove always unused show_date_human() declaration Ævar Arnfjörð Bjarmason
2022-02-04 23:53                               ` [PATCH v2 2/5] date API: create a date.h, split from cache.h Ævar Arnfjörð Bjarmason
2022-02-04 23:53                               ` [PATCH v2 3/5] date API: provide and use a DATE_MODE_INIT Ævar Arnfjörð Bjarmason
2022-02-04 23:53                               ` [PATCH v2 4/5] date API: add basic API docs Ævar Arnfjörð Bjarmason
2022-02-04 23:53                               ` [PATCH v2 5/5] date API: add and use a date_mode_release() Ævar Arnfjörð Bjarmason
2022-02-14 17:25                               ` [PATCH v2 0/5] date.[ch] API: split from cache.h, add API docs, stop leaking memory Ævar Arnfjörð Bjarmason
2022-02-14 19:52                                 ` Junio C Hamano
2022-02-16  8:14                               ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2022-02-16  8:14                                 ` [PATCH v3 1/5] cache.h: remove always unused show_date_human() declaration Ævar Arnfjörð Bjarmason
2022-02-16  8:14                                 ` [PATCH v3 2/5] date API: create a date.h, split from cache.h Ævar Arnfjörð Bjarmason
2022-02-16  8:14                                 ` [PATCH v3 3/5] date API: provide and use a DATE_MODE_INIT Ævar Arnfjörð Bjarmason
2022-02-16  8:14                                 ` [PATCH v3 4/5] date API: add basic API docs Ævar Arnfjörð Bjarmason
2022-02-16  8:14                                 ` [PATCH v3 5/5] date API: add and use a date_mode_release() Ævar Arnfjörð Bjarmason
2022-02-16 17:45                                 ` [PATCH v3 0/5] date.[ch] API: split from cache.h, add API docs, stop leaking memory Junio C Hamano
     [not found]     ` <cover-v3-0.8-00000000000-20210831T132607Z-avarab@gmail.com>
2021-08-31 13:35       ` [PATCH v3 1/8] Makefile: add SANITIZE=leak flag to GIT-BUILD-OPTIONS Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 2/8] CI: refactor "if" to "case" statement Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 3/8] tests: add a test mode for SANITIZE=leak, run it in CI Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 4/8] tests: annotate t000*.sh with TEST_PASSES_SANITIZE_LEAK=true Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 5/8] tests: annotate t001*.sh " Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 6/8] tests: annotate t002*.sh " Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 7/8] tests: annotate select t0*.sh " Ævar Arnfjörð Bjarmason
2021-08-31 13:35       ` [PATCH v3 8/8] tests: annotate select t*.sh " Æ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.