{"thread":{"id":"54754","subject":"[PATCH 0/2] upload-pack.c: fix partial clone allowed filter regression","startedAt":"2020-12-03T18:56:05Z","lastAt":"2020-12-04T21:04:48Z","messageCount":4,"participants":["Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"411268","messageId":"cover.1607021483.git.me@ttaylorr.com","threadId":"54754","inReplyTo":null,"subject":"[PATCH 0/2] upload-pack.c: fix partial clone allowed filter regression","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-12-03T18:55:03Z","receivedAt":"2020-12-03T18:56:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nHere's a regression fix that I noticed this morning while deploying 2.28\nout to GitHub. The gist is that we accidentally call\n'string_list_clear(..., 1)' when freeing the list of allowed/banned\nobject filters, but the ->util pointer either contains '0', or '1'. So,\nthe free() of the util pointer is bogus, and we end up crashing.\n\nThe fix in the second patch is straightforward, but it doesn't work\nwithout the first patch, which teaches 'git clone' to match 'git fetch'\nand not ignore an error from 'transport_fetch_refs()'.\n\nUnfortunately, this regression has been in Git since 2.28. Fortunately,\nnobody has seemed to notice, so I doubt that it's bitten anybody out in\nthe wild. But, it's still worth fixing, since it is so obviously broken.\n\nThanks,\nTaylor\n\nTaylor Blau (2):\n builtin/clone.c: don't ignore transport_fetch_refs() errors\n upload-pack.c: don't free allowed_filters util pointers\n\n builtin/clone.c | 15 +++++++++++----\n t/t5616-partial-clone.sh | 10 +++++++++-\n upload-pack.c | 2 +-\n 3 files changed, 21 insertions(+), 6 deletions(-)\n\n--\n2.29.2.533.g07db1f5344\n"},{"id":"411269","messageId":"a3c721054f337eac2b3a038e51693f44b68dd619.1607021483.git.me@ttaylorr.com","threadId":"54754","inReplyTo":"cover.1607021483.git.me@ttaylorr.com","subject":"[PATCH 2/2] upload-pack.c: don't free allowed_filters util pointers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-12-03T18:55:18Z","receivedAt":"2020-12-03T18:56:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To keep track of which object filters are allowed or not, 'git\nupload-pack' stores the name of each filter in a string_list, and sets\nit ->util pointer to be either 0 or 1, indicating whether it is banned\nor allowed.\n\nLater on, we attempt to clear that list, but we incorrectly ask for the\nutil pointers to be free()'d, too. This behavior (introduced back in\n6dd3456a8c (upload-pack.c: allow banning certain object filter(s),\n2020-08-03)) leads to an invalid free, and causes us to crash.\n\nIn order to trigger this, one needs to fetch from a server that (a) has\nat least one object filter allowed, and (b) issue a fetch that contains\na subset of the allowed filters (i.e., we cannot ask for a banned\nfilter, since this causes us to die() before we hit the bogus\nstring_list_clear()).\n\nIn that case, whatever banned filters exist will cause a noop free()\n(since those ->util pointers are set to 0), but the first allowed filter\nwe try to free will crash us.\n\nWe never noticed this in the tests because we didn't have an example of\nsetting 'uploadPackFilter' configuration variables and then following up\nwith a valid fetch. The first new 'git clone' prevents further\nregression here. For good measure on top, add a test which checks the\nsame behavior at a tree depth greater than 0.\n\nSigned-off-by: Taylor Blau \n---\n t/t5616-partial-clone.sh | 10 +++++++++-\n upload-pack.c | 2 +-\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex f4d49d8335..5da945cf15 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -281,7 +281,15 @@ test_expect_success 'upload-pack limits tree depth filters' '\n \ttest_config -C srv.bare uploadpackfilter.tree.maxDepth 0 &&\n \ttest_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \\\n \t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n-\ttest_i18ngrep \"tree filter allows max depth 0, but got 1\" err\n+\ttest_i18ngrep \"tree filter allows max depth 0, but got 1\" err &&\n+\n+\tgit clone --no-checkout --filter=tree:0 \"file://$(pwd)/srv.bare\" pc4 &&\n+\n+\ttest_config -C srv.bare uploadpackfilter.tree.maxDepth 5 &&\n+\tgit clone --no-checkout --filter=tree:5 \"file://$(pwd)/srv.bare\" pc5 &&\n+\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=tree:6 \\\n+\t\t\"file://$(pwd)/srv.bare\" pc6 2>err &&\n+\ttest_i18ngrep \"tree filter allows max depth 5, but got 6\" err\n '\n \n test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 5dc8e1f844..d89c7e4a02 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -154,7 +154,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \tstring_list_clear(&data->deepen_not, 0);\n \tobject_array_clear(&data->extra_edge_obj);\n \tlist_objects_filter_release(&data->filter_options);\n-\tstring_list_clear(&data->allowed_filters, 1);\n+\tstring_list_clear(&data->allowed_filters, 0);\n \n \tfree((char *)data->pack_objects_hook);\n }\n-- \n2.29.2.533.g07db1f5344\n"},{"id":"411270","messageId":"27a817597850a2e6a13efe1d5ea105d70e1e4040.1607021483.git.me@ttaylorr.com","threadId":"54754","inReplyTo":"cover.1607021483.git.me@ttaylorr.com","subject":"[PATCH 1/2] builtin/clone.c: don't ignore transport_fetch_refs() errors","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-12-03T18:55:13Z","receivedAt":"2020-12-03T18:56:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"If 'git clone' couldn't execute 'transport_fetch_refs()' (e.g., because\nof an error on the remote's side in 'git upload-pack'), then it will\nsilently ignore it.\n\nEven though this has been the case at least since clone was ported to C\n(way back in 8434c2f1af (Build in clone, 2008-04-27)), 'git fetch'\ndoesn't ignore these and reports any failures it sees.\n\nThat suggests that ignoring the return value in 'git clone' is simply an\noversight that should be corrected. That's exactly what this patch does.\n(Noticing and fixing this is no coincidence, we'll want it in the next\npatch in order to demonstrate a regression in 'git upload-pack' via a\n'git clone'.)\n\nThere's no additional logging here, but that matches how 'git fetch'\nhandles the same case. An assumption there is that whichever part of\ntransport_fetch_refs() fails will complain loudly, so any additional\nlogging here is redundant.\n\nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n builtin/clone.c | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a0841923cf..a5630337e4 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1293,8 +1293,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t\tbreak;\n \t\t\t}\n \n-\t\tif (!is_local && !complete_refs_before_fetch)\n-\t\t\ttransport_fetch_refs(transport, mapped_refs);\n+\t\tif (!is_local && !complete_refs_before_fetch) {\n+\t\t\terr = transport_fetch_refs(transport, mapped_refs);\n+\t\t\tif (err)\n+\t\t\t\tgoto cleanup;\n+\t\t}\n \n \t\tremote_head = find_ref_by_name(refs, \"HEAD\");\n \t\tremote_head_points_at =\n@@ -1339,8 +1342,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tif (is_local)\n \t\tclone_local(path, git_dir);\n-\telse if (refs && complete_refs_before_fetch)\n-\t\ttransport_fetch_refs(transport, mapped_refs);\n+\telse if (refs && complete_refs_before_fetch) {\n+\t\terr = transport_fetch_refs(transport, mapped_refs);\n+\t\tif (err)\n+\t\t\tgoto cleanup;\n+\t}\n \n \tupdate_remote_refs(refs, mapped_refs, remote_head_points_at,\n \t\t\t branch_top.buf, reflog_msg.buf, transport,\n@@ -1367,6 +1373,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tjunk_mode = JUNK_LEAVE_REPO;\n \terr = checkout(submodule_progress);\n \n+cleanup:\n \tfree(remote_name);\n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\n-- \n2.29.2.533.g07db1f5344\n\n"},{"id":"411403","messageId":"X8qkRRrWCbDn1MAu@coredump.intra.peff.net","threadId":"54754","inReplyTo":"a3c721054f337eac2b3a038e51693f44b68dd619.1607021483.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] upload-pack.c: don't free allowed_filters util pointers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-04T21:04:05Z","receivedAt":"2020-12-04T21:04:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 03, 2020 at 01:55:18PM -0500, Taylor Blau wrote:\n\n> We never noticed this in the tests because we didn't have an example of\n> setting 'uploadPackFilter' configuration variables and then following up\n> with a valid fetch. The first new 'git clone' prevents further\n> regression here. For good measure on top, add a test which checks the\n> same behavior at a tree depth greater than 0.\n\nVery nice. The fix is obviously the right thing, but I like the\nimprovements to the tests here especially.\n\n-Peff\n"}]}