{"thread":{"id":"58405","subject":"[PATCH 0/5] plugging some list-objects-filter leaks","startedAt":"2022-09-08T04:52:25Z","lastAt":"2022-09-11T04:51:19Z","messageCount":8,"participants":["Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"462788","messageId":"Yxl1BNQoy6Drf0Oe@coredump.intra.peff.net","threadId":"58405","inReplyTo":null,"subject":"[PATCH 0/5] plugging some list-objects-filter leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-08T04:52:20Z","receivedAt":"2022-09-08T04:52:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The test I sent earlier in [1] fails the linux-leaks CI job, not because\nit introduces new leaks, but just because it runs existing leaks in a\ntest marked as passing-leaks.\n\nOf course we can drop the passing flag, but I figured it would probably\nbe an easy fix. Famous last words. It turned into quite a rabbit hole of\nactual leaks (albeit small and bounded per process) and some\nquestionable memory ownership semantics.\n\nHere's the series I came up with. I'm cc-ing Stolee as the last person\nunfortunate enough to have touched this area. :)\n\n  [1/5]: list_objects_filter_copy(): deep-copy sparse_oid_name field\n  [2/5]: transport: deep-copy object-filter struct for fetch-pack\n  [3/5]: transport: free filter options in disconnect_git()\n  [4/5]: list_objects_filter_options: plug leak of filter_spec strings\n  [5/5]: prepare_repo_settings(): plug leak of config values\n\n list-objects-filter-options.c | 20 ++++++++++++++------\n repo-settings.c               |  7 +++----\n transport.c                   |  5 ++++-\n 3 files changed, 21 insertions(+), 11 deletions(-)\n\n-Peff\n\n[1] https://lore.kernel.org/git/YxkG3A30vNfu55Sx@coredump.intra.peff.net/\n"},{"id":"462790","messageId":"Yxl1hcX97W4wpWNj@coredump.intra.peff.net","threadId":"58405","inReplyTo":"Yxl1BNQoy6Drf0Oe@coredump.intra.peff.net","subject":"[PATCH 1/5] list_objects_filter_copy(): deep-copy sparse_oid_name field","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-08T04:54:29Z","receivedAt":"2022-09-08T04:54:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The purpose of our copy function is to do a deep copy of each field so\nthat the source and destination structs become independent. We correctly\ncopy the filter_spec string list, but we forgot the sparse_oid_name\nfield. By doing a shallow copy of the pointer, that puts us at risk for\na use-after-free if one or both of the structs is cleaned up.\n\nI don't think this can be triggered in practice, because we tend to leak\nthe structs rather than actually clean them up. But this should\nfuture-proof us for plugging those leaks.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think this is the only thing missing. You can correlate with the\n_release() function to see what other things might need a deep copy, and\neverything else seems covered.\n\n list-objects-filter-options.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 4b25287886..41c41c9d45 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -418,6 +418,7 @@ void list_objects_filter_copy(\n \tstring_list_init_dup(&dest->filter_spec);\n \tfor_each_string_list_item(item, &src->filter_spec)\n \t\tstring_list_append(&dest->filter_spec, item->string);\n+\tdest->sparse_oid_name = xstrdup_or_null(src->sparse_oid_name);\n \n \tALLOC_ARRAY(dest->sub, dest->sub_alloc);\n \tfor (i = 0; i < src->sub_nr; i++)\n-- \n2.37.3.1139.g47294c03c7\n\n"},{"id":"462791","messageId":"Yxl2OSCCYDFqCp1/@coredump.intra.peff.net","threadId":"58405","inReplyTo":"Yxl1BNQoy6Drf0Oe@coredump.intra.peff.net","subject":"[PATCH 2/5] transport: deep-copy object-filter struct for fetch-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-08T04:57:29Z","receivedAt":"2022-09-08T04:57:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the transport code for the git protocol calls into fetch_pack(), it\nhas to fill out a fetch_pack_args struct that is mostly taken from the\ntransport options. We pass along any object-filter data by doing a\nstruct assignment of the list_objects_filter_options struct. But doing\nso isn't safe; it contains allocated pointers in its filter_spec\nstring_list, which could lead to a double-free if one side mutates or\nfrees the string_list.\n\nAnd indeed, the fetch-pack code does clear and rewrite the list via\nexpand_list_objects_filter_spec(), leaving the transport code with\ndangling pointers.\n\nThis hasn't been a problem so far, though, because the transport code\ndoesn't look further at the filter struct. But it should, because in\nsome cases (when fetch-pack doesn't rewrite the list), it ends up\nleaking the string_list.\n\nSo let's start by turning this shallow copy into a deep one, which\nshould let us fix the transport leak in a subsequent patch. Likewise,\nwe'll free the deep copy we made here when we're done with it (to avoid\nleaking).\n\nNote that it would also work to pass fetch-pack a pointer to our filter\nstruct, rather than a copy. But it's awkward for fetch-pack to take a\npointer in its arg struct; the actual git-fetch-pack command allocates a\nfetch_pack_args struct on the stack and expects it to contain the filter\noptions. It could be rewritten to avoid this, but a deep copy serves our\npurposes just as well.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI wrote up the \"pass a pointer\" version and it's really not too bad. But\non the other hand, the deep copy here did turn up the bug in the\nprevious patch (once we start freeing both copies, it caused a\ndouble-free of the sparse_oid_name field).\n\n transport.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/transport.c b/transport.c\nindex b51e991e44..24540f642a 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -386,7 +386,8 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.cloning = transport->cloning;\n \targs.update_shallow = data->options.update_shallow;\n \targs.from_promisor = data->options.from_promisor;\n-\targs.filter_options = data->options.filter_options;\n+\tlist_objects_filter_copy(&args.filter_options,\n+\t\t\t\t &data->options.filter_options);\n \targs.refetch = data->options.refetch;\n \targs.stateless_rpc = transport->stateless_rpc;\n \targs.server_options = transport->server_options;\n@@ -453,6 +454,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \n \tfree_refs(refs_tmp);\n \tfree_refs(refs);\n+\tlist_objects_filter_release(&args.filter_options);\n \treturn ret;\n }\n \n-- \n2.37.3.1139.g47294c03c7\n\n"},{"id":"462792","messageId":"Yxl2Yz8bg9T0dBGo@coredump.intra.peff.net","threadId":"58405","inReplyTo":"Yxl1BNQoy6Drf0Oe@coredump.intra.peff.net","subject":"[PATCH 3/5] transport: free filter options in disconnect_git()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-08T04:58:11Z","receivedAt":"2022-09-08T04:58:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a user of the transport API calls transport_set_option() with\nTRANS_OPT_LIST_OBJECTS_FILTER, it doesn't pass a struct, but rather a\nstring with the filter-spec, which the transport code then stores in its\nown list_objects_filter_options struct.\n\nWhen the caller is done and we call transport_disconnect(), the contents\nof that filter struct are then leaked. We should release it before\nfreeing the transport struct.\n\nAnother way to solve this would be for transport_set_option() to pass a\npointer to the struct. But that's awkward, because there's a generic\ntransport-option interface that always takes a string. Plus it opens up\nquestions of memory lifetimes; by storing its own filter-options struct,\nthe transport code remains self-contained.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n transport.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/transport.c b/transport.c\nindex 24540f642a..6ec6130852 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -895,6 +895,7 @@ static int disconnect_git(struct transport *transport)\n \t\tfinish_connect(data->conn);\n \t}\n \n+\tlist_objects_filter_release(&data->options.filter_options);\n \tfree(data);\n \treturn 0;\n }\n-- \n2.37.3.1139.g47294c03c7\n\n"},{"id":"462793","messageId":"Yxl3I+3G15fDv0CG@coredump.intra.peff.net","threadId":"58405","inReplyTo":"Yxl1BNQoy6Drf0Oe@coredump.intra.peff.net","subject":"[PATCH 4/5] list_objects_filter_options: plug leak of filter_spec strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-08T05:01:23Z","receivedAt":"2022-09-08T05:01:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The list_objects_filter_options struct contains a string_list to store\nthe filter_spec. Because we allow the options struct to be\nzero-initialized by callers, the string_list's strdup_strings field is\ngenerally not set.\n\nBecause we don't want to depend on the memory lifetimes of any strings\npassed in to the list-objects API, everything we add to the string_list\nis duplicated (either via xstrdup(), or via strbuf_detach()). So far so\ngood, but now we have a problem at cleanup time: when we clear the\nlist, the string_list API doesn't realize that it needs to free all of\nthose strings, and we leak them.\n\nOne option would be to set strdup_strings right before clearing the\nlist. But this is tricky for two reasons:\n\n  1. There's one spot, in partial_clone_get_default_filter_spec(),\n     that fails to duplicate its argument. We could fix that, but...\n\n  2. We clear the list in a surprising number of places. As you might\n     expect, we do so in list_objects_filter_release(). But we also\n     clear and rewrite it in expand_list_objects_filter_spec(),\n     list_objects_filter_spec(), and transform_to_combine_type().\n     We'd have to put the same hack in all of those spots.\n\nInstead, let's just set strdup_strings before adding anything. That lets\nus drop the extra manual xstrdup() calls, fixes the spot mentioned\nin (1) above that _should_ be duplicating, and future-proofs further\ncalls. We do have to switch the strbuf_detach() calls to use the nodup\nform, but that's an easy change, and the resulting code more clearly\nshows the expected ownership transfer.\n\nThis also resolves a weird inconsistency: when we make a deep copy with\nlist_objects_filter_copy(), it initializes the copy's filter_spec with\nstring_list_init_dup(). So the copy frees its string_list memory\ncorrectly, but accidentally leaks the extra manual-xstrdup()'d strings!\n\nThere is one hiccup, though. In an ideal world, everyone would allocate\nthe list_objects_filter_options struct with an initializer which used\nSTRING_LIST_INIT_DUP under the hood. But there are a bunch of existing\ncallers which think that zero-initializing is good enough. We can leave\nthem as-is by noting that the list is always initially populated via\nparse_list_objects_filter(). So we can just initialize the\nstrdup_strings flag there.\n\nThis is arguably a band-aid, but it works reliably. And it doesn't make\nanything harder if we want to switch all the callers later to a new\nLIST_OBJECTS_FILTER_INIT.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n list-objects-filter-options.c | 19 +++++++++++++------\n 1 file changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 41c41c9d45..6cc4eb8e1c 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -207,7 +207,7 @@ static void filter_spec_append_urlencode(\n \tstruct strbuf buf = STRBUF_INIT;\n \tstrbuf_addstr_urlencode(&buf, raw, allow_unencoded);\n \ttrace_printf(\"Add to combine filter-spec: %s\\n\", buf.buf);\n-\tstring_list_append(&filter->filter_spec, strbuf_detach(&buf, NULL));\n+\tstring_list_append_nodup(&filter->filter_spec, strbuf_detach(&buf, NULL));\n }\n \n /*\n@@ -226,12 +226,13 @@ static void transform_to_combine_type(\n \t\t\txcalloc(initial_sub_alloc, sizeof(*sub_array));\n \t\tsub_array[0] = *filter_options;\n \t\tmemset(filter_options, 0, sizeof(*filter_options));\n+\t\tstring_list_init_dup(&filter_options->filter_spec);\n \t\tfilter_options->sub = sub_array;\n \t\tfilter_options->sub_alloc = initial_sub_alloc;\n \t}\n \tfilter_options->sub_nr = 1;\n \tfilter_options->choice = LOFC_COMBINE;\n-\tstring_list_append(&filter_options->filter_spec, xstrdup(\"combine:\"));\n+\tstring_list_append(&filter_options->filter_spec, \"combine:\");\n \tfilter_spec_append_urlencode(\n \t\tfilter_options,\n \t\tlist_objects_filter_spec(&filter_options->sub[0]));\n@@ -256,8 +257,14 @@ void parse_list_objects_filter(\n \tstruct strbuf errbuf = STRBUF_INIT;\n \tint parse_error;\n \n+\tif (!filter_options->filter_spec.strdup_strings) {\n+\t\tif (filter_options->filter_spec.nr)\n+\t\t\tBUG(\"unexpected non-allocated string in filter_spec\");\n+\t\tfilter_options->filter_spec.strdup_strings = 1;\n+\t}\n+\n \tif (!filter_options->choice) {\n-\t\tstring_list_append(&filter_options->filter_spec, xstrdup(arg));\n+\t\tstring_list_append(&filter_options->filter_spec, arg);\n \n \t\tparse_error = gently_parse_list_objects_filter(\n \t\t\tfilter_options, arg, &errbuf);\n@@ -268,7 +275,7 @@ void parse_list_objects_filter(\n \t\t */\n \t\ttransform_to_combine_type(filter_options);\n \n-\t\tstring_list_append(&filter_options->filter_spec, xstrdup(\"+\"));\n+\t\tstring_list_append(&filter_options->filter_spec, \"+\");\n \t\tfilter_spec_append_urlencode(filter_options, arg);\n \t\tALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,\n \t\t\t      filter_options->sub_alloc);\n@@ -306,7 +313,7 @@ const char *list_objects_filter_spec(struct list_objects_filter_options *filter)\n \t\tstrbuf_add_separated_string_list(\n \t\t\t&concatted, \"\", &filter->filter_spec);\n \t\tstring_list_clear(&filter->filter_spec, /*free_util=*/0);\n-\t\tstring_list_append(\n+\t\tstring_list_append_nodup(\n \t\t\t&filter->filter_spec, strbuf_detach(&concatted, NULL));\n \t}\n \n@@ -321,7 +328,7 @@ const char *expand_list_objects_filter_spec(\n \t\tstrbuf_addf(&expanded_spec, \"blob:limit=%lu\",\n \t\t\t    filter->blob_limit_value);\n \t\tstring_list_clear(&filter->filter_spec, /*free_util=*/0);\n-\t\tstring_list_append(\n+\t\tstring_list_append_nodup(\n \t\t\t&filter->filter_spec,\n \t\t\tstrbuf_detach(&expanded_spec, NULL));\n \t}\n-- \n2.37.3.1139.g47294c03c7\n\n"},{"id":"462794","messageId":"Yxl3elDV2kpAfAvU@coredump.intra.peff.net","threadId":"58405","inReplyTo":"Yxl1BNQoy6Drf0Oe@coredump.intra.peff.net","subject":"[PATCH 5/5] prepare_repo_settings(): plug leak of config values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-08T05:02:50Z","receivedAt":"2022-09-08T05:02:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We call repo_config_get_string() for fetch.negotiationAlgorithm, which\nallocates a copy of the string, but we never free it.\n\nWe could add a call to free(), but there's an even simpler solution: we\ndon't need the string to persist beyond a few strcasecmp() calls, so we\ncan instead use the \"_tmp\" variant which gives us a const pointer to the\ncached value.\n\nWe need to switch the type of \"strval\" to \"const char *\" for this to\nwork, which affects a similar call that checks core.untrackedCache. But\nit's in the same boat! It doesn't actually need the value to persist\nbeyond a maybe_bool() check (though it does remember to correctly free\nthe string afterwards). So we can simplify it at the same time.\n\nNote that this core.untrackedCache check arguably should be using\nrepo_config_get_maybe_bool(), but there are some subtle behavior\nchanges. E.g., it doesn't currently allow a value-less \"true\". Arguably\nit should, but let's avoid lumping further changes in what should be a\nsimple leak cleanup.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n repo-settings.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 43bc881bfc..e8b58151bc 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -22,7 +22,7 @@ void prepare_repo_settings(struct repository *r)\n {\n \tint experimental;\n \tint value;\n-\tchar *strval;\n+\tconst char *strval;\n \tint manyfiles;\n \n \tif (!r->gitdir)\n@@ -77,7 +77,7 @@ void prepare_repo_settings(struct repository *r)\n \tif (!repo_config_get_int(r, \"index.version\", &value))\n \t\tr->settings.index_version = value;\n \n-\tif (!repo_config_get_string(r, \"core.untrackedcache\", &strval)) {\n+\tif (!repo_config_get_string_tmp(r, \"core.untrackedcache\", &strval)) {\n \t\tint v = git_parse_maybe_bool(strval);\n \n \t\t/*\n@@ -88,10 +88,9 @@ void prepare_repo_settings(struct repository *r)\n \t\tif (v >= 0)\n \t\t\tr->settings.core_untracked_cache = v ?\n \t\t\t\tUNTRACKED_CACHE_WRITE : UNTRACKED_CACHE_REMOVE;\n-\t\tfree(strval);\n \t}\n \n-\tif (!repo_config_get_string(r, \"fetch.negotiationalgorithm\", &strval)) {\n+\tif (!repo_config_get_string_tmp(r, \"fetch.negotiationalgorithm\", &strval)) {\n \t\tint fetch_default = r->settings.fetch_negotiation_algorithm;\n \t\tif (!strcasecmp(strval, \"skipping\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n-- \n2.37.3.1139.g47294c03c7\n"},{"id":"462851","messageId":"15688f6a-4757-e201-9106-663899e35fec@github.com","threadId":"58405","inReplyTo":"Yxl1BNQoy6Drf0Oe@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] plugging some list-objects-filter leaks","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-09T14:20:37Z","receivedAt":"2022-09-09T14:20:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/8/2022 12:52 AM, Jeff King wrote:\n> The test I sent earlier in [1] fails the linux-leaks CI job, not because\n> it introduces new leaks, but just because it runs existing leaks in a\n> test marked as passing-leaks.\n> \n> Of course we can drop the passing flag, but I figured it would probably\n> be an easy fix. Famous last words. It turned into quite a rabbit hole of\n> actual leaks (albeit small and bounded per process) and some\n> questionable memory ownership semantics.\n\nReading the patches, you make good arguments about the various trade-\noffs in these sticky places. I agree with you in all cases, mostly\nbecause the alternatives would not be any better unless we did a _lot_\nof work to rewrite a lot more code than these patches. Even then, the\nbenefit is unclear.\n\n> Here's the series I came up with. I'm cc-ing Stolee as the last person\n> unfortunate enough to have touched this area. :)\n\nLucky me!\n\nThese patches look good. Thanks!\n\n-Stolee\n"},{"id":"462902","messageId":"Yx1pQKfmVZegmYzM@coredump.intra.peff.net","threadId":"58405","inReplyTo":"15688f6a-4757-e201-9106-663899e35fec@github.com","subject":"Re: [PATCH 0/5] plugging some list-objects-filter leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-11T04:51:12Z","receivedAt":"2022-09-11T04:51:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 09, 2022 at 10:20:37AM -0400, Derrick Stolee wrote:\n\n> > Of course we can drop the passing flag, but I figured it would probably\n> > be an easy fix. Famous last words. It turned into quite a rabbit hole of\n> > actual leaks (albeit small and bounded per process) and some\n> > questionable memory ownership semantics.\n> \n> Reading the patches, you make good arguments about the various trade-\n> offs in these sticky places. I agree with you in all cases, mostly\n> because the alternatives would not be any better unless we did a _lot_\n> of work to rewrite a lot more code than these patches. Even then, the\n> benefit is unclear.\n\nThanks for looking them over. I wondered if I was just being lazy and/or\ncowardly in not teaching callers to initialize the filter struct\nspecifically (rather than memset to zero).\n\nSo of course I started looking at it, and of course it was way more\ncomplicated than you'd imagine. So I have no regrets in drawing the line\nwhere I did in this leak-plugging series.\n\nBut now that I did do it, it turned out to reveal other oddities in the\nfilter code! So I'll post that as a separate series on top. Even if we\ndon't go all the way with the cleanup, we should at least fix the\noddities.\n\n> > Here's the series I came up with. I'm cc-ing Stolee as the last person\n> > unfortunate enough to have touched this area. :)\n> \n> Lucky me!\n\nIt's the gift that keeps on giving. Guess what you're about to get cc'd\non. ;)\n\n-Peff\n"}]}