git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 2/5] transport: deep-copy object-filter struct for fetch-pack

From
Jeff King <peff@peff.net>
Date
Sep 8, 2022, 04:57 UTC
Message-ID
<Yxl2OSCCYDFqCp1/@coredump.intra.peff.net>
In-Reply-To
<Yxl1BNQoy6Drf0Oe@coredump.intra.peff.net>

When the transport code for the git protocol calls into fetch_pack(), it has to fill out a fetch_pack_args struct that is mostly taken from the transport options. We pass along any object-filter data by doing a struct assignment of the list_objects_filter_options struct. But doing so isn't safe; it contains allocated pointers in its filter_spec string_list, which could lead to a double-free if one side mutates or frees the string_list.

And indeed, the fetch-pack code does clear and rewrite the list via expand_list_objects_filter_spec(), leaving the transport code with dangling pointers.

This hasn't been a problem so far, though, because the transport code doesn't look further at the filter struct. But it should, because in some cases (when fetch-pack doesn't rewrite the list), it ends up leaking the string_list.

So let's start by turning this shallow copy into a deep one, which should let us fix the transport leak in a subsequent patch. Likewise, we'll free the deep copy we made here when we're done with it (to avoid leaking).

Note that it would also work to pass fetch-pack a pointer to our filter struct, rather than a copy. But it's awkward for fetch-pack to take a pointer in its arg struct; the actual git-fetch-pack command allocates a fetch_pack_args struct on the stack and expects it to contain the filter options. It could be rewritten to avoid this, but a deep copy serves our purposes just as well.

Signed-off-by: Jeff King <peff@peff.net>
---
I wrote up the "pass a pointer" version and it's really not too bad. But
on the other hand, the deep copy here did turn up the bug in the
previous patch (once we start freeing both copies, it caused a
double-free of the sparse_oid_name field).
 transport.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/transport.c b/transport.c
index b51e991e44..24540f642a 100644
--- a/transport.c
+++ b/transport.c
@@ -386,7 +386,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.cloning = transport->cloning;
 	args.update_shallow = data->options.update_shallow;
 	args.from_promisor = data->options.from_promisor;
-	args.filter_options = data->options.filter_options;
+	list_objects_filter_copy(&args.filter_options,
+				 &data->options.filter_options);
 	args.refetch = data->options.refetch;
 	args.stateless_rpc = transport->stateless_rpc;
 	args.server_options = transport->server_options;
@@ -453,6 +454,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 
 	free_refs(refs_tmp);
 	free_refs(refs);
+	list_objects_filter_release(&args.filter_options);
 	return ret;
 }
 
-- 
2.37.3.1139.g47294c03c7
Previous: Jeff KingNext: Jeff King
Message 3 of 8 in “plugging some list-objects-filter leaks”
  1. 0/5 plugging some list-objects-filter leaksJeff King, Sep 8, 2022
  2. 1/5 list_objects_filter_copy(): deep-copy sparse_oid_name fieldJeff King, Sep 8, 2022
  3. 2/5 transport: deep-copy object-filter struct for fetch-packJeff King, Sep 8, 2022
  4. 3/5 transport: free filter options in disconnect_git()Jeff King, Sep 8, 2022
  5. 4/5 list_objects_filter_options: plug leak of filter_spec stringsJeff King, Sep 8, 2022
  6. 5/5 prepare_repo_settings(): plug leak of config valuesJeff King, Sep 8, 2022
  7. Derrick StoleeSep 9, 2022
  8. Jeff KingSep 11, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.