[PATCH 0/28] leak fixes for http fetch/push

[PATCH 0/28] leak fixes for http fetch/push

[Jeff King]

Patrick asked me to take a look at the remaining leaks in the http
push/fetch code, especially the dumb variants. So here are enough
patches to all of these scripts running leak-free:

  t5500-fetch-pack.sh
  t5539-fetch-http-shallow.sh
  t5540-http-push-webdav.sh
  t5541-http-push-smart.sh
  t5542-push-http-shallow.sh
  t5550-http-fetch-dumb.sh
  t5551-http-fetch-smart.sh
  t5582-fetch-negative-refspec.sh
  t5619-clone-local-ambiguous-transport.sh

I did have to cheat a little and steal some of the patches from his
"leak fixes part 8" work-in-progress (which are at the front of this
series). But otherwise this should be independent of the other
leak-fixing work and can be applied directly on master.

I tried to put patches touching similar areas together, but there's
probably still a bit of a jumble, just because I got there by picking at
the pile of leak logs for each script. :)

Most of them are pretty obvious "add a free call" one-liners. If you
really want to put on your thinking cap, try patches 7, 14, and 21 (I'm
going to pretend I spaced them out evenly for your reading pleasure).

  [01/28]: http-fetch: clear leaking git-index-pack(1) arguments
  [02/28]: shallow: fix leak when unregistering last shallow root
  [03/28]: fetch-pack: fix leaking sought refs
  [04/28]: connect: clear child process before freeing in diagnostic mode
  [05/28]: fetch-pack: free object filter before exiting
  [06/28]: fetch-pack, send-pack: clean up shallow oid array
  [07/28]: commit: avoid leaking already-saved buffer
  [08/28]: send-pack: free cas options before exit
  [09/28]: transport-helper: fix strbuf leak in push_refs_with_push()
  [10/28]: fetch: free "raw" string when shrinking refspec
  [11/28]: fetch-pack: clear pack lockfiles list
  [12/28]: transport-helper: fix leak of dummy refs_list
  [13/28]: http: fix leak when redacting cookies from curl trace
  [14/28]: http: fix leak of http_object_request struct
  [15/28]: http: call git_inflate_end() when releasing http_object_request
  [16/28]: http: stop leaking buffer in http_get_info_packs()
  [17/28]: remote-curl: free HEAD ref with free_one_ref()
  [18/28]: http-walker: free fake packed_git list
  [19/28]: http-push: clear refspecs before exiting
  [20/28]: http-push: free repo->url string
  [21/28]: http-push: free curl header lists
  [22/28]: http-push: free transfer_request dest field
  [23/28]: http-push: free transfer_request strbuf
  [24/28]: http-push: free remote_ls_ctx.dentry_name
  [25/28]: http-push: free xml_ctx.cdata after use
  [26/28]: http-push: clean up objects list
  [27/28]: http-push: clean up loose request when falling back to packed
  [28/28]: http-push: clean up local_refs at exit

 builtin/fetch-pack.c                       | 14 +++++++-
 builtin/fetch.c                            |  1 +
 builtin/push.c                             |  1 +
 builtin/send-pack.c                        |  2 ++
 commit.c                                   |  3 +-
 connect.c                                  |  1 +
 http-fetch.c                               | 16 ++++++---
 http-push.c                                | 40 +++++++++++++++-------
 http-walker.c                              | 18 +++++++---
 http.c                                     | 16 ++++++---
 http.h                                     |  4 +--
 refspec.c                                  |  2 +-
 refspec.h                                  |  2 +-
 remote-curl.c                              |  2 +-
 remote.c                                   |  2 +-
 remote.h                                   |  1 +
 shallow.c                                  |  5 ++-
 t/t5500-fetch-pack.sh                      |  1 +
 t/t5539-fetch-http-shallow.sh              |  1 +
 t/t5540-http-push-webdav.sh                |  1 +
 t/t5541-http-push-smart.sh                 |  1 +
 t/t5542-push-http-shallow.sh               |  1 +
 t/t5550-http-fetch-dumb.sh                 |  1 +
 t/t5551-http-fetch-smart.sh                |  1 +
 t/t5582-fetch-negative-refspec.sh          |  1 +
 t/t5619-clone-local-ambiguous-transport.sh |  1 +
 t/t5700-protocol-v1.sh                     |  1 +
 transport-helper.c                         | 11 ++++--
 transport.c                                | 11 +++++-
 29 files changed, 123 insertions(+), 39 deletions(-)

-Peff

[PATCH 01/28] http-fetch: clear leaking git-index-pack(1) arguments

[Jeff King]

From: Patrick Steinhardt <ps@pks.im>

We never clear the arguments that we pass to git-index-pack(1). Create a
common exit path and release them there to plug this leak.

This is leak is exposed by t5702, but plugging the leak does not make
the whole test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jeff King <peff@peff.net>
---
Obviously I didn't write this or the next two patches, but FWIW, I
reviewed them and they all look good to me.

 http-fetch.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/http-fetch.c b/http-fetch.c
index d460bb1837..02ab80533f 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -106,6 +106,7 @@ int cmd_main(int argc, const char **argv)
 	int nongit;
 	struct object_id packfile_hash;
 	struct strvec index_pack_args = STRVEC_INIT;
+	int ret;
 
 	setup_git_directory_gently(&nongit);
 
@@ -157,8 +158,8 @@ int cmd_main(int argc, const char **argv)
 
 		fetch_single_packfile(&packfile_hash, argv[arg],
 				      index_pack_args.v);
-
-		return 0;
+		ret = 0;
+		goto out;
 	}
 
 	if (index_pack_args.nr)
@@ -170,7 +171,12 @@ int cmd_main(int argc, const char **argv)
 		commit_id = (char **) &argv[arg++];
 		commits = 1;
 	}
-	return fetch_using_walker(argv[arg], get_verbosely, get_recover,
-				  commits, commit_id, write_ref,
-				  commits_on_stdin);
+
+	ret = fetch_using_walker(argv[arg], get_verbosely, get_recover,
+				 commits, commit_id, write_ref,
+				 commits_on_stdin);
+
+out:
+	strvec_clear(&index_pack_args);
+	return ret;
 }
-- 
2.46.2.1011.gf1f9323e02

[PATCH 02/28] shallow: fix leak when unregistering last shallow root

[Jeff King]

From: Patrick Steinhardt <ps@pks.im>

When unregistering a shallow root we shrink the array of grafts by one
and move remaining grafts one to the left. This can of course only
happen when there are any grafts left, because otherwise there is
nothing to move. As such, this code is guarded by a condition that only
performs the move in case there are grafts after the position of the
graft to be unregistered.

By mistake we also put the call to free the unregistered graft into that
condition. But that doesn't make any sense, as we want to always free
the graft when it exists. Fix the resulting memory leak by doing so.

This leak is exposed by t5500, but plugging it does not make the whole
test suite pass.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jeff King <peff@peff.net>
---
 shallow.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/shallow.c b/shallow.c
index dcebc263d7..4bb1518dbc 100644
--- a/shallow.c
+++ b/shallow.c
@@ -51,12 +51,11 @@ int unregister_shallow(const struct object_id *oid)
 	int pos = commit_graft_pos(the_repository, oid);
 	if (pos < 0)
 		return -1;
-	if (pos + 1 < the_repository->parsed_objects->grafts_nr) {
-		free(the_repository->parsed_objects->grafts[pos]);
+	free(the_repository->parsed_objects->grafts[pos]);
+	if (pos + 1 < the_repository->parsed_objects->grafts_nr)
 		MOVE_ARRAY(the_repository->parsed_objects->grafts + pos,
 			   the_repository->parsed_objects->grafts + pos + 1,
 			   the_repository->parsed_objects->grafts_nr - pos - 1);
-	}
 	the_repository->parsed_objects->grafts_nr--;
 	return 0;
 }
-- 
2.46.2.1011.gf1f9323e02

[PATCH 03/28] fetch-pack: fix leaking sought refs

[Jeff King]

From: Patrick Steinhardt <ps@pks.im>

When calling `fetch_pack()` the caller is expected to pass in a set of
sought-after refs that they want to fetch. This array gets massaged to
not contain duplicate entries, which is done by replacing duplicate refs
with `NULL` pointers. This modifies the caller-provided array, and in
case we do unset any pointers the caller now loses track of that ref and
cannot free it anymore.

Now the obvious fix would be to not only unset these pointers, but to
also free their contents. But this doesn't work because callers continue
to use those refs. Another potential solution would be to copy the array
in `fetch_pack()` so that we dont modify the caller-provided one. But
that doesn't work either because the NULL-ness of those entries is used
by callers to skip over ref entries that we didn't even try to fetch in
`report_unmatched_refs()`.

