On 08/03/2021 20:12, Jeff King wrote:
On Mon, Mar 08, 2021 at 06:36:16PM +0000, Andrzej Hunt via GitGitGadget wrote:
@@ -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(path);
repo = absolute_pathdup(repo_name);
You mentioned that "path" gets reused again later. Should we use
FREE_AND_NULL() to make sure that nobody tries to look at it in the
meantime?
That sounds sensible - I wasn't too sure at first because we are
unconditionally setting path later... but I can't see an reason not to
make this safer.
But that makes me wonder if the definition of path should set it to NULL
too. Setting it to NULL after freeing it guarantees that we can't read
the old value anymore. However later code has no idea if path will be
NULL or merely initialised (at least until we overwrite it with the new
path).
I've provisionally updated my patch to also set path = NULL at point of
definition, but I don't know if that's idiomatic in this scenario.
@@ -1393,6 +1394,12 @@ 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((void *)remote_head_points_at);
We should avoid casting away constness when possible (because it is
often a sign that sometimes the variable _isn't_ pointing to owned
memory). In this case, I think freeing is the right thing; our
guess_remote_head() returns a copy of the struct (which is non-const).
Should remote_head_points_at just be declared without const?
I think so - I'll change remote_head_points_at to non-const.
+ free_refs((void *)refs);
This one is more questionable to me. It comes from
transport_get_remote_refs(), which does return a const pointer. And it
looks like that memory is owned by the transport struct. So presumably
we need to tell the transport code to clean itself up (or mark it with
UNLEAK). Or perhaps there's a bug in the transport code (e.g., should it
be freeing transport->remote_refs in transport_disconnect()? You'd want
to make sure that no other callers expect the ref list to live on past
the disconnect).
There are indeed multiple locations where we store the fetched refs,
followed by transport_disconnect(), followed by trying to use the refs
(that are nomimanlly owned by the now disconnected transport):
https://git.kernel.org/pub/scm/git/git.git/tree/builtin/remote.c?h=next#n953
https://git.kernel.org/pub/scm/git/git.git/tree/builtin/ls-remote.c?h=next#n122
However all other locations could handle free()'ing during
transport_disconnect - and the 2 I've linked to above are easy enough to
fix. So I'll give up on free_refs() from cmd_clone(), and will move it
into transport_disconnect() as suggested. I've added a new patch to the
end of the series to take care of this change.
(Which ultimately means we've now solved the same pattern of leak for
all 5 users of transport_get_remote_refs().)
