On Wed, Nov 06, 2024 at 04:11:21PM +0100, Patrick Steinhardt wrote:
> There are two users of `UNLEAK()` left in our codebase:
>
> - In "builtin/clone.c", annotating the `repo` variable. That leak has
> already been fixed though as you can see in the context, where we do
> know to free `repo_to_free`.
>
> - In "builtin/diff.c", to unleak entries of the `blob[]` array. That
> leak has also been fixed, because the entries we assign to that
> array come from `rev.pending.objects`, and we do eventually release
> `rev`.
>
> This neatly demonstrates one of the issues with `UNLEAK()`: it is quite
> easy for the annotation to become stale. A second issue is that its
> whole intent is to paper over leaks. And while that has been a necessary
> evil in the past, because Git was leaking left and right, it isn't
> really much of an issue nowadays where our test suite has no known leaks
> anymore.
>
> Remove the last two users
OK.
> and drop the now-unused `UNLEAK()` annotation.
Perhaps it would be convenient to do this as a separate commit.
Just for reference we have this annotation since 0e5bba53af (add
UNLEAK annotation for reducing leak false positives, 2017-09-08).
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> builtin/clone.c | 1 -
> builtin/diff.c | 1 -
> git-compat-util.h | 20 --------------------
> usage.c | 15 ---------------
> 4 files changed, 37 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 59fcb317a68..e851b1475d7 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1586,7 +1586,6 @@ int cmd_clone(int argc,
> free(dir);
> free(path);
> free(repo_to_free);
> - UNLEAK(repo);
> junk_mode = JUNK_LEAVE_ALL;
>
> transport_ls_refs_options_release(&transport_ls_refs_options);
> diff --git a/builtin/diff.c b/builtin/diff.c
> index dca52d4221e..2fe92f373e9 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -628,6 +628,5 @@ int cmd_diff(int argc,
> release_revisions(&rev);
> object_array_clear(&ent);
> symdiff_release(&sdiff);
> - UNLEAK(blob);
> return result;
> }
> diff --git a/git-compat-util.h b/git-compat-util.h
> index e4a306dd563..a06d4f3809e 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1527,26 +1527,6 @@ int cmd_main(int, const char **);
> int common_exit(const char *file, int line, int code);
> #define exit(code) exit(common_exit(__FILE__, __LINE__, (code)))
>
> -/*
> - * You can mark a stack variable with UNLEAK(var) to avoid it being
> - * reported as a leak by tools like LSAN or valgrind. The argument
> - * should generally be the variable itself (not its address and not what
> - * it points to). It's safe to use this on pointers which may already
> - * have been freed, or on pointers which may still be in use.
> - *
> - * Use this _only_ for a variable that leaks by going out of scope at
> - * program exit (so only from cmd_* functions or their direct helpers).
> - * Normal functions, especially those which may be called multiple
> - * times, should actually free their memory. This is only meant as
> - * an annotation, and does nothing in non-leak-checking builds.
> - */
> -#ifdef SUPPRESS_ANNOTATED_LEAKS
> -void unleak_memory(const void *ptr, size_t len);
> -#define UNLEAK(var) unleak_memory(&(var), sizeof(var))
> -#else
> -#define UNLEAK(var) do {} while (0)
> -#endif
> -
> #define z_const
> #include <zlib.h>
>
> diff --git a/usage.c b/usage.c
> index 7a2f7805f57..29a9725784a 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -350,18 +350,3 @@ void bug_fl(const char *file, int line, const char *fmt, ...)
> trace2_cmd_error_va(fmt, ap);
> va_end(ap);
> }
> -
> -#ifdef SUPPRESS_ANNOTATED_LEAKS
> -void unleak_memory(const void *ptr, size_t len)
> -{
> - static struct suppressed_leak_root {
> - struct suppressed_leak_root *next;
> - char data[FLEX_ARRAY];
> - } *suppressed_leaks;
> - struct suppressed_leak_root *root;
> -
> - FLEX_ALLOC_MEM(root, data, ptr, len);
> - root->next = suppressed_leaks;
> - suppressed_leaks = root;
> -}
> -#endif
> --
> 2.47.0.229.g8f8d6eee53.dirty
>
