On 09/06/2021 16:38, Ævar Arnfjörð Bjarmason wrote:
[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.
I'm not familiar with git's development conventions/philosophy, but my
2c is that it's better not to enable it by default in order to minimise
divergence from the code that users are running. OTOH it's not a major
difference in behaviour so perhaps that's not a concern here.
More significantly: I get the impression it's easier to do leak checking
using LSAN, which requires recompiling git anyway - at which point you
get the flag for free - so how often will people actually perform leak
checking with Valgrind in the first place?
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);
Although this is possible, I don't think it's worth it: if UNLEAK
already exists, we might as well use it here to make the code simpler.
And UNLEAK is unlikely to go away anytime soon because... (continued below)
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.
There are trickier examples where a cmd_* function has a complex struct
on the stack, and correctly clearing all allocated memory pointed to by
its members (or in turn further children with potentially multiple
levels of indirection) is a lot of work - and that work doesn't actually
benefit the user in any way. In other words, we either need to be able
to use UNLEAK to suppress certain classes of uninteresting memory leaks
- which allows us to focus on the interesting/real leaks - or someone
has to spend a lot of time doing cleanup by hand (and/or someone has to
implement a bunch of new cleanup functions)).
In your example above, the UNLEAK can be avoided at the cost of one
additional tracking variable - but in many other cases avoiding an
UNLEAK is much more expensive. It's certainly valid to debate the merits
of the UNLEAK here, but that won't remove the need for UNLEAK's
existence in general.
(The most common example that I remember is where cmd_* has a rev_info,
and AFAICT there's no one-liner to clean that up. Using UNLEAK is
honestly the best approach there. I don't think I've actually submitted
any patches doing this, but I have a few in my local backlog.)
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
It's easy to perform leak-checking runs *if* you're OK recompiling with
LSAN, instead of using valgrind. My usual recipe for running against a
range of tests is something like:
make SANITIZE=address,leak
ASAN_OPTIONS="detect_leaks=1:abort_on_error=1" CFLAGS="-Og -g"
T="\$(wildcard t00[0-9][0-9]-*.sh)" test
Additionally: I usually specify CC=clang, although gcc+LSAN has mostly
been stable enough in my experience so you might be able to skip that.
(I've found ASAN+LSAN to be more stable than LSAN by itself, which is
why I specify address+leak, but adding ASAN in turn requires overriding
ASAN_OPTIONS to reenable leak checking.)
I don't know whether or not Valgrind is more/less effective at finding
leaks, so being able to run the test suite under valgrind would be nice
for comparison purposes though.
ATB,
Andrzej