Instead, we make it the responsibility of our callers to duplicate these
arrays as needed. It ain't pretty, but it works to plug the memory leak.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c   | 11 ++++++++++-
 t/t5700-protocol-v1.sh |  1 +
 transport.c            | 11 ++++++++++-
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 49222a36fa..c5319232a5 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -53,6 +53,7 @@ int cmd_fetch_pack(int argc,
 	struct ref *fetched_refs = NULL, *remote_refs = NULL;
 	const char *dest = NULL;
 	struct ref **sought = NULL;
+	struct ref **sought_to_free = NULL;
 	int nr_sought = 0, alloc_sought = 0;
 	int fd[2];
 	struct string_list pack_lockfiles = STRING_LIST_INIT_DUP;
@@ -243,6 +244,13 @@ int cmd_fetch_pack(int argc,
 		BUG("unknown protocol version");
 	}
 
+	/*
+	 * Create a shallow copy of `sought` so that we can free all of its entries.
+	 * This is because `fetch_pack()` will modify the array to evict some
+	 * entries, but won't free those.
+	 */
+	DUP_ARRAY(sought_to_free, sought, nr_sought);
+
 	fetched_refs = fetch_pack(&args, fd, remote_refs, sought, nr_sought,
 			 &shallow, pack_lockfiles_ptr, version);
 
@@ -280,7 +288,8 @@ int cmd_fetch_pack(int argc,
 		       oid_to_hex(&ref->old_oid), ref->name);
 
 	for (size_t i = 0; i < nr_sought; i++)
-		free_one_ref(sought[i]);
+		free_one_ref(sought_to_free[i]);
+	free(sought_to_free);
 	free(sought);
 	free_refs(fetched_refs);
 	free_refs(remote_refs);
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index a73b4d4ff6..985e04d06e 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -11,6 +11,7 @@ export GIT_TEST_PROTOCOL_VERSION
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Test protocol v1 with 'git://' transport
diff --git a/transport.c b/transport.c
index 3c4714581f..1098bbd60e 100644
--- a/transport.c
+++ b/transport.c
@@ -414,7 +414,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	struct git_transport_data *data = transport->data;
 	struct ref *refs = NULL;
 	struct fetch_pack_args args;
-	struct ref *refs_tmp = NULL;
+	struct ref *refs_tmp = NULL, **to_fetch_dup = NULL;
 
 	memset(&args, 0, sizeof(args));
 	args.uploadpack = data->options.uploadpack;
@@ -477,6 +477,14 @@ static int fetch_refs_via_pack(struct transport *transport,
 		goto cleanup;
 	}
 
+	/*
+	 * Create a shallow copy of `sought` so that we can free all of its entries.
+	 * This is because `fetch_pack()` will modify the array to evict some
+	 * entries, but won't free those.
+	 */
+	DUP_ARRAY(to_fetch_dup, to_fetch, nr_heads);
+	to_fetch = to_fetch_dup;
+
 	refs = fetch_pack(&args, data->fd,
 			  refs_tmp ? refs_tmp : transport->remote_refs,
 			  to_fetch, nr_heads, &data->shallow,
@@ -500,6 +508,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 		ret = -1;
 	data->conn = NULL;
 
+	free(to_fetch_dup);
 	free_refs(refs_tmp);
 	free_refs(refs);
 	list_objects_filter_release(&args.filter_options);
-- 
2.46.2.1011.gf1f9323e02

Re: [PATCH 03/28] fetch-pack: fix leaking sought refs

[René Scharfe]

Am 24.09.24 um 23:51 schrieb Jeff King:
> From: Patrick Steinhardt <ps@pks.im>
>
> When calling `fetch_pack()` the caller is expected to pass in a set of
> sought-after refs that they want to fetch. This array gets massaged to
> not contain duplicate entries, which is done by replacing duplicate refs
> with `NULL` pointers. This modifies the caller-provided array, and in
> case we do unset any pointers the caller now loses track of that ref and
> cannot free it anymore.
>
> Now the obvious fix would be to not only unset these pointers, but to
> also free their contents. But this doesn't work because callers continue
> to use those refs. Another potential solution would be to copy the array
> in `fetch_pack()` so that we dont modify the caller-provided one. But
> that doesn't work either because the NULL-ness of those entries is used
> by callers to skip over ref entries that we didn't even try to fetch in
> `report_unmatched_refs()`.
>
> Instead, we make it the responsibility of our callers to duplicate these
> arrays as needed. It ain't pretty, but it works to plug the memory leak.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/fetch-pack.c   | 11 ++++++++++-
>  t/t5700-protocol-v1.sh |  1 +
>  transport.c            | 11 ++++++++++-
>  3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 49222a36fa..c5319232a5 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -53,6 +53,7 @@ int cmd_fetch_pack(int argc,
>  	struct ref *fetched_refs = NULL, *remote_refs = NULL;
>  	const char *dest = NULL;
>  	struct ref **sought = NULL;
> +	struct ref **sought_to_free = NULL;
>  	int nr_sought = 0, alloc_sought = 0;
>  	int fd[2];
>  	struct string_list pack_lockfiles = STRING_LIST_INIT_DUP;
> @@ -243,6 +244,13 @@ int cmd_fetch_pack(int argc,
>  		BUG("unknown protocol version");
>  	}
>
> +	/*
> +	 * Create a shallow copy of `sought` so that we can free all of its entries.
> +	 * This is because `fetch_pack()` will modify the array to evict some
> +	 * entries, but won't free those.
> +	 */
> +	DUP_ARRAY(sought_to_free, sought, nr_sought);
> +
>  	fetched_refs = fetch_pack(&args, fd, remote_refs, sought, nr_sought,
>  			 &shallow, pack_lockfiles_ptr, version);
>
> @@ -280,7 +288,8 @@ int cmd_fetch_pack(int argc,
>  		       oid_to_hex(&ref->old_oid), ref->name);
>
>  	for (size_t i = 0; i < nr_sought; i++)
> -		free_one_ref(sought[i]);
> +		free_one_ref(sought_to_free[i]);
> +	free(sought_to_free);

OK.

>  	free(sought);
>  	free_refs(fetched_refs);
>  	free_refs(remote_refs);
> diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
> index a73b4d4ff6..985e04d06e 100755
> --- a/t/t5700-protocol-v1.sh
> +++ b/t/t5700-protocol-v1.sh
> @@ -11,6 +11,7 @@ export GIT_TEST_PROTOCOL_VERSION
>  GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  # Test protocol v1 with 'git://' transport
> diff --git a/transport.c b/transport.c
> index 3c4714581f..1098bbd60e 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -414,7 +414,7 @@ static int fetch_refs_via_pack(struct transport *transport,
>  	struct git_transport_data *data = transport->data;
>  	struct ref *refs = NULL;
>  	struct fetch_pack_args args;
> -	struct ref *refs_tmp = NULL;
> +	struct ref *refs_tmp = NULL, **to_fetch_dup = NULL;
>
>  	memset(&args, 0, sizeof(args));
>  	args.uploadpack = data->options.uploadpack;
> @@ -477,6 +477,14 @@ static int fetch_refs_via_pack(struct transport *transport,
>  		goto cleanup;
>  	}
>
> +	/*
> +	 * Create a shallow copy of `sought` so that we can free all of its entries.
> +	 * This is because `fetch_pack()` will modify the array to evict some
> +	 * entries, but won't free those.
> +	 */
> +	DUP_ARRAY(to_fetch_dup, to_fetch, nr_heads);
> +	to_fetch = to_fetch_dup;
> +
>  	refs = fetch_pack(&args, data->fd,
>  			  refs_tmp ? refs_tmp : transport->remote_refs,
>  			  to_fetch, nr_heads, &data->shallow,
> @@ -500,6 +508,7 @@ static int fetch_refs_via_pack(struct transport *transport,
>  		ret = -1;
>  	data->conn = NULL;
>
> +	free(to_fetch_dup);

This makes a shallow copy and passes it to fetch_pack() and later to
report_unmatched_refs().  It shields callers of fetch_refs_via_pack()
from the effect of fetch_pack()'s NULLification.  This is what the
commit message says would not work.  What am I missing?

>  	free_refs(refs_tmp);
>  	free_refs(refs);
>  	list_objects_filter_release(&args.filter_options);

Re: [PATCH 03/28] fetch-pack: fix leaking sought refs

[Patrick Steinhardt]

On Wed, Sep 25, 2024 at 07:17:24PM +0200, René Scharfe wrote:
> Am 24.09.24 um 23:51 schrieb Jeff King:
> > From: Patrick Steinhardt <ps@pks.im>
> > diff --git a/transport.c b/transport.c
> > index 3c4714581f..1098bbd60e 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -414,7 +414,7 @@ static int fetch_refs_via_pack(struct transport *transport,
> >  	struct git_transport_data *data = transport->data;
> >  	struct ref *refs = NULL;
> >  	struct fetch_pack_args args;
> > -	struct ref *refs_tmp = NULL;
> > +	struct ref *refs_tmp = NULL, **to_fetch_dup = NULL;
> >
> >  	memset(&args, 0, sizeof(args));
> >  	args.uploadpack = data->options.uploadpack;
> > @@ -477,6 +477,14 @@ static int fetch_refs_via_pack(struct transport *transport,
> >  		goto cleanup;
> >  	}
> >
> > +	/*
> > +	 * Create a shallow copy of `sought` so that we can free all of its entries.
> > +	 * This is because `fetch_pack()` will modify the array to evict some
> > +	 * entries, but won't free those.
> > +	 */
> > +	DUP_ARRAY(to_fetch_dup, to_fetch, nr_heads);
> > +	to_fetch = to_fetch_dup;
> > +
> >  	refs = fetch_pack(&args, data->fd,
> >  			  refs_tmp ? refs_tmp : transport->remote_refs,
> >  			  to_fetch, nr_heads, &data->shallow,
> > @@ -500,6 +508,7 @@ static int fetch_refs_via_pack(struct transport *transport,
> >  		ret = -1;
> >  	data->conn = NULL;
> >
> > +	free(to_fetch_dup);
> 
> This makes a shallow copy and passes it to fetch_pack() and later to
> report_unmatched_refs().  It shields callers of fetch_refs_via_pack()
> from the effect of fetch_pack()'s NULLification.  This is what the
> commit message says would not work.  What am I missing?

`fetch_refs_via_pack()` is called via `transport_fetch_refs()`, which
constructs the array of refs to fetch ad-hoc and then discards it
immediately. So it doesn't ever inspect the modified array in the first
place, and consequently we can guard against the NULLification here.

This code is quite intricate :/

Patrick

[PATCH 04/28] connect: clear child process before freeing in diagnostic mode

[Jeff King]

The git_connect() function has a special CONNECT_DIAG_URL mode, where we
stop short of actually connecting to the other side and just print some
parsing details. For URLs that require a child process (like ssh), we
free() the child_process struct but forget to clear it, leaking the
strings we stuffed into its "env" list.

This leak is triggered many times in t5500, which uses "fetch-pack
--diag-url", but we're not yet ready to mark it as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 connect.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/connect.c b/connect.c
index 6829ab3974..58f53d8dcb 100644
--- a/connect.c
+++ b/connect.c
@@ -1485,6 +1485,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 
 				free(hostandport);
 				free(path);
+				child_process_clear(conn);
 				free(conn);
 				strbuf_release(&cmd);
 				return NULL;
-- 
2.46.2.1011.gf1f9323e02

Re: [PATCH 04/28] connect: clear child process before freeing in diagnostic mode

[Patrick Steinhardt]

On Tue, Sep 24, 2024 at 05:51:24PM -0400, Jeff King wrote:
> The git_connect() function has a special CONNECT_DIAG_URL mode, where we
> stop short of actually connecting to the other side and just print some
> parsing details. For URLs that require a child process (like ssh), we
> free() the child_process struct but forget to clear it, leaking the
> strings we stuffed into its "env" list.
> 
> This leak is triggered many times in t5500, which uses "fetch-pack
> --diag-url", but we're not yet ready to mark it as leak-free.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  connect.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/connect.c b/connect.c
> index 6829ab3974..58f53d8dcb 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -1485,6 +1485,7 @@ struct child_process *git_connect(int fd[2], const char *url,
>  
>  				free(hostandport);
>  				free(path);
> +				child_process_clear(conn);
>  				free(conn);
>  				strbuf_release(&cmd);
>  				return NULL;

There's only a single exit path in this function that ends up discarding
the `struct child_process`, so this looks good to me.

Patrick

[PATCH 05/28] fetch-pack: free object filter before exiting

[Jeff King]

Our fetch_pack_args holds a filter_options struct that may be populated
with allocated strings by the by the "--filter" command-line option. We
must free it before exiting to avoid a leak when the program exits.

The usual fetch code paths that use transport.c don't have the same
leak, because we do the cleanup in disconnect_git().

Fixing this leak lets us mark t5500 as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c  | 1 +
 t/t5500-fetch-pack.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c5319232a5..cfc6951d23 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -293,5 +293,6 @@ int cmd_fetch_pack(int argc,
 	free(sought);
 	free_refs(fetched_refs);
 	free_refs(remote_refs);
+	list_objects_filter_release(&args.filter_options);
 	return ret;
 }
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 585ea0ee16..605f17240c 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -8,6 +8,7 @@ test_description='Testing multi_ack pack fetching'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Test fetch-pack/upload-pack pair.
-- 
2.46.2.1011.gf1f9323e02

Re: [PATCH 05/28] fetch-pack: free object filter before exiting

[Patrick Steinhardt]

On Tue, Sep 24, 2024 at 05:52:07PM -0400, Jeff King wrote:
> Our fetch_pack_args holds a filter_options struct that may be populated
> with allocated strings by the by the "--filter" command-line option. We

s/by the by the/by the/

> must free it before exiting to avoid a leak when the program exits.
> 
> The usual fetch code paths that use transport.c don't have the same
> leak, because we do the cleanup in disconnect_git().
> 
> Fixing this leak lets us mark t5500 as leak-free.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/fetch-pack.c  | 1 +
>  t/t5500-fetch-pack.sh | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index c5319232a5..cfc6951d23 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -293,5 +293,6 @@ int cmd_fetch_pack(int argc,
>  	free(sought);
>  	free_refs(fetched_refs);
>  	free_refs(remote_refs);
> +	list_objects_filter_release(&args.filter_options);
>  	return ret;
>  }

This fix seems familiar to me, I think I also had it at one point in
time. Cannot find it in any of my series though, so... *shrug* Anyway,
this looks good to me.

Patrick

[PATCH 06/28] fetch-pack, send-pack: clean up shallow oid array

[Jeff King]

When we call get_remote_heads() for protocol v0, that may populate the
"shallow" oid_array, which must be cleaned up to avoid a leak at the
program exit. The same problem exists for both fetch-pack and send-pack,
but not for the usual transport.c code paths, since we already do this
cleanup in disconnect_git().

Fixing this lets us mark t5542 as leak-free for the send-pack side, but
fetch-pack will need some more fixes before we can do the same for
t5539.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c         | 1 +
 builtin/send-pack.c          | 1 +
 t/t5542-push-http-shallow.sh | 1 +
 3 files changed, 3 insertions(+)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index cfc6951d23..ef4143eef3 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc,
 	free_refs(fetched_refs);
 	free_refs(remote_refs);
 	list_objects_filter_release(&args.filter_options);
+	oid_array_clear(&shallow);
 	return ret;
 }
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 81fc96d423..c49fe6c53c 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -343,5 +343,6 @@ int cmd_send_pack(int argc,
 	free_refs(remote_refs);
 	free_refs(local_refs);
 	refspec_clear(&rs);
+	oid_array_clear(&shallow);
 	return ret;
 }
diff --git a/t/t5542-push-http-shallow.sh b/t/t5542-push-http-shallow.sh
index c2cc83182f..07624a1d7f 100755
--- a/t/t5542-push-http-shallow.sh
+++ b/t/t5542-push-http-shallow.sh
@@ -5,6 +5,7 @@ test_description='push from/to a shallow clone over http'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
-- 
2.46.2.1011.gf1f9323e02

Re: [PATCH 06/28] fetch-pack, send-pack: clean up shallow oid array

[Patrick Steinhardt]

On Tue, Sep 24, 2024 at 05:52:25PM -0400, Jeff King wrote:
> When we call get_remote_heads() for protocol v0, that may populate the
> "shallow" oid_array, which must be cleaned up to avoid a leak at the
> program exit. The same problem exists for both fetch-pack and send-pack,
> but not for the usual transport.c code paths, since we already do this
> cleanup in disconnect_git().
> 
> Fixing this lets us mark t5542 as leak-free for the send-pack side, but
> fetch-pack will need some more fixes before we can do the same for
> t5539.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/fetch-pack.c         | 1 +
>  builtin/send-pack.c          | 1 +
>  t/t5542-push-http-shallow.sh | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index cfc6951d23..ef4143eef3 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc,
>  	free_refs(fetched_refs);
>  	free_refs(remote_refs);
>  	list_objects_filter_release(&args.filter_options);
> +	oid_array_clear(&shallow);
>  	return ret;
>  }

I wonder about the early exit path we have when `finish_connect()`
returns non-zero. Should we make it go via the common cleanup path as
well, or do we not care in the error case?

> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 81fc96d423..c49fe6c53c 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -343,5 +343,6 @@ int cmd_send_pack(int argc,
>  	free_refs(remote_refs);
>  	free_refs(local_refs);
>  	refspec_clear(&rs);
> +	oid_array_clear(&shallow);
>  	return ret;
>  }

We also have an early exit in this function when `match_push_refs()`
returns non-zero.

Patrick

Re: [PATCH 06/28] fetch-pack, send-pack: clean up shallow oid array

[Jeff King]

On Thu, Sep 26, 2024 at 03:50:02PM +0200, Patrick Steinhardt wrote:

> > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> > index cfc6951d23..ef4143eef3 100644
> > --- a/builtin/fetch-pack.c
> > +++ b/builtin/fetch-pack.c
> > @@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc,
> >  	free_refs(fetched_refs);
> >  	free_refs(remote_refs);
> >  	list_objects_filter_release(&args.filter_options);
> > +	oid_array_clear(&shallow);
> >  	return ret;
> >  }
> 
> I wonder about the early exit path we have when `finish_connect()`
> returns non-zero. Should we make it go via the common cleanup path as
> well, or do we not care in the error case?

Yeah, I think it would technically be a leak if we hit that error case.
But it's pretty unlikely in practice (you'd have to finish the fetch and
then the "ssh" or "upload-pack" sub-process returns a non-zero exit code
anyway).

I'd prefer not to deal with it here for two reasons:

  1. The same leak is true of all the existing cleanup. So it really is
     orthogonal to what this patch is doing, and should be a separate
     patch.

  2. I think cleaning up in top-level builtins like this is mostly
     cosmetic to appease the leak checker. In the real world the memory
     is going back to the OS when we return anyway. Even in a libified
     world, I don't think cmd_fetch_pack() is a reasonable entry point
     (we already have a more lib-ish fetch_pack() that is used by the
     transport code).

So I don't think it would be wrong to refactor the cleanup to trigger on
that other early return. But I also suspect there are many such paths
lurking (and will continue to lurk, even after we run clean with
SANITIZE=leak) and I'm not sure how productive it is to hunt them down.

> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> > index 81fc96d423..c49fe6c53c 100644
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> > @@ -343,5 +343,6 @@ int cmd_send_pack(int argc,
> >  	free_refs(remote_refs);
> >  	free_refs(local_refs);
> >  	refspec_clear(&rs);
> > +	oid_array_clear(&shallow);
> >  	return ret;
> >  }
> 
> We also have an early exit in this function when `match_push_refs()`
> returns non-zero.

And I think this is in the same boat.


-Peff

[PATCH 07/28] commit: avoid leaking already-saved buffer

[Jeff King]

When we parse a commit via repo_parse_commit_internal(), if
save_commit_buffer is set we'll stuff the buffer of the object contents
into a cache, overwriting any previous value.

This can result in a leak of that previously cached value, though it's
rare in practice. If we have a value in the cache it would have come
from a previous parse, and during that parse we'd set the object.parsed
flag, causing any subsequent parse attempts to exit without doing any
work.

But it's possible to "unparse" a commit, which we do when registering a
commit graft. And since shallow fetches are implemented using grafts,
the leak is triggered in practice by t5539.

There are a number of possible ways to address this:

  1. the unparsing function could clear the cached commit buffer, too. I
     think this would work for the case I found, but I'm not sure if
     there are other ways to end up in the same state (an unparsed
     commit with an entry in the commit buffer cache).

  2. when we parse, we could check the buffer cache and prefer it to
     reading the contents from the object database. In theory the
     contents of a particular sha1 are immutable, but the code in
     question is violating the immutability with grafts. So this
     approach makes me a bit nervous, although I think it would work in
     practice (the grafts are applied to what we parse, but we still
     retain the original contents).

  3. We could realize the cache is already populated and discard its
     contents before overwriting. It's possible some other code could be
     holding on to a pointer to the old cache entry (and we'd introduce
     a use-after-free), but I think the risk of that is relatively low.

  4. The reverse of (3): when the cache is populated, don't bother
     saving our new copy. This is perhaps a little weird, since we'll
     have just populated the commit struct based on a different buffer.
     But the two buffers should be the same, even in the presence of
     grafts (as in (2) above).

I went with option 4. It addresses the leak directly and doesn't carry
any risk of breaking other assumptions. And it's the same technique used
by parse_object_buffer() for this situation, though I'm not sure when it
would even come up there. The extra safety has been there since
bd1e17e245 (Make "parse_object()" also fill in commit message buffer
data., 2005-05-25).

This lets us mark t5539 as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 commit.c                      | 3 ++-
 t/t5539-fetch-http-shallow.sh | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 3a54e4db0d..cc03a93036 100644
--- a/commit.c
+++ b/commit.c
@@ -595,7 +595,8 @@ int repo_parse_commit_internal(struct repository *r,
 	}
 
 	ret = parse_commit_buffer(r, item, buffer, size, 0);
-	if (save_commit_buffer && !ret) {
+	if (save_commit_buffer && !ret &&
+	    !get_cached_commit_buffer(r, item, NULL)) {
 		set_commit_buffer(r, item, buffer, size);
 		return 0;
 	}
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 3ea75d34ca..82fe09d0a9 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -5,6 +5,7 @@ test_description='fetch/clone from a shallow clone over http'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
-- 
2.46.2.1011.gf1f9323e02

Re: [PATCH 07/28] commit: avoid leaking already-saved buffer

[Patrick Steinhardt]

On Tue, Sep 24, 2024 at 05:54:34PM -0400, Jeff King wrote:
> When we parse a commit via repo_parse_commit_internal(), if
> save_commit_buffer is set we'll stuff the buffer of the object contents
> into a cache, overwriting any previous value.

Interesting. I saw some cases that I think could be caused by this, but
couldn't make much sense of them.

> This can result in a leak of that previously cached value, though it's
> rare in practice. If we have a value in the cache it would have come
> from a previous parse, and during that parse we'd set the object.parsed
> flag, causing any subsequent parse attempts to exit without doing any
> work.
> 
> But it's possible to "unparse" a commit, which we do when registering a
> commit graft. And since shallow fetches are implemented using grafts,
> the leak is triggered in practice by t5539.
> 
> There are a number of possible ways to address this:
> 
>   1. the unparsing function could clear the cached commit buffer, too. I

s/the/The/

>      think this would work for the case I found, but I'm not sure if
>      there are other ways to end up in the same state (an unparsed
>      commit with an entry in the commit buffer cache).
> 
>   2. when we parse, we could check the buffer cache and prefer it to

s/when/When/

>      reading the contents from the object database. In theory the
>      contents of a particular sha1 are immutable, but the code in
>      question is violating the immutability with grafts. So this
>      approach makes me a bit nervous, although I think it would work in
>      practice (the grafts are applied to what we parse, but we still
>      retain the original contents).
> 
>   3. We could realize the cache is already populated and discard its
>      contents before overwriting. It's possible some other code could be
>      holding on to a pointer to the old cache entry (and we'd introduce
>      a use-after-free), but I think the risk of that is relatively low.
> 
>   4. The reverse of (3): when the cache is populated, don't bother
>      saving our new copy. This is perhaps a little weird, since we'll
>      have just populated the commit struct based on a different buffer.
>      But the two buffers should be the same, even in the presence of
>      grafts (as in (2) above).
> 
> I went with option 4. It addresses the leak directly and doesn't carry
> any risk of breaking other assumptions. And it's the same technique used
> by parse_object_buffer() for this situation, though I'm not sure when it
> would even come up there. The extra safety has been there since
> bd1e17e245 (Make "parse_object()" also fill in commit message buffer
> data., 2005-05-25).

Okay. This feels a bit weird indeed, but the fact that we already use
the same strategy in other places makes me feel way safer.

> This lets us mark t5539 as leak-free.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  commit.c                      | 3 ++-
>  t/t5539-fetch-http-shallow.sh | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/commit.c b/commit.c
> index 3a54e4db0d..cc03a93036 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -595,7 +595,8 @@ int repo_parse_commit_internal(struct repository *r,
>  	}
>  
>  	ret = parse_commit_buffer(r, item, buffer, size, 0);
> -	if (save_commit_buffer && !ret) {
> +	if (save_commit_buffer && !ret &&
> +	    !get_cached_commit_buffer(r, item, NULL)) {
>  		set_commit_buffer(r, item, buffer, size);
>  		return 0;
>  	}

And the fix is trivial.

Patrick

[PATCH 08/28] send-pack: free cas options before exit

[Jeff King]

The send-pack --force-with-lease option populates a push_cas_option
struct with allocated strings. Exiting without cleaning this up will
cause leak-checkers to complain.

We can fix this by calling clear_cas_option(), after making it publicly
available. Previously it was used only for resetting the list when we
saw --no-force-with-lease.

The git-push command has the same "leak", though in this case it won't
trigger a leak-checker since it stores the push_cas_option struct as a
global rather than on the stack (and is thus reachable even after main()
exits). I've added cleanup for it here anyway, though, as
future-proofing.

The leak is triggered by t5541 (it tests --force-with-lease over http,
which requires a separate send-pack process under the hood), but we
can't mark it as leak-free yet.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/push.c      | 1 +
 builtin/send-pack.c | 1 +
 remote.c            | 2 +-
 remote.h            | 1 +
 4 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/push.c b/builtin/push.c
index e6f48969b8..59d4485603 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -669,6 +669,7 @@ int cmd_push(int argc,
 	rc = do_push(flags, push_options, remote);
 	string_list_clear(&push_options_cmdline, 0);
 	string_list_clear(&push_options_config, 0);
+	clear_cas_option(&cas);
 	if (rc == -1)
 		usage_with_options(push_usage, options);
 	else
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index c49fe6c53c..8b1d46e79a 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -344,5 +344,6 @@ int cmd_send_pack(int argc,
 	free_refs(local_refs);
 	refspec_clear(&rs);
 	oid_array_clear(&shallow);
+	clear_cas_option(&cas);
 	return ret;
 }
diff --git a/remote.c b/remote.c
index 390a03c264..e291e8ff5c 100644
--- a/remote.c
+++ b/remote.c
@@ -2544,7 +2544,7 @@ struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map)
 /*
  * Compare-and-swap
  */
-static void clear_cas_option(struct push_cas_option *cas)
+void clear_cas_option(struct push_cas_option *cas)
 {
 	int i;
 
diff --git a/remote.h b/remote.h
index a58713f20a..ad4513f639 100644
--- a/remote.h
+++ b/remote.h
@@ -409,6 +409,7 @@ struct push_cas_option {
 };
 
 int parseopt_push_cas_option(const struct option *, const char *arg, int unset);
+void clear_cas_option(struct push_cas_option *);
 
 int is_empty_cas(const struct push_cas_option *);
 void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);
-- 
2.46.2.1011.gf1f9323e02

Re: [PATCH 08/28] send-pack: free cas options before exit

[Patrick Steinhardt]

On Tue, Sep 24, 2024 at 05:55:39PM -0400, Jeff King wrote:
> diff --git a/remote.c b/remote.c
> index 390a03c264..e291e8ff5c 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2544,7 +2544,7 @@ struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map)
>  /*
>   * Compare-and-swap
>   */
> -static void clear_cas_option(struct push_cas_option *cas)
> +void clear_cas_option(struct push_cas_option *cas)
>  {
>  	int i;
>  
> diff --git a/remote.h b/remote.h
> index a58713f20a..ad4513f639 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -409,6 +409,7 @@ struct push_cas_option {
>  };
>  
>  int parseopt_push_cas_option(const struct option *, const char *arg, int unset);
> +void clear_cas_option(struct push_cas_option *);

Nit: I was wondering whether we'd also want to fix up this functions
name to conform to our style guide, which says this should be called
`push_cas_option_clear()` instead. But I don't mind it much, so please
feel free to ignore this nit.

Patrick

Re: [PATCH 08/28] send-pack: free cas options before exit

[Jeff King]

On Thu, Sep 26, 2024 at 03:50:09PM +0200, Patrick Steinhardt wrote:

> On Tue, Sep 24, 2024 at 05:55:39PM -0400, Jeff King wrote:
> > diff --git a/remote.c b/remote.c
> > index 390a03c264..e291e8ff5c 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -2544,7 +2544,7 @@ struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map)
> >  /*
> >   * Compare-and-swap
> >   */
> > -static void clear_cas_option(struct push_cas_option *cas)
> > +void clear_cas_option(struct push_cas_option *cas)
> >  {
> >  	int i;
> >  
> > diff --git a/remote.h b/remote.h
> > index a58713f20a..ad4513f639 100644
> > --- a/remote.h
> > +++ b/remote.h
> > @@ -409,6 +409,7 @@ struct push_cas_option {
> >  };
> >  
> >  int parseopt_push_cas_option(const struct option *, const char *arg, int unset);
> > +void clear_cas_option(struct push_cas_option *);
> 
> Nit: I was wondering whether we'd also want to fix up this functions
> name to conform to our style guide, which says this should be called
> `push_cas_option_clear()` instead. But I don't mind it much, so please
> feel free to ignore this nit.

I'd prefer to punt on that for now, as the whole suite of "methods" for
this struct would need to be renamed to match that style. If we were
making a too-short name into a public symbol, I'd be worried about
addressing that now, but I think this is purely about style and can
wait.

-Peff

[PATCH 09/28] transport-helper: fix strbuf leak in push_refs_with_push()

[Jeff King]

We loop over the refs to push, building up a strbuf with the set of
"push" directives to send to the remote helper. But if the atomic-push
flag is set and we hit a rejected ref, we'll bail from the function
early. We clean up most things, but forgot to release the strbuf.

Fixing this lets us mark t5541 as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
Arguably this whole function could benefit from a cleanup goto, but it's
a little non-trivial. So I stuck with simple-and-stupid.

 t/t5541-http-push-smart.sh | 1 +
 transport-helper.c         | 1 +
 2 files changed, 2 insertions(+)

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 71428f3d5c..3ad514bbd4 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -7,6 +7,7 @@ test_description='test smart pushing over http via http-backend'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 ROOT_PATH="$PWD"
diff --git a/transport-helper.c b/transport-helper.c
index c688967b8c..9c8abd8eca 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1023,6 +1023,7 @@ static int push_refs_with_push(struct transport *transport,
 			if (atomic) {
 				reject_atomic_push(remote_refs, mirror);
 				string_list_clear(&cas_options, 0);
+				strbuf_release(&buf);
 				return 0;
 			} else
 				continue;
-- 
2.46.2.1011.gf1f9323e02

Re: [PATCH 09/28] transport-helper: fix strbuf leak in push_refs_with_push()

[Patrick Steinhardt]

On Tue, Sep 24, 2024 at 05:56:34PM -0400, Jeff King wrote:
> diff --git a/transport-helper.c b/transport-helper.c
> index c688967b8c..9c8abd8eca 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -1023,6 +1023,7 @@ static int push_refs_with_push(struct transport *transport,
>  			if (atomic) {
>  				reject_atomic_push(remote_refs, mirror);
>  				string_list_clear(&cas_options, 0);
> +				strbuf_release(&buf);
>  				return 0;
>  			} else
>  				continue;

What's not visible here is that a few lines further down we have another
early return where we don't clear `buf`. But that exit is conditioned on
`buf.len == 0`, and given that we never `strbuf_reset()` the buffer we
know that `buf.len == 0` only when it hasn't ever be allocated.

So this LGTM.

Patrick

Re: [PATCH 09/28] transport-helper: fix strbuf leak in push_refs_with_push()

[Jeff King]

On Thu, Sep 26, 2024 at 03:50:14PM +0200, Patrick Steinhardt wrote:

> On Tue, Sep 24, 2024 at 05:56:34PM -0400, Jeff King wrote:
> > diff --git a/transport-helper.c b/transport-helper.c
> > index c688967b8c..9c8abd8eca 100644
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -1023,6 +1023,7 @@ static int push_refs_with_push(struct transport *transport,
> >  			if (atomic) {
> >  				reject_atomic_push(remote_refs, mirror);
> >  				string_list_clear(&cas_options, 0);
> > +				strbuf_release(&buf);
> >  				return 0;
> >  			} else
> >  				continue;
> 
> What's not visible here is that a few lines further down we have another
> early return where we don't clear `buf`. But that exit is conditioned on
> `buf.len == 0`, and given that we never `strbuf_reset()` the buffer we
> know that `buf.len == 0` only when it hasn't ever be allocated.

Yeah, I noticed that, too, and came to the same conclusion. As you note,
it's _possible_ to have an allocated but zero-length strbuf, but I don't
think it happens here. So it's OK in practice.

If we were going to refactor, I think it would make sense to do it with
a cleanup label to cover both of these early returns, plus the cleanup
at the end. I was mostly just going for minimal changes where possible.

-Peff

[PATCH 10/28] fetch: free "raw" string when shrinking refspec

[Jeff King]

The "--prefetch" option to git-fetch modifies the default refspec,
including eliminating some entries entirely. When we drop an entry we
free the strings in the refspec_item, but we forgot to free the matching
string in the "raw" array of the refspec struct. There's no behavioral
bug here (since we correctly shrink the raw array, too), but we're
leaking the allocated string.

Let's add in the leak-fix, and while we're at it drop "const" from
the type of the raw string array. These strings are always allocated by
refspec_append(), etc, and this makes the memory ownership more clear.

This is all a bit more intimate with the refspec code than I'd like, and
I suspect it would be better if each refspec_item held on to its own raw
string, we had a single array, and we could use refspec_item_clear() to
clean up everything. But that's a non-trivial refactoring, since
refspec_item structs can be held outside of a "struct refspec", without
having a matching raw string at all. So let's leave that for now and
just fix the leak in the most immediate way.

This lets us mark t5582 as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch.c                   | 1 +
 refspec.c                         | 2 +-
 refspec.h                         | 2 +-
 t/t5582-fetch-negative-refspec.sh | 1 +
 4 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c900f57721..80a64d0d26 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -456,6 +456,7 @@ static void filter_prefetch_refspec(struct refspec *rs)
 
 			free(rs->items[i].src);
 			free(rs->items[i].dst);
+			free(rs->raw[i]);
 
 			for (j = i + 1; j < rs->nr; j++) {
 				rs->items[j - 1] = rs->items[j];
diff --git a/refspec.c b/refspec.c
index ec90ab349a..c3cf003443 100644
--- a/refspec.c
+++ b/refspec.c
@@ -225,7 +225,7 @@ void refspec_clear(struct refspec *rs)
 	rs->nr = 0;
 
 	for (i = 0; i < rs->raw_nr; i++)
-		free((char *)rs->raw[i]);
+		free(rs->raw[i]);
 	FREE_AND_NULL(rs->raw);
 	rs->raw_alloc = 0;
 	rs->raw_nr = 0;
diff --git a/refspec.h b/refspec.h
index 754be45cee..3760fdaf2b 100644
--- a/refspec.h
+++ b/refspec.h
@@ -43,7 +43,7 @@ struct refspec {
 	int alloc;
 	int nr;
 
-	const char **raw;
+	char **raw;
 	int raw_alloc;
 	int raw_nr;
 
diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
index 7a80e47c2b..7fa54a4029 100755
--- a/t/t5582-fetch-negative-refspec.sh
+++ b/t/t5582-fetch-negative-refspec.sh
@@ -8,6 +8,7 @@ test_description='"git fetch" with negative refspecs.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
-- 
2.46.2.1011.gf1f9323e02

[PATCH 11/28] fetch-pack: clear pack lockfiles list

[Jeff King]

If the --lock-pack option is passed (which it typically is when
fetch-pack is used under the hood by smart-http), then we may end up
with entries in our pack_lockfiles string_list. We need to clear them
before returning to avoid a leak.

In git-fetch this isn't a problem, since the same cleanup happens via
transport_unlock_pack(). But the leak is detectable in t5551, which does
http fetches.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index ef4143eef3..62e8c3aa6b 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -295,5 +295,6 @@ int cmd_fetch_pack(int argc,
 	free_refs(remote_refs);
 	list_objects_filter_release(&args.filter_options);
 	oid_array_clear(&shallow);
+	string_list_clear(&pack_lockfiles, 0);
 	return ret;
 }
-- 
2.46.2.1011.gf1f9323e02

[PATCH 12/28] transport-helper: fix leak of dummy refs_list

[Jeff King]

When using a remote-helper, the fetch_refs() function will issue a
"list" command if we haven't already done so. We don't care about the
result, but this is just to maintain compatibility as explained in
ac3fda82bf (transport-helper: skip ls-refs if unnecessary, 2019-08-21).

But get_refs_list_using_list(), the function we call to issue the
command, does parse and return the resulting ref list, which we simply
leak. We should record the return value and free it immediately (another
approach would be to teach it to avoid allocating at all, but it does
not seem worth the trouble to micro-optimize this mostly historical
case).

Triggering this requires the v0 protocol (since in v2 we use stateless
connect to take over the connection). You can see it in t5551.37, "fetch
by SHA-1 without tag following", as it explicitly enables v0.

Signed-off-by: Jeff King <peff@peff.net>
---
 transport-helper.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 9c8abd8eca..013ec79dc9 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -717,8 +717,14 @@ static int fetch_refs(struct transport *transport,
 		return -1;
 	}
 
-	if (!data->get_refs_list_called)
-		get_refs_list_using_list(transport, 0);
+	if (!data->get_refs_list_called) {
+		/*
+		 * We do not care about the list of refs returned, but only
+		 * that the "list" command was sent.
+		 */
+		struct ref *dummy = get_refs_list_using_list(transport, 0);
+		free_refs(dummy);
+	}
 
 	count = 0;
 	for (i = 0; i < nr_heads; i++)
-- 
2.46.2.1011.gf1f9323e02

[PATCH 13/28] http: fix leak when redacting cookies from curl trace

[Jeff King]

When redacting headers for GIT_TRACE_CURL, we build up a redacted cookie
header in a local strbuf, and then copy it into the output. But we
forget to release the temporary strbuf, leaking it for every cookie
header we show.

The other redacted headers don't run into this problem, since they're
able to work in-place in the output buffer. But the cookie parsing is
too complicated for that, since we redact the cookies individually.

This leak is triggered by the cookie tests in t5551.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/http.c b/http.c
index 6c6cc5c822..cc136408c0 100644
--- a/http.c
+++ b/http.c
@@ -800,6 +800,7 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset)
 
 		strbuf_setlen(header, sensitive_header - header->buf);
 		strbuf_addbuf(header, &redacted_header);
+		strbuf_release(&redacted_header);
 		ret = 1;
 	}
 	return ret;
-- 
2.46.2.1011.gf1f9323e02

[PATCH 14/28] http: fix leak of http_object_request struct

[Jeff King]

The new_http_object_request() function allocates a struct on the heap,
along with some fields inside the struct. But the matching function to
clean it up, release_http_object_request(), only frees the interior
fields without freeing the struct itself, causing a leak.

The related http_pack_request new/release pair gets this right, and at
first glance we should be able to do the same thing and just add a
single free() call. But there's a catch.

These http_object_request structs are typically embedded in the
object_request struct of http-walker.c. And when we clean up that parent
struct, it sanity-checks the embedded struct to make sure we are not
leaking descriptors. Which means a use-after-free if we simply free()
the embedded struct.

I have no idea how valuable that sanity-check is, or whether it can
simply be deleted. This all goes back to 5424bc557f (http*: add helper
methods for fetching objects (loose), 2009-06-06). But the obvious way
to make it all work is to be sure we set the pointer to NULL after
freeing it (and our freeing process closes the descriptor, so we know
there is no leak).

To make sure we do that consistently, we'll switch the pointer we take
in release_http_object_request() to a pointer-to-pointer, and we'll set
it to NULL ourselves. And then the compiler can help us find each caller
which needs to be updated.

Most cases will just pass "&obj_req->req", which will obviously do the
right thing. In a few cases, like http-push's finish_request(), we are
working with a copy of the pointer, so we don't NULL the original. But
it's OK because the next step is to free the struct containing the
original pointer anyway.

This lets us mark t5551 as leak-free. Ironically this is the "smart"
http test, and the leak here only affects dumb http. But there's a
single dumb-http invocation in there. The full dumb tests are in t5550,
which still has some more leaks.

This also makes t5559 leak-free, as it's just an HTTP/2 variant of
t5551. But we don't need to mark it as such, since it inherits the flag
from t5551.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c                 |  4 ++--
 http-walker.c               |  8 ++++----
 http.c                      | 11 ++++++++---
 http.h                      |  4 ++--
 t/t5551-http-fetch-smart.sh |  1 +
 5 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/http-push.c b/http-push.c
index 7315a694aa..7196ffa525 100644
--- a/http-push.c
+++ b/http-push.c
@@ -275,7 +275,7 @@ static void start_fetch_loose(struct transfer_request *request)
 	if (!start_active_slot(slot)) {
 		fprintf(stderr, "Unable to start GET request\n");
 		repo->can_update_info_refs = 0;
-		release_http_object_request(obj_req);
+		release_http_object_request(&obj_req);
 		release_request(request);
 	}
 }
@@ -580,7 +580,7 @@ static void finish_request(struct transfer_request *request)
 
 		/* Try fetching packed if necessary */
 		if (request->obj->flags & LOCAL) {
-			release_http_object_request(obj_req);
+			release_http_object_request(&obj_req);
 			release_request(request);
 		} else
 			start_fetch_packed(request);
diff --git a/http-walker.c b/http-walker.c
index e417a7f51c..9c1e5c37e6 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -74,7 +74,7 @@ static void start_object_request(struct object_request *obj_req)
 	obj_req->state = ACTIVE;
 	if (!start_active_slot(slot)) {
 		obj_req->state = ABORTED;
-		release_http_object_request(req);
+		release_http_object_request(&req);
 		return;
 	}
 }
@@ -110,7 +110,7 @@ static void process_object_response(void *callback_data)
 		if (obj_req->repo->next) {
 			obj_req->repo =
 				obj_req->repo->next;
-			release_http_object_request(obj_req->req);
+			release_http_object_request(&obj_req->req);
 			start_object_request(obj_req);
 			return;
 		}
@@ -495,7 +495,7 @@ static int fetch_object(struct walker *walker, unsigned char *hash)
 
 	if (repo_has_object_file(the_repository, &obj_req->oid)) {
 		if (obj_req->req)
-			abort_http_object_request(obj_req->req);
+			abort_http_object_request(&obj_req->req);
 		abort_object_request(obj_req);
 		return 0;
 	}
@@ -543,7 +543,7 @@ static int fetch_object(struct walker *walker, unsigned char *hash)
 		strbuf_release(&buf);
 	}
 
-	release_http_object_request(req);
+	release_http_object_request(&obj_req->req);
 	release_object_request(obj_req);
 	return ret;
 }
diff --git a/http.c b/http.c
index cc136408c0..d0242ffb50 100644
--- a/http.c
+++ b/http.c
@@ -2816,15 +2816,17 @@ int finish_http_object_request(struct http_object_request *freq)
 	return freq->rename;
 }
 
-void abort_http_object_request(struct http_object_request *freq)
+void abort_http_object_request(struct http_object_request **freq_p)
 {
+	struct http_object_request *freq = *freq_p;
 	unlink_or_warn(freq->tmpfile.buf);
 
-	release_http_object_request(freq);
+	release_http_object_request(freq_p);
 }
 
-void release_http_object_request(struct http_object_request *freq)
+void release_http_object_request(struct http_object_request **freq_p)
 {
+	struct http_object_request *freq = *freq_p;
 	if (freq->localfile != -1) {
 		close(freq->localfile);
 		freq->localfile = -1;
@@ -2838,4 +2840,7 @@ void release_http_object_request(struct http_object_request *freq)
 	}
 	curl_slist_free_all(freq->headers);
 	strbuf_release(&freq->tmpfile);
+
+	free(freq);
+	*freq_p = NULL;
 }
diff --git a/http.h b/http.h
index a516ca4a9a..46e334c2c2 100644
--- a/http.h
+++ b/http.h
@@ -240,8 +240,8 @@ struct http_object_request *new_http_object_request(
 	const char *base_url, const struct object_id *oid);
 void process_http_object_request(struct http_object_request *freq);
 int finish_http_object_request(struct http_object_request *freq);
-void abort_http_object_request(struct http_object_request *freq);
-void release_http_object_request(struct http_object_request *freq);
+void abort_http_object_request(struct http_object_request **freq);
+void release_http_object_request(struct http_object_request **freq);
 
 /*
  * Instead of using environment variables to determine if curl tracing happens,
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 7b5ab0eae1..e36dfde17e 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -5,6 +5,7 @@ test_description="test smart fetching over http via http-backend ($HTTP_PROTO)"
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-httpd.sh
 test "$HTTP_PROTO" = "HTTP/2" && enable_http2
-- 
2.46.2.1011.gf1f9323e02

Re: [PATCH 14/28] http: fix leak of http_object_request struct

[Patrick Steinhardt]

On Tue, Sep 24, 2024 at 06:01:09PM -0400, Jeff King wrote:
> The new_http_object_request() function allocates a struct on the heap,
> along with some fields inside the struct. But the matching function to
> clean it up, release_http_object_request(), only frees the interior
> fields without freeing the struct itself, causing a leak.

Oh yeah, I remember staring at this code and being completely confused
as to how this all works.

> diff --git a/http.c b/http.c
> index cc136408c0..d0242ffb50 100644
> --- a/http.c
> +++ b/http.c
> @@ -2816,15 +2816,17 @@ int finish_http_object_request(struct http_object_request *freq)
>  	return freq->rename;
>  }
>  
> -void abort_http_object_request(struct http_object_request *freq)
> +void abort_http_object_request(struct http_object_request **freq_p)
>  {
> +	struct http_object_request *freq = *freq_p;
>  	unlink_or_warn(freq->tmpfile.buf);
>  
> -	release_http_object_request(freq);
> +	release_http_object_request(freq_p);
>  }
>  
> -void release_http_object_request(struct http_object_request *freq)
> +void release_http_object_request(struct http_object_request **freq_p)
>  {
> +	struct http_object_request *freq = *freq_p;
>  	if (freq->localfile != -1) {
>  		close(freq->localfile);
>  		freq->localfile = -1;
> @@ -2838,4 +2840,7 @@ void release_http_object_request(struct http_object_request *freq)
>  	}
>  	curl_slist_free_all(freq->headers);
>  	strbuf_release(&freq->tmpfile);
> +
> +	free(freq);
> +	*freq_p = NULL;
>  }

Okay, looks simple enough. But I found the whole code in "http.c" to be
quite... elusive, so I had a hard time finding my way.

Patrick

Re: [PATCH 14/28] http: fix leak of http_object_request struct

[Jeff King]

On Thu, Sep 26, 2024 at 03:50:17PM +0200, Patrick Steinhardt wrote:

> Okay, looks simple enough. But I found the whole code in "http.c" to be
> quite... elusive, so I had a hard time finding my way.

You are not the only one. ;)

-Peff

[PATCH 15/28] http: call git_inflate_end() when releasing http_object_request

[Jeff King]

In new_http_object_request(), we initialize the zlib stream with
git_inflate_init(). We must have a matching git_inflate_end() to avoid
leaking any memory allocated by zlib.

In most cases this happens in finish_http_object_request(), but we don't
always get there. If we abort a request mid-stream, then we may clean it
up without hitting that function.

We can't just add a git_inflate_end() call to the release function,
though. That would double-free the cases that did actually finish.
Instead, we'll move the call from the finish function to the release
function. This does delay it for the cases that do finish, but I don't
think it matters. We should have already reached Z_STREAM_END (and
complain if we didn't), and we do not record any status code from
git_inflate_end().

This leak is triggered by t5550 at least (and probably other dumb-http
tests).

I did find one other related spot of interest. If we try to read a
previously downloaded file and fail, we reset the stream by calling
memset() followed by a fresh git_inflate_init(). I don't think this case
is triggered in the test suite, but it seemed like an obvious leak, so I
added the appropriate git_inflate_end() before the memset() there.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/http.c b/http.c
index d0242ffb50..4d841becca 100644
--- a/http.c
+++ b/http.c
@@ -2726,6 +2726,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	 * file; also rewind to the beginning of the local file.
 	 */
 	if (prev_read == -1) {
+		git_inflate_end(&freq->stream);
 		memset(&freq->stream, 0, sizeof(freq->stream));
 		git_inflate_init(&freq->stream);
 		the_hash_algo->init_fn(&freq->c);
@@ -2799,7 +2800,6 @@ int finish_http_object_request(struct http_object_request *freq)
 		return -1;
 	}
 
-	git_inflate_end(&freq->stream);
 	the_hash_algo->final_oid_fn(&freq->real_oid, &freq->c);
 	if (freq->zret != Z_STREAM_END) {
 		unlink_or_warn(freq->tmpfile.buf);
@@ -2840,6 +2840,7 @@ void release_http_object_request(struct http_object_request **freq_p)
 	}
 	curl_slist_free_all(freq->headers);
 	strbuf_release(&freq->tmpfile);
+	git_inflate_end(&freq->stream);
 
 	free(freq);
 	*freq_p = NULL;
-- 
2.46.2.1011.gf1f9323e02

Re: [PATCH 15/28] http: call git_inflate_end() when releasing http_object_request

[Patrick Steinhardt]

On Tue, Sep 24, 2024 at 06:02:13PM -0400, Jeff King wrote:
> In new_http_object_request(), we initialize the zlib stream with
> git_inflate_init(). We must have a matching git_inflate_end() to avoid
> leaking any memory allocated by zlib.
> 
> In most cases this happens in finish_http_object_request(), but we don't
> always get there. If we abort a request mid-stream, then we may clean it
> up without hitting that function.
> 
> We can't just add a git_inflate_end() call to the release function,
> though. That would double-free the cases that did actually finish.
> Instead, we'll move the call from the finish function to the release
> function. This does delay it for the cases that do finish, but I don't
> think it matters. We should have already reached Z_STREAM_END (and
> complain if we didn't), and we do not record any status code from
> git_inflate_end().

I had to read this paragraph multiple times to understand it, as I
wondered why you did end up adding it to `release_http_object_request()`
even though the paragraph claims that you cannot. But what you say is
that you must _move_ the call, not add it, and that's what the patch
does.

So yeah, that does make sense.

Patrick

Re: [PATCH 15/28] http: call git_inflate_end() when releasing http_object_request

[Jeff King]

On Thu, Sep 26, 2024 at 03:50:20PM +0200, Patrick Steinhardt wrote:

> > We can't just add a git_inflate_end() call to the release function,
> > though. That would double-free the cases that did actually finish.
> > Instead, we'll move the call from the finish function to the release
> > function. This does delay it for the cases that do finish, but I don't
> > think it matters. We should have already reached Z_STREAM_END (and
> > complain if we didn't), and we do not record any status code from
> > git_inflate_end().
> 
> I had to read this paragraph multiple times to understand it, as I
> wondered why you did end up adding it to `release_http_object_request()`
> even though the paragraph claims that you cannot. But what you say is
> that you must _move_ the call, not add it, and that's what the patch
> does.

Yeah, I could see the confusion. It is really "We can't just add a
git_inflate_end() call and be done. We must also...".

I don't think it's worth a re-roll on its own, but if I do re-roll I'll
try to clarify.

-Peff

[PATCH 16/28] http: stop leaking buffer in http_get_info_packs()

[Jeff King]

We use http_get_strbuf() to fetch the remote info/packs content into a
strbuf, but never free it, causing a leak. There's no need to hold onto
it, as we've already parsed it completely.

This lets us mark t5619 as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c                                     | 1 +
 t/t5619-clone-local-ambiguous-transport.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/http.c b/http.c
index 4d841becca..54463770b4 100644
--- a/http.c
+++ b/http.c
@@ -2475,6 +2475,7 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)
 
 cleanup:
 	free(url);
+	strbuf_release(&buf);
 	return ret;
 }
 
diff --git a/t/t5619-clone-local-ambiguous-transport.sh b/t/t5619-clone-local-ambiguous-transport.sh
index cce62bf78d..1d4efe414d 100755
--- a/t/t5619-clone-local-ambiguous-transport.sh
+++ b/t/t5619-clone-local-ambiguous-transport.sh
@@ -2,6 +2,7 @@
 
 test_description='test local clone with ambiguous transport'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-httpd.sh"
 
-- 
2.46.2.1011.gf1f9323e02

[PATCH 17/28] remote-curl: free HEAD ref with free_one_ref()

[Jeff King]

After dumb-http downloads the remote info/refs file, it adds an extra
HEAD ref struct to our list by downloading the remote symref and finding
the matching ref within our list. If either of those fails, we throw
away the ref struct. But we do so with free(), when we should use
free_one_ref() to catch any embedded allocations (in particular, if
fetching the remote HEAD succeeded but the branch is unborn, its
ref->symref field will be populated but we'll still throw it all away).

This leak is triggered by t5550 (but we still have a little more work to
mark it leak-free).

Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 4adcf25ed6..9a71e04301 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -347,7 +347,7 @@ static struct ref *parse_info_refs(struct discovery *heads)
 		ref->next = refs;
 		refs = ref;
 	} else {
-		free(ref);
+		free_one_ref(ref);
 	}
 
 	return refs;
-- 
2.46.2.1011.gf1f9323e02

[PATCH 18/28] http-walker: free fake packed_git list

[Jeff King]

The dumb-http walker code creates a "fake" packed_git list representing
packs we've downloaded from the remote (I call it "fake" because
generally that struct is only used and managed by the local repository
struct). But during our cleanup phase we don't touch those at all,
causing a leak.

There's no support here from the rest of the object-database API, as
these structs are not meant to be freed, except when closing the object
store completely. But we can see that raw_object_store_clear() just
calls free() on them, and that's enough here to fix the leak.

I also added a call to close_pack() before each. In the regular code
this happens via close_object_store(), which we do as part of
raw_object_store_clear(). This is necessary to prevent leaking mmap'd
data (like the pack idx) or descriptors. The leak-checker won't catch
either of these itself, but I did confirm with some hacky warning()
calls and running t5550 that it's easy to leak at least index data.

This is all much more intimate with the packed_git struct than I'd like,
but I think fixing it would be a pretty big refactor. And it's just not
worth it for dumb-http code which is rarely used these days. If we can
silence the leak-checker without creating too much hassle, we should
just do that.

This lets us mark t5550 as leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-walker.c              | 10 ++++++++++
 t/t5550-http-fetch-dumb.sh |  1 +
 2 files changed, 11 insertions(+)

diff --git a/http-walker.c b/http-walker.c
index 9c1e5c37e6..fb2d86d5e7 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -579,8 +579,18 @@ static void cleanup(struct walker *walker)
 	if (data) {
 		alt = data->alt;
 		while (alt) {
+			struct packed_git *pack;
+
 			alt_next = alt->next;
 
+			pack = alt->packs;
+			while (pack) {
+				struct packed_git *pack_next = pack->next;
+				close_pack(pack);
+				free(pack);
+				pack = pack_next;
+			}
+
 			free(alt->base);
 			free(alt);
 
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index ea8e48f627..58189c9f7d 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -4,6 +4,7 @@ test_description='test dumb fetching over http via static file'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 if test_have_prereq !REFFILES
-- 
2.46.2.1011.gf1f9323e02

[PATCH 19/28] http-push: clear refspecs before exiting

[Jeff King]

We parse the command-line arguments into a refspec struct, but we never
free them. We should do so before exiting to avoid triggering the
leak-checker.

This triggers in t5540 many times (basically every invocation of
http-push).

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/http-push.c b/http-push.c
index 7196ffa525..f60b2ceba5 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1983,5 +1983,7 @@ int cmd_main(int argc, const char **argv)
 		request = next_request;
 	}
 
+	refspec_clear(&rs);
+
 	return rc;
 }
-- 
2.46.2.1011.gf1f9323e02

[PATCH 20/28] http-push: free repo->url string

[Jeff King]

Our repo->url string comes from str_end_url_with_slash(), which always
allocates its output buffer. We should free it before exiting to avoid
triggering the leak-checker.

This can be seen by leak-checking t5540.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/http-push.c b/http-push.c
index f60b2ceba5..52c53928a9 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1972,6 +1972,7 @@ int cmd_main(int argc, const char **argv)
  cleanup:
 	if (info_ref_lock)
 		unlock_remote(info_ref_lock);
+	free(repo->url);
 	free(repo);
 
 	http_cleanup();
-- 
2.46.2.1011.gf1f9323e02

Re: [PATCH 20/28] http-push: free repo->url string

[Patrick Steinhardt]

On Tue, Sep 24, 2024 at 06:04:46PM -0400, Jeff King wrote:
> Our repo->url string comes from str_end_url_with_slash(), which always
> allocates its output buffer. We should free it before exiting to avoid
> triggering the leak-checker.
> 
> This can be seen by leak-checking t5540.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  http-push.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/http-push.c b/http-push.c
> index f60b2ceba5..52c53928a9 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1972,6 +1972,7 @@ int cmd_main(int argc, const char **argv)
>   cleanup:
>  	if (info_ref_lock)
>  		unlock_remote(info_ref_lock);
> +	free(repo->url);
>  	free(repo);
>  
>  	http_cleanup();

I was wondering whether we also need to free `repo->path`, which is a
`char *` as well. But that is only being assigned pointers into command
line argument strings, and thus we do not have to free it.

Patrick

Re: [PATCH 20/28] http-push: free repo->url string

[Jeff King]

On Thu, Sep 26, 2024 at 03:50:24PM +0200, Patrick Steinhardt wrote:

> > diff --git a/http-push.c b/http-push.c
> > index f60b2ceba5..52c53928a9 100644
> > --- a/http-push.c
> > +++ b/http-push.c
> > @@ -1972,6 +1972,7 @@ int cmd_main(int argc, const char **argv)
> >   cleanup:
> >  	if (info_ref_lock)
> >  		unlock_remote(info_ref_lock);
> > +	free(repo->url);
> >  	free(repo);
> >  
> >  	http_cleanup();
> 
> I was wondering whether we also need to free `repo->path`, which is a
> `char *` as well. But that is only being assigned pointers into command
> line argument strings, and thus we do not have to free it.

Yeah, I think that "path" has the wrong type, and should be "const char
*". I was a little surprised the compiler does not complain, but it is
the old C gotcha: we assign to it via strchr(), which launders away the
const.

It would probably be OK to fix, though in general my dream is still that
we'd delete all of this code in the not too distant future. Even if we
keep dumb-http fetch for its resumability, dumb-http push just seems
like a useless holdover from the early days.

-Peff

[PATCH 21/28] http-push: free curl header lists

[Jeff King]

To pass headers to curl, we have to allocate a curl_slist linked list
and then feed it to curl_easy_setopt(). But the header list is not
copied by curl, and must remain valid until we are finished with the
request.

A few spots in http-push get this right, freeing the list after
finishing the request, but many do not. In most cases the fix is simple:
we set up the curl slot, start it, and then use run_active_slot() to
take it to completion. After that, we don't need the headers anymore and
can call curl_slist_free_all().

But one case is trickier: when we do a MOVE request, we start the
request but don't immediately finish it. It's possible we could change
this to be more like the other requests, but I didn't want to get into
risky refactoring of this code. So we need to stick the header list into
the request struct and remember to free it later.

Curiously, the struct already has a headers field for this purpose! It
goes all the way back to 58e60dd203 (Add support for pushing to a remote
repository using HTTP/DAV, 2005-11-02), but it doesn't look like it was
ever used. We can make use of it just by assigning our headers to it,
and there is already code in finish_request() to clean it up.

This fixes several leaks triggered by t5540.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/http-push.c b/http-push.c
index 52c53928a9..451f7d14bb 100644
--- a/http-push.c
+++ b/http-push.c
@@ -437,9 +437,11 @@ static void start_move(struct transfer_request *request)
 	if (start_active_slot(slot)) {
 		request->slot = slot;
 		request->state = RUN_MOVE;
+		request->headers = dav_headers;
 	} else {
 		request->state = ABORTED;
 		FREE_AND_NULL(request->url);
+		curl_slist_free_all(dav_headers);
 	}
 }
 
@@ -1398,6 +1400,7 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock)
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
 		strbuf_release(&out_buffer.buf);
+		curl_slist_free_all(dav_headers);
 		if (results.curl_result != CURLE_OK) {
 			fprintf(stderr,
 				"PUT error: curl result=%d, HTTP code=%ld\n",
@@ -1407,6 +1410,7 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock)
 		}
 	} else {
 		strbuf_release(&out_buffer.buf);
+		curl_slist_free_all(dav_headers);
 		fprintf(stderr, "Unable to start PUT request\n");
 		return 0;
 	}
@@ -1516,6 +1520,7 @@ static void update_remote_info_refs(struct remote_lock *lock)
 					results.curl_result, results.http_code);
 			}
 		}
