{"thread":{"id":"61664","subject":"[PATCH] bundle-uri.c: Fix double increment in depth","startedAt":"2024-06-21T09:23:19Z","lastAt":"2024-06-21T17:18:09Z","messageCount":4,"participants":["Toon Claes","Christian Couder","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"497472","messageId":"20240621092258.1557258-1-toon@iotcl.com","threadId":"61664","inReplyTo":null,"subject":"[PATCH] bundle-uri.c: Fix double increment in depth","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-06-21T09:22:58Z","receivedAt":"2024-06-21T09:23:19Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"A bundle URI can serve a gitformat-bundle(5) or a bundle list. This\nplain text file is in the Git config format containing other bundle\nURIs. To avoid these bundle lists to nest too deep, we've set a limit\nwith `max_bundle_uri_depth`. Although, when walk through the tree of\nbundles, the current depth is incremented in download_bundle_list() and\nthen calls download_bundle_to_file(), which also increments the depth.\nRemove the increment in download_bundle_to_file().\n\nSigned-off-by: Toon Claes <toon@iotcl.com>\n---\n bundle-uri.c                |  2 +-\n t/t5558-clone-bundle-uri.sh | 62 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 1 deletion(-)\n\ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex 91b3319a5c..7b1a711919 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -436,7 +436,7 @@ static int download_bundle_to_file(struct remote_bundle_info *bundle, void *data\n \tif (ctx->mode == BUNDLE_MODE_ANY && ctx->count)\n \t\treturn 0;\n\n-\tres = fetch_bundle_uri_internal(ctx->r, bundle, ctx->depth + 1, ctx->list);\n+\tres = fetch_bundle_uri_internal(ctx->r, bundle, ctx->depth, ctx->list);\n\n \t/*\n \t * Only increment count if the download succeeded. If our mode is\ndiff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh\nindex 1ca5f745e7..f3a8494297 100755\n--- a/t/t5558-clone-bundle-uri.sh\n+++ b/t/t5558-clone-bundle-uri.sh\n@@ -259,6 +259,68 @@ test_expect_success 'clone bundle list (file, any mode, all failures)' '\n \t! grep \"refs/bundles/\" refs\n '\n\n+test_expect_success 'clone bundle list (file, above max depth)' '\n+\tcat >bundle-list-1 <<-EOF &&\n+\t[bundle]\n+\t\tversion = 1\n+\t\tmode = any\n+\n+\t[bundle \"bundle-list-2\"]\n+\t\turi = file://$(pwd)/bundle-list-2\n+\tEOF\n+\n+\tcat >bundle-list-2 <<-EOF &&\n+\t[bundle]\n+\t\tversion = 1\n+\t\tmode = any\n+\n+\t[bundle \"bundle-list-3\"]\n+\t\turi = file://$(pwd)/bundle-list-3\n+\tEOF\n+\n+\tcat >bundle-list-3 <<-EOF &&\n+\t[bundle]\n+\t\tversion = 1\n+\t\tmode = any\n+\n+\t[bundle \"bundle-list-4\"]\n+\t\turi = file://$(pwd)/bundle-list-4\n+\tEOF\n+\n+\tcat >bundle-list-4 <<-EOF &&\n+\t[bundle]\n+\t\tversion = 1\n+\t\tmode = any\n+\n+\t[bundle \"bundle-0\"]\n+\t\turi = file://$(pwd)/clone-from/bundle-0.bundle\n+\tEOF\n+\n+\tgit clone --bundle-uri=\"file://$(pwd)/bundle-list-1\" \\\n+\t\tclone-from clone-too-deep 2>err &&\n+\t! grep \"fatal\" err &&\n+\tgrep \"warning: exceeded bundle URI recursion limit\" err &&\n+\n+\tgit -C clone-from for-each-ref --format=\"%(objectname)\" >oids &&\n+\tgit -C clone-too-deep cat-file --batch-check <oids &&\n+\n+\tgit -C clone-too-deep for-each-ref --format=\"%(refname)\" >refs &&\n+\t! grep \"refs/bundles/\" refs\n+'\n+\n+test_expect_success 'clone bundle list (file, below max depth)' '\n+\tgit clone --bundle-uri=\"file://$(pwd)/bundle-list-2\" \\\n+\t\tclone-from clone-max-depth 2>err &&\n+\t! grep \"fatal\" err &&\n+\t! grep \"warning: exceeded bundle URI recursion limit\" err &&\n+\n+\tgit -C clone-from for-each-ref --format=\"%(objectname)\" >oids &&\n+\tgit -C clone-max-depth cat-file --batch-check <oids &&\n+\n+\tgit -C clone-max-depth for-each-ref --format=\"%(refname)\" >refs &&\n+\t! grep \"refs/bundles/\" refs\n+'\n+\n #########################################################################\n # HTTP tests begin here\n\n--\n2.45.0\n"},{"id":"497495","messageId":"CAP8UFD3jGVaO2JdKE0H22mf4E8YBxrEwuGqJuyeRGBVh==23NQ@mail.gmail.com","threadId":"61664","inReplyTo":"20240621092258.1557258-1-toon@iotcl.com","subject":"Re: [PATCH] bundle-uri.c: Fix double increment in depth","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-06-21T15:24:35Z","receivedAt":"2024-06-21T15:24:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 21, 2024 at 11:23 AM Toon Claes <toon@iotcl.com> wrote:\n>\n> A bundle URI can serve a gitformat-bundle(5) or a bundle list. This\n> plain text file is in the Git config format containing other bundle\n> URIs. To avoid these bundle lists to nest too deep, we've set a limit\n> with `max_bundle_uri_depth`.\n\nYeah, max_bundle_uri_depth seems to be hardcoded to 4.\n\n> Although, when walk through the tree of\n\ns/walk/walking/\n\n> bundles, the current depth is incremented in download_bundle_list() and\n> then calls download_bundle_to_file(), which also increments the depth.\n\ns/and then calls/which then calls/\n\n> Remove the increment in download_bundle_to_file().\n\nThe increment is removed by replacing:\n\n    fetch_bundle_uri_internal( ..., ctx->depth + 1, ...)\n\nwith:\n\n    fetch_bundle_uri_internal( ..., ctx->depth, ...)\n\nin download_bundle_to_file(). Ok.\n\nIt looks like there is another similar call to that function like this:\n\nfetch_bundle_uri_internal( ... , ctx.depth + 1, ... )\n\nin fetch_bundles_by_token() though.\n\nThere ctx.depth is initialized to 0 before the call, so it looks like\nit could work, but fetch_bundle_uri_internal() can call\nfetch_bundle_list_in_config_format() which can call\ndownload_bundle_list() which, as we saw above, still increases the\ndepth by 1.\n\nSo even if download_bundle_list() then calls download_bundle_to_file()\nwithout increasing the depth, I am not sure it works well in all\ncases. At least I think a bit more explanations might be needed.\n\n> +test_expect_success 'clone bundle list (file, above max depth)' '\n> +       cat >bundle-list-1 <<-EOF &&\n> +       [bundle]\n> +               version = 1\n> +               mode = any\n> +\n> +       [bundle \"bundle-list-2\"]\n> +               uri = file://$(pwd)/bundle-list-2\n> +       EOF\n> +\n> +       cat >bundle-list-2 <<-EOF &&\n> +       [bundle]\n> +               version = 1\n> +               mode = any\n> +\n> +       [bundle \"bundle-list-3\"]\n> +               uri = file://$(pwd)/bundle-list-3\n> +       EOF\n> +\n> +       cat >bundle-list-3 <<-EOF &&\n> +       [bundle]\n> +               version = 1\n> +               mode = any\n> +\n> +       [bundle \"bundle-list-4\"]\n> +               uri = file://$(pwd)/bundle-list-4\n> +       EOF\n> +\n> +       cat >bundle-list-4 <<-EOF &&\n> +       [bundle]\n> +               version = 1\n> +               mode = any\n> +\n> +       [bundle \"bundle-0\"]\n> +               uri = file://$(pwd)/clone-from/bundle-0.bundle\n\nIs there a reason why it's not more like:\n\n       [bundle \"bundle-list-5\"]\n               uri = file://$(pwd)/bundle-list-5\n\n?\n\n> +       EOF\n\nIt looks like the above is the setup part of the following tests, so\nit could perhaps be moved into a separate `test_expect_success 'setup\ndeep clone bundle list'` test.\n\n> +       git clone --bundle-uri=\"file://$(pwd)/bundle-list-1\" \\\n> +               clone-from clone-too-deep 2>err &&\n> +       ! grep \"fatal\" err &&\n> +       grep \"warning: exceeded bundle URI recursion limit\" err &&\n> +\n> +       git -C clone-from for-each-ref --format=\"%(objectname)\" >oids &&\n> +       git -C clone-too-deep cat-file --batch-check <oids &&\n> +\n> +       git -C clone-too-deep for-each-ref --format=\"%(refname)\" >refs &&\n> +       ! grep \"refs/bundles/\" refs\n> +'\n> +\n> +test_expect_success 'clone bundle list (file, below max depth)' '\n> +       git clone --bundle-uri=\"file://$(pwd)/bundle-list-2\" \\\n> +               clone-from clone-max-depth 2>err &&\n> +       ! grep \"fatal\" err &&\n> +       ! grep \"warning: exceeded bundle URI recursion limit\" err &&\n> +\n> +       git -C clone-from for-each-ref --format=\"%(objectname)\" >oids &&\n> +       git -C clone-max-depth cat-file --batch-check <oids &&\n> +\n> +       git -C clone-max-depth for-each-ref --format=\"%(refname)\" >refs &&\n> +       ! grep \"refs/bundles/\" refs\n> +'\n\nThanks!\n"},{"id":"497502","messageId":"xmqqfrt6nsds.fsf@gitster.g","threadId":"61664","inReplyTo":"CAP8UFD3jGVaO2JdKE0H22mf4E8YBxrEwuGqJuyeRGBVh==23NQ@mail.gmail.com","subject":"Re: [PATCH] bundle-uri.c: Fix double increment in depth","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T16:59:59Z","receivedAt":"2024-06-21T17:00:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> On Fri, Jun 21, 2024 at 11:23 AM Toon Claes <toon@iotcl.com> wrote:\n>>\n>> A bundle URI can serve a gitformat-bundle(5) or a bundle list. This\n>> plain text file is in the Git config format containing other bundle\n>> URIs. To avoid these bundle lists to nest too deep, we've set a limit\n>> with `max_bundle_uri_depth`.\n>\n> Yeah, max_bundle_uri_depth seems to be hardcoded to 4.\n>\n>> Although, when walk through the tree of\n>\n> s/walk/walking/\n\nWhile you are typofixing ...\n\n> Subject: Re: [PATCH] bundle-uri.c: Fix double increment in depth\n\n... also \"Fix\" -> \"fix\".\n\n\n>> bundles, the current depth is incremented in download_bundle_list() and\n>> then calls download_bundle_to_file(), which also increments the depth.\n>\n> s/and then calls/which then calls/\n>\n>> Remove the increment in download_bundle_to_file().\n>\n> The increment is removed by replacing:\n>\n>     fetch_bundle_uri_internal( ..., ctx->depth + 1, ...)\n>\n> with:\n>\n>     fetch_bundle_uri_internal( ..., ctx->depth, ...)\n>\n> in download_bundle_to_file(). Ok.\n>\n> It looks like there is another similar call to that function like this:\n>\n> fetch_bundle_uri_internal( ... , ctx.depth + 1, ... )\n>\n> in fetch_bundles_by_token() though.\n\nI have to wonder if the code should pass the whole ctx around,\ninstead of passing depth separately, and increment it at the single\nplace that matters, in order to reduce the chance of similar problem\nhappening.  The place that matters the recursion depth can be the\ndownload_bundle_list() function---that is the one that controls the\nrecursion, and it is incrementing the depth for the calls it makes\n(via the for_all_* callback mechanism).  Alternatively, it can be\nthe fetch_bundle_uri_internal() function where actual copying, for\nwhich we do want to enforce the depth limit, happens.  The function\neven has the code for depth limit, so having an increment next to it\nmay make it more readable and understandable.\n\nSo instead of taking ctx->r, ctx->depth+1, and ctx->list separately,\nshouldn't fetch_bundle_uri_internal() take the whole ctx and use\nctx->depth (not +1---incrementing it is not its business) and the\nwhole (current and future) problem like this goes away, no?\n\n"},{"id":"497505","messageId":"CAPig+cQ7y61LZW9zDMiHLmyEm=DLcBsuR_yPOzXiK6y9iXazWw@mail.gmail.com","threadId":"61664","inReplyTo":"20240621092258.1557258-1-toon@iotcl.com","subject":"Re: [PATCH] bundle-uri.c: Fix double increment in depth","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-06-21T17:17:57Z","receivedAt":"2024-06-21T17:18:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 21, 2024 at 5:23 AM Toon Claes <toon@iotcl.com> wrote:\n> A bundle URI can serve a gitformat-bundle(5) or a bundle list. This\n> plain text file is in the Git config format containing other bundle\n> URIs. To avoid these bundle lists to nest too deep, we've set a limit\n> with `max_bundle_uri_depth`. Although, when walk through the tree of\n> bundles, the current depth is incremented in download_bundle_list() and\n> then calls download_bundle_to_file(), which also increments the depth.\n> Remove the increment in download_bundle_to_file().\n>\n> Signed-off-by: Toon Claes <toon@iotcl.com>\n> ---\n> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh\n> @@ -259,6 +259,68 @@ test_expect_success 'clone bundle list (file, any mode, all failures)' '\n> +test_expect_success 'clone bundle list (file, above max depth)' '\n> +       cat >bundle-list-1 <<-EOF &&\n> +       [bundle]\n> +               version = 1\n> +               mode = any\n> +\n> +       [bundle \"bundle-list-2\"]\n> +               uri = file://$(pwd)/bundle-list-2\n> +       EOF\n> +\n> +       cat >bundle-list-2 <<-EOF &&\n> +       [bundle]\n> +               version = 1\n> +               mode = any\n> +\n> +       [bundle \"bundle-list-3\"]\n> +               uri = file://$(pwd)/bundle-list-3\n> +       EOF\n> + [...]\n\nA very minor (and subjective) comment beyond those offered by other reviewers...\n\nThe above \"setup\" functionality can be collapsed to:\n\n    for i in $(test_seq 4)\n    do\n        cat >bundle-list-$i <<-EOF || return 1\n        [bundle]\n            version = 1\n            mode = any\n        [bundle \"bundle-list-$(($i + 1))\"]\n            uri = file://$(pwd)/bundle-list-$(($i + 1))\n        EOF\n    done\n"}]}