patch, 5 partsplugging some list-objects-filter leaks

The test I sent earlier in [1] fails the linux-leaks CI job, not because it introduces new leaks, but just because it runs existing leaks in a test marked as passing-leaks.

Of course we can drop the passing flag, but I figured it would probably be an easy fix. Famous last words. It turned into quite a rabbit hole of actual leaks (albeit small and bounded per process) and some questionable memory ownership semantics.

Here's the series I came up with. I'm cc-ing Stolee as the last person unfortunate enough to have touched this area. :)

  [1/5]: list_objects_filter_copy(): deep-copy sparse_oid_name field
  [2/5]: transport: deep-copy object-filter struct for fetch-pack
  [3/5]: transport: free filter options in disconnect_git()
  [4/5]: list_objects_filter_options: plug leak of filter_spec strings
  [5/5]: prepare_repo_settings(): plug leak of config values

 list-objects-filter-options.c | 20 ++++++++++++++------
 repo-settings.c               |  7 +++----
 transport.c                   |  5 ++++-
 3 files changed, 21 insertions(+), 11 deletions(-)

-Peff

[1] https://lore.kernel.org/git/YxkG3A30vNfu55Sx@coredump.intra.peff.net/

The purpose of our copy function is to do a deep copy of each field so that the source and destination structs become independent. We correctly copy the filter_spec string list, but we forgot the sparse_oid_name field. By doing a shallow copy of the pointer, that puts us at risk for a use-after-free if one or both of the structs is cleaned up.

I don't think this can be triggered in practice, because we tend to leak the structs rather than actually clean them up. But this should future-proof us for plugging those leaks.

Signed-off-by: Jeff King <peff@peff.net>
---
I think this is the only thing missing. You can correlate with the
_release() function to see what other things might need a deep copy, and
everything else seems covered.

 list-objects-filter-options.c | 1 +
 1 file changed, 1 insertion(+)

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(-)

If a user of the transport API calls transport_set_option() with TRANS_OPT_LIST_OBJECTS_FILTER, it doesn't pass a struct, but rather a string with the filter-spec, which the transport code then stores in its own list_objects_filter_options struct.

When the caller is done and we call transport_disconnect(), the contents of that filter struct are then leaked. We should release it before freeing the transport struct.

Another way to solve this would be for transport_set_option() to pass a pointer to the struct. But that's awkward, because there's a generic transport-option interface that always takes a string. Plus it opens up questions of memory lifetimes; by storing its own filter-options struct, the transport code remains self-contained.

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

The list_objects_filter_options struct contains a string_list to store the filter_spec. Because we allow the options struct to be zero-initialized by callers, the string_list's strdup_strings field is generally not set.

Because we don't want to depend on the memory lifetimes of any strings passed in to the list-objects API, everything we add to the string_list is duplicated (either via xstrdup(), or via strbuf_detach()). So far so good, but now we have a problem at cleanup time: when we clear the list, the string_list API doesn't realize that it needs to free all of those strings, and we leak them.

One option would be to set strdup_strings right before clearing the list. But this is tricky for two reasons:

  1. There's one spot, in partial_clone_get_default_filter_spec(),
     that fails to duplicate its argument. We could fix that, but...

  2. We clear the list in a surprising number of places. As you might
     expect, we do so in list_objects_filter_release(). But we also
     clear and rewrite it in expand_list_objects_filter_spec(),
     list_objects_filter_spec(), and transform_to_combine_type().
     We'd have to put the same hack in all of those spots.

Instead, let's just set strdup_strings before adding anything. That lets us drop the extra manual xstrdup() calls, fixes the spot mentioned in (1) above that _should_ be duplicating, and future-proofs further calls. We do have to switch the strbuf_detach() calls to use the nodup form, but that's an easy change, and the resulting code more clearly shows the expected ownership transfer.

This also resolves a weird inconsistency: when we make a deep copy with list_objects_filter_copy(), it initializes the copy's filter_spec with string_list_init_dup(). So the copy frees its string_list memory correctly, but accidentally leaks the extra manual-xstrdup()'d strings!

There is one hiccup, though. In an ideal world, everyone would allocate the list_objects_filter_options struct with an initializer which used STRING_LIST_INIT_DUP under the hood. But there are a bunch of existing callers which think that zero-initializing is good enough. We can leave them as-is by noting that the list is always initially populated via parse_list_objects_filter(). So we can just initialize the strdup_strings flag there.

This is arguably a band-aid, but it works reliably. And it doesn't make anything harder if we want to switch all the callers later to a new LIST_OBJECTS_FILTER_INIT.

Signed-off-by: Jeff King <peff@peff.net>
---
 list-objects-filter-options.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

We call repo_config_get_string() for fetch.negotiationAlgorithm, which allocates a copy of the string, but we never free it.

We could add a call to free(), but there's an even simpler solution: we don't need the string to persist beyond a few strcasecmp() calls, so we can instead use the "_tmp" variant which gives us a const pointer to the cached value.

We need to switch the type of "strval" to "const char *" for this to work, which affects a similar call that checks core.untrackedCache. But it's in the same boat! It doesn't actually need the value to persist beyond a maybe_bool() check (though it does remember to correctly free the string afterwards). So we can simplify it at the same time.

Note that this core.untrackedCache check arguably should be using repo_config_get_maybe_bool(), but there are some subtle behavior changes. E.g., it doesn't currently allow a value-less "true". Arguably it should, but let's avoid lumping further changes in what should be a simple leak cleanup.

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

On 9/8/2022 12:52 AM, Jeff King wrote:

Reading the patches, you make good arguments about the various trade- offs in these sticky places. I agree with you in all cases, mostly because the alternatives would not be any better unless we did a _lot_ of work to rewrite a lot more code than these patches. Even then, the benefit is unclear.

Lucky me!

These patches look good. Thanks!

-Stolee

On Fri, Sep 09, 2022 at 10:20:37AM -0400, Derrick Stolee wrote:

Thanks for looking them over. I wondered if I was just being lazy and/or cowardly in not teaching callers to initialize the filter struct specifically (rather than memset to zero).

So of course I started looking at it, and of course it was way more complicated than you'd imagine. So I have no regrets in drawing the line where I did in this leak-plugging series.

But now that I did do it, it turned out to reveal other oddities in the filter code! So I'll post that as a separate series on top. Even if we don't go all the way with the cleanup, we should at least fix the oddities.

It's the gift that keeps on giving. Guess what you're about to get cc'd on. ;)

-Peff