+		curl_slist_free_all(dav_headers);
 	}
 	strbuf_release(&buffer.buf);
 }
-- 
2.46.2.1011.gf1f9323e02

Re: [PATCH 21/28] http-push: free curl header lists

[Patrick Steinhardt]

On Tue, Sep 24, 2024 at 06:05:50PM -0400, Jeff King wrote:
> diff --git a/http-push.c b/http-push.c
> index 52c53928a9..451f7d14bb 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1398,6 +1400,7 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock)
>  	if (start_active_slot(slot)) {
>  		run_active_slot(slot);
>  		strbuf_release(&out_buffer.buf);
> +		curl_slist_free_all(dav_headers);
>  		if (results.curl_result != CURLE_OK) {
>  			fprintf(stderr,
>  				"PUT error: curl result=%d, HTTP code=%ld\n",
> @@ -1407,6 +1410,7 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock)
>  		}
>  	} else {
>  		strbuf_release(&out_buffer.buf);
> +		curl_slist_free_all(dav_headers);
>  		fprintf(stderr, "Unable to start PUT request\n");
>  		return 0;
>  	}

I was quite confused by the layout of this function, where we had
another `return 1` at the end. It took me a second to realize that this
is the error case for the `if (start_active_slot(slot))` condition
further up. But we do already free the headers in that case, so we're
good.

Patric

[PATCH 22/28] http-push: free transfer_request dest field

[Jeff King]

When we issue a PUT request, we store the destination in the "dest"
field by detaching from a strbuf. But we never free the result, causing
a leak.

We can address this in the release_request() function. But note that we
also need to initialize it to NULL, as most other request types do not
set it at all.

Curiously there are _two_ functions to initialize a transfer_request
struct. Adding the initialization only to add_fetch_request() seems to
be enough for t5540, but I won't pretend to understand why. Rather than
just adding "request->dest = NULL" in both spots, let's zero the whole
struct. That addresses this problem, as well as any future ones (and it
can't possibly hurt, as by definition we'd be hitting uninitialized
memory previously).

This fixes several leaks noticed by t5540.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/http-push.c b/http-push.c
index 451f7d14bb..9aa4d11ccd 100644
--- a/http-push.c
+++ b/http-push.c
@@ -514,6 +514,7 @@ static void release_request(struct transfer_request *request)
 	}
 
 	free(request->url);
+	free(request->dest);
 	free(request);
 }
 
@@ -651,11 +652,8 @@ static void add_fetch_request(struct object *obj)
 		return;
 
 	obj->flags |= FETCHING;
-	request = xmalloc(sizeof(*request));
+	CALLOC_ARRAY(request, 1);
 	request->obj = obj;
-	request->url = NULL;
-	request->lock = NULL;
-	request->headers = NULL;
 	request->state = NEED_FETCH;
 	request->next = request_queue_head;
 	request_queue_head = request;
@@ -687,11 +685,9 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)
 	}
 
 	obj->flags |= PUSHING;
-	request = xmalloc(sizeof(*request));
+	CALLOC_ARRAY(request, 1);
 	request->obj = obj;
-	request->url = NULL;
 	request->lock = lock;
-	request->headers = NULL;
 	request->state = NEED_PUSH;
 	request->next = request_queue_head;
 	request_queue_head = request;
-- 
2.46.2.1011.gf1f9323e02

[PATCH 23/28] http-push: free transfer_request strbuf

[Jeff King]

When we issue a PUT, we initialize and fill a strbuf embedded in the
transfer_request struct. But we never release this buffer, causing a
leak.

We can fix this by adding a strbuf_release() call to release_request().
If we stopped there, then non-PUT requests would try to release a
zero-initialized strbuf. This works OK in practice, but we should try to
follow the strbuf API more closely. So instead, we'll always initialize
the strbuf when we create the transfer_request struct.

That in turn means switching the strbuf_init() call in start_put() to a
simple strbuf_grow().

This leak is triggered in t5540.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/http-push.c b/http-push.c
index 9aa4d11ccd..8acdb3f265 100644
--- a/http-push.c
+++ b/http-push.c
@@ -375,7 +375,7 @@ static void start_put(struct transfer_request *request)
 	/* Set it up */
 	git_deflate_init(&stream, zlib_compression_level);
 	size = git_deflate_bound(&stream, len + hdrlen);
-	strbuf_init(&request->buffer.buf, size);
+	strbuf_grow(&request->buffer.buf, size);
 	request->buffer.posn = 0;
 
 	/* Compress it */
@@ -515,6 +515,7 @@ static void release_request(struct transfer_request *request)
 
 	free(request->url);
 	free(request->dest);
+	strbuf_release(&request->buffer.buf);
 	free(request);
 }
 
@@ -655,6 +656,7 @@ static void add_fetch_request(struct object *obj)
 	CALLOC_ARRAY(request, 1);
 	request->obj = obj;
 	request->state = NEED_FETCH;
+	strbuf_init(&request->buffer.buf, 0);
 	request->next = request_queue_head;
 	request_queue_head = request;
 
@@ -689,6 +691,7 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)
 	request->obj = obj;
 	request->lock = lock;
 	request->state = NEED_PUSH;
+	strbuf_init(&request->buffer.buf, 0);
 	request->next = request_queue_head;
 	request_queue_head = request;
 
-- 
2.46.2.1011.gf1f9323e02

[PATCH 24/28] http-push: free remote_ls_ctx.dentry_name

[Jeff King]

The remote_ls_ctx struct has dentry_name string, which is filled in with
a heap allocation in the handle_remote_ls_ctx() XML callback. After the
XML parse is done in remote_ls(), we should free the string to avoid a
leak.

This fixes several leaks found by running t5540.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/http-push.c b/http-push.c
index 8acdb3f265..2e1c6851bb 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1183,6 +1183,7 @@ static void remote_ls(const char *path, int flags,
 	}
 
 	free(ls.path);
+	free(ls.dentry_name);
 	free(url);
 	strbuf_release(&out_buffer.buf);
 	strbuf_release(&in_buffer);
-- 
2.46.2.1011.gf1f9323e02

[PATCH 25/28] http-push: free xml_ctx.cdata after use

[Jeff King]

When we ask libexpat to parse XML data, we sometimes set xml_cdata as a
CharacterDataHandler callback. This fills in an allocated string in the
xml_ctx struct which we never free, causing a leak.

I won't pretend to understand the purpose of the field, but it looks
like it is used by other callbacks during the parse. At any rate, we
never look at it again after XML_Parse() returns, so we should be OK to
free() it then.

This fixes several leaks triggered by t5540.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/http-push.c b/http-push.c
index 2e1c6851bb..1146d7c6fe 100644
--- a/http-push.c
+++ b/http-push.c
@@ -913,6 +913,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 			result = XML_Parse(parser, in_buffer.buf,
 					   in_buffer.len, 1);
 			free(ctx.name);
+			free(ctx.cdata);
 			if (result != XML_STATUS_OK) {
 				fprintf(stderr, "XML error: %s\n",
 					XML_ErrorString(
@@ -1170,6 +1171,7 @@ static void remote_ls(const char *path, int flags,
 			result = XML_Parse(parser, in_buffer.buf,
 					   in_buffer.len, 1);
 			free(ctx.name);
+			free(ctx.cdata);
 
 			if (result != XML_STATUS_OK) {
 				fprintf(stderr, "XML error: %s\n",
-- 
2.46.2.1011.gf1f9323e02

[PATCH 26/28] http-push: clean up objects list

[Jeff King]

In http-push's get_delta(), we generate a list of pending objects by
recursively processing trees and blobs, adding them to a linked list.
And then we iterate over the list, adding a new request for each
element.

But since we iterate using the list head pointer, at the end it is NULL
and all of the actual list structs have been leaked.

We can fix this either by using a separate iterator and then calling
object_list_free(), or by just freeing as we go. I picked the latter,
just because it means we continue to shrink the list as we go, though
I'm not sure it matters in practice (we call add_send_request() in the
loop, but I don't think it ever looks at the global objects list
itself).

This fixes several leaks noticed in t5540.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/http-push.c b/http-push.c
index 1146d7c6fe..1cddd2fb37 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1374,9 +1374,13 @@ static int get_delta(struct rev_info *revs, struct remote_lock *lock)
 	}
 
 	while (objects) {
+		struct object_list *next = objects->next;
+
 		if (!(objects->item->flags & UNINTERESTING))
 			count += add_send_request(objects->item, lock);
-		objects = objects->next;
+
+		free(objects);
+		objects = next;
 	}
 
 	return count;
-- 
2.46.2.1011.gf1f9323e02

[PATCH 27/28] http-push: clean up loose request when falling back to packed

[Jeff King]

In http-push's finish_request(), if we fail a loose object request we
may fall back to trying a packed request. But if we do so, we leave the
http_loose_object_request in place, leaking it.

We can fix this by always cleaning it up. Note that the obj_req pointer
here (which we'll set to NULL) is a copy of the request->userData
pointer, which will now point to freed memory. But that's OK. We'll
either release the parent request struct entirely, or we'll convert it
into a packed request, which will overwrite userData itself.

This leak is found by t5540, but it's not quite leak-free yet.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/http-push.c b/http-push.c
index 1cddd2fb37..b36b1f9e35 100644
--- a/http-push.c
+++ b/http-push.c
@@ -582,9 +582,10 @@ static void finish_request(struct transfer_request *request)
 			if (obj_req->rename == 0)
 				request->obj->flags |= (LOCAL | REMOTE);
 
+		release_http_object_request(&obj_req);
+
 		/* Try fetching packed if necessary */
 		if (request->obj->flags & LOCAL) {
-			release_http_object_request(&obj_req);
 			release_request(request);
 		} else
 			start_fetch_packed(request);
-- 
2.46.2.1011.gf1f9323e02

[PATCH 28/28] http-push: clean up local_refs at exit

[Jeff King]

We allocate a list of ref structs from get_local_heads() but never clean
it up. We should do so before exiting to avoid complaints from the
leak-checker. Note that we have to initialize it to NULL, because
there's one code path that can jump to the cleanup label before we
assign to it.

Fixing this lets us mark t5540 as leak-free.

Curiously building with SANITIZE=leak and gcc does not seem to find this
problem, but switching to clang does. It seems like a fairly obvious
leak, though.

I was curious that the matching remote_refs did not have the same leak.
But that is because we store the list in a global variable, so it's
still reachable after we exit. Arguably we could treat it the same as
future-proofing, but I didn't bother (now that the script is marked
leak-free, anybody moving it to a stack variable will notice).

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c                 | 3 ++-
 t/t5540-http-push-webdav.sh | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/http-push.c b/http-push.c
index b36b1f9e35..aad89f2eab 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1719,7 +1719,7 @@ int cmd_main(int argc, const char **argv)
 	int rc = 0;
 	int i;
 	int new_refs;
-	struct ref *ref, *local_refs;
+	struct ref *ref, *local_refs = NULL;
 
 	CALLOC_ARRAY(repo, 1);
 
@@ -1997,6 +1997,7 @@ int cmd_main(int argc, const char **argv)
 	}
 
 	refspec_clear(&rs);
+	free_refs(local_refs);
 
 	return rc;
 }
diff --git a/t/t5540-http-push-webdav.sh b/t/t5540-http-push-webdav.sh
index 37db3dec0c..27389b0908 100755
--- a/t/t5540-http-push-webdav.sh
+++ b/t/t5540-http-push-webdav.sh
@@ -10,6 +10,7 @@ This test runs various sanity checks on http-push.'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 if git http-push > /dev/null 2>&1 || [ $? -eq 128 ]
-- 
2.46.2.1011.gf1f9323e02

Re: [PATCH 0/28] leak fixes for http fetch/push

[Patrick Steinhardt]

On Tue, Sep 24, 2024 at 05:49:30PM -0400, Jeff King wrote:
> Patrick asked me to take a look at the remaining leaks in the http
> push/fetch code, especially the dumb variants. So here are enough
> patches to all of these scripts running leak-free:

Thank you for taking a look at this, highly appreciated! The series
looks good to me, I've only got a couple of nits regarding typos that
you may or may not want to address. Either way is fine with me.

Patrick

Re: [PATCH 0/28] leak fixes for http fetch/push

[Jeff King]

On Thu, Sep 26, 2024 at 03:50:34PM +0200, Patrick Steinhardt wrote:

> On Tue, Sep 24, 2024 at 05:49:30PM -0400, Jeff King wrote:
> > Patrick asked me to take a look at the remaining leaks in the http
> > push/fetch code, especially the dumb variants. So here are enough
> > patches to all of these scripts running leak-free:
> 
> Thank you for taking a look at this, highly appreciated! The series
> looks good to me, I've only got a couple of nits regarding typos that
> you may or may not want to address. Either way is fine with me.

I responded to a few, but I don't think any of them really merits a
re-roll. Thanks for reviewing!

-Peff