threads / patch / 61664

patchbundle-uri.c: Fix double increment in depth

Subject: [PATCH] bundle-uri.c: Fix double increment in depth

## tl;dr

4 messages between Jun 21, 2024 and Jun 21, 2024. Diffs are folded; open one to read it.

replies: 3people: 4as markdown or json

Toon Claes· Jun 21, 2024, 09:22 UTC · lore

A bundle URI can serve a gitformat-bundle(5) or a bundle list. This plain text file is in the Git config format containing other bundle URIs. To avoid these bundle lists to nest too deep, we've set a limit with `max_bundle_uri_depth`. Although, when walk through the tree of bundles, the current depth is incremented in download_bundle_list() and then calls download_bundle_to_file(), which also increments the depth. Remove the increment in download_bundle_to_file().

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 bundle-uri.c                |  2 +-
 t/t5558-clone-bundle-uri.sh | 62 +++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 1 deletion(-)
Show changes to 2 files +63 −2

bundle-uri.c, t/t5558-clone-bundle-uri.sh

diff --git a/bundle-uri.c b/bundle-uri.c
index 91b3319a5c..7b1a711919 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -436,7 +436,7 @@ static int download_bundle_to_file(struct remote_bundle_info *bundle, void *data
 	if (ctx->mode == BUNDLE_MODE_ANY && ctx->count)
 		return 0;

-	res = fetch_bundle_uri_internal(ctx->r, bundle, ctx->depth + 1, ctx->list);
+	res = fetch_bundle_uri_internal(ctx->r, bundle, ctx->depth, ctx->list);

 	/*
 	 * Only increment count if the download succeeded. If our mode is
diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
index 1ca5f745e7..f3a8494297 100755
--- a/t/t5558-clone-bundle-uri.sh
+++ b/t/t5558-clone-bundle-uri.sh
@@ -259,6 +259,68 @@ test_expect_success 'clone bundle list (file, any mode, all failures)' '
 	! grep "refs/bundles/" refs
 '

+test_expect_success 'clone bundle list (file, above max depth)' '
+	cat >bundle-list-1 <<-EOF &&
+	[bundle]
+		version = 1
+		mode = any
+
+	[bundle "bundle-list-2"]
+		uri = file://$(pwd)/bundle-list-2
+	EOF
+
+	cat >bundle-list-2 <<-EOF &&
+	[bundle]
+		version = 1
+		mode = any
+
+	[bundle "bundle-list-3"]
+		uri = file://$(pwd)/bundle-list-3
+	EOF
+
+	cat >bundle-list-3 <<-EOF &&
+	[bundle]
+		version = 1
+		mode = any
+
+	[bundle "bundle-list-4"]
+		uri = file://$(pwd)/bundle-list-4
+	EOF
+
+	cat >bundle-list-4 <<-EOF &&
+	[bundle]
+		version = 1
+		mode = any
+
+	[bundle "bundle-0"]
+		uri = file://$(pwd)/clone-from/bundle-0.bundle
+	EOF
+
+	git clone --bundle-uri="file://$(pwd)/bundle-list-1" \
+		clone-from clone-too-deep 2>err &&
+	! grep "fatal" err &&
+	grep "warning: exceeded bundle URI recursion limit" err &&
+
+	git -C clone-from for-each-ref --format="%(objectname)" >oids &&
+	git -C clone-too-deep cat-file --batch-check <oids &&
+
+	git -C clone-too-deep for-each-ref --format="%(refname)" >refs &&
+	! grep "refs/bundles/" refs
+'
+
+test_expect_success 'clone bundle list (file, below max depth)' '
+	git clone --bundle-uri="file://$(pwd)/bundle-list-2" \
+		clone-from clone-max-depth 2>err &&
+	! grep "fatal" err &&
+	! grep "warning: exceeded bundle URI recursion limit" err &&
+
+	git -C clone-from for-each-ref --format="%(objectname)" >oids &&
+	git -C clone-max-depth cat-file --batch-check <oids &&
+
+	git -C clone-max-depth for-each-ref --format="%(refname)" >refs &&
+	! grep "refs/bundles/" refs
+'
+
 #########################################################################
 # HTTP tests begin here

--
2.45.0
Christian Couder· Jun 21, 2024, 15:24 UTC · re: Toon Claes · lore

Re: [PATCH] bundle-uri.c: Fix double increment in depth

On Fri, Jun 21, 2024 at 11:23 AM Toon Claes <toon@iotcl.com> wrote:
Show 5 quoted lines
>
> A bundle URI can serve a gitformat-bundle(5) or a bundle list. This
> plain text file is in the Git config format containing other bundle
> URIs. To avoid these bundle lists to nest too deep, we've set a limit
> with `max_bundle_uri_depth`.
Yeah, max_bundle_uri_depth seems to be hardcoded to 4.
> Although, when walk through the tree of
s/walk/walking/
> bundles, the current depth is incremented in download_bundle_list() and
> then calls download_bundle_to_file(), which also increments the depth.
s/and then calls/which then calls/
> Remove the increment in download_bundle_to_file().
The increment is removed by replacing:
    fetch_bundle_uri_internal( ..., ctx->depth + 1, ...)
with:
    fetch_bundle_uri_internal( ..., ctx->depth, ...)
in download_bundle_to_file(). Ok.
It looks like there is another similar call to that function like this:
fetch_bundle_uri_internal( ... , ctx.depth + 1, ... )
in fetch_bundles_by_token() though.

There ctx.depth is initialized to 0 before the call, so it looks like it could work, but fetch_bundle_uri_internal() can call fetch_bundle_list_in_config_format() which can call download_bundle_list() which, as we saw above, still increases the depth by 1.

So even if download_bundle_list() then calls download_bundle_to_file() without increasing the depth, I am not sure it works well in all cases. At least I think a bit more explanations might be needed.

Show 35 quoted lines
> +test_expect_success 'clone bundle list (file, above max depth)' '
> +       cat >bundle-list-1 <<-EOF &&
> +       [bundle]
> +               version = 1
> +               mode = any
> +
> +       [bundle "bundle-list-2"]
> +               uri = file://$(pwd)/bundle-list-2
> +       EOF
> +
> +       cat >bundle-list-2 <<-EOF &&
> +       [bundle]
> +               version = 1
> +               mode = any
> +
> +       [bundle "bundle-list-3"]
> +               uri = file://$(pwd)/bundle-list-3
> +       EOF
> +
> +       cat >bundle-list-3 <<-EOF &&
> +       [bundle]
> +               version = 1
> +               mode = any
> +
> +       [bundle "bundle-list-4"]
> +               uri = file://$(pwd)/bundle-list-4
> +       EOF
> +
> +       cat >bundle-list-4 <<-EOF &&
> +       [bundle]
> +               version = 1
> +               mode = any
> +
> +       [bundle "bundle-0"]
> +               uri = file://$(pwd)/clone-from/bundle-0.bundle
Is there a reason why it's not more like:
       [bundle "bundle-list-5"]
               uri = file://$(pwd)/bundle-list-5
?
> +       EOF

It looks like the above is the setup part of the following tests, so it could perhaps be moved into a separate `test_expect_success 'setup deep clone bundle list'` test.

Show 24 quoted lines
> +       git clone --bundle-uri="file://$(pwd)/bundle-list-1" \
> +               clone-from clone-too-deep 2>err &&
> +       ! grep "fatal" err &&
> +       grep "warning: exceeded bundle URI recursion limit" err &&
> +
> +       git -C clone-from for-each-ref --format="%(objectname)" >oids &&
> +       git -C clone-too-deep cat-file --batch-check <oids &&
> +
> +       git -C clone-too-deep for-each-ref --format="%(refname)" >refs &&
> +       ! grep "refs/bundles/" refs
> +'
> +
> +test_expect_success 'clone bundle list (file, below max depth)' '
> +       git clone --bundle-uri="file://$(pwd)/bundle-list-2" \
> +               clone-from clone-max-depth 2>err &&
> +       ! grep "fatal" err &&
> +       ! grep "warning: exceeded bundle URI recursion limit" err &&
> +
> +       git -C clone-from for-each-ref --format="%(objectname)" >oids &&
> +       git -C clone-max-depth cat-file --batch-check <oids &&
> +
> +       git -C clone-max-depth for-each-ref --format="%(refname)" >refs &&
> +       ! grep "refs/bundles/" refs
> +'
Thanks!
Junio C Hamano· Jun 21, 2024, 16:59 UTC · re: Christian Couder · lore

Re: [PATCH] bundle-uri.c: Fix double increment in depth

Christian Couder <christian.couder@gmail.com> writes:
Show 12 quoted lines
> On Fri, Jun 21, 2024 at 11:23 AM Toon Claes <toon@iotcl.com> wrote:
>>
>> A bundle URI can serve a gitformat-bundle(5) or a bundle list. This
>> plain text file is in the Git config format containing other bundle
>> URIs. To avoid these bundle lists to nest too deep, we've set a limit
>> with `max_bundle_uri_depth`.
>
> Yeah, max_bundle_uri_depth seems to be hardcoded to 4.
>
>> Although, when walk through the tree of
>
> s/walk/walking/
While you are typofixing ...
> Subject: Re: [PATCH] bundle-uri.c: Fix double increment in depth
... also "Fix" -> "fix".
Show 22 quoted lines
>> bundles, the current depth is incremented in download_bundle_list() and
>> then calls download_bundle_to_file(), which also increments the depth.
>
> s/and then calls/which then calls/
>
>> Remove the increment in download_bundle_to_file().
>
> The increment is removed by replacing:
>
>     fetch_bundle_uri_internal( ..., ctx->depth + 1, ...)
>
> with:
>
>     fetch_bundle_uri_internal( ..., ctx->depth, ...)
>
> in download_bundle_to_file(). Ok.
>
> It looks like there is another similar call to that function like this:
>
> fetch_bundle_uri_internal( ... , ctx.depth + 1, ... )
>
> in fetch_bundles_by_token() though.

I have to wonder if the code should pass the whole ctx around, instead of passing depth separately, and increment it at the single place that matters, in order to reduce the chance of similar problem happening. The place that matters the recursion depth can be the download_bundle_list() function---that is the one that controls the recursion, and it is incrementing the depth for the calls it makes (via the for_all_* callback mechanism). Alternatively, it can be the fetch_bundle_uri_internal() function where actual copying, for which we do want to enforce the depth limit, happens. The function even has the code for depth limit, so having an increment next to it may make it more readable and understandable.

So instead of taking ctx->r, ctx->depth+1, and ctx->list separately, shouldn't fetch_bundle_uri_internal() take the whole ctx and use ctx->depth (not +1---incrementing it is not its business) and the whole (current and future) problem like this goes away, no?

Eric Sunshine· Jun 21, 2024, 17:17 UTC · re: Toon Claes · lore

Re: [PATCH] bundle-uri.c: Fix double increment in depth

On Fri, Jun 21, 2024 at 5:23 AM Toon Claes <toon@iotcl.com> wrote:
Show 31 quoted lines
> A bundle URI can serve a gitformat-bundle(5) or a bundle list. This
> plain text file is in the Git config format containing other bundle
> URIs. To avoid these bundle lists to nest too deep, we've set a limit
> with `max_bundle_uri_depth`. Although, when walk through the tree of
> bundles, the current depth is incremented in download_bundle_list() and
> then calls download_bundle_to_file(), which also increments the depth.
> Remove the increment in download_bundle_to_file().
>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
> @@ -259,6 +259,68 @@ test_expect_success 'clone bundle list (file, any mode, all failures)' '
> +test_expect_success 'clone bundle list (file, above max depth)' '
> +       cat >bundle-list-1 <<-EOF &&
> +       [bundle]
> +               version = 1
> +               mode = any
> +
> +       [bundle "bundle-list-2"]
> +               uri = file://$(pwd)/bundle-list-2
> +       EOF
> +
> +       cat >bundle-list-2 <<-EOF &&
> +       [bundle]
> +               version = 1
> +               mode = any
> +
> +       [bundle "bundle-list-3"]
> +               uri = file://$(pwd)/bundle-list-3
> +       EOF
> + [...]
A very minor (and subjective) comment beyond those offered by other reviewers...
The above "setup" functionality can be collapsed to:
    for i in $(test_seq 4)
    do
        cat >bundle-list-$i <<-EOF || return 1
        [bundle]
            version = 1
            mode = any
        [bundle "bundle-list-$(($i + 1))"]
            uri = file://$(pwd)/bundle-list-$(($i + 1))
        EOF
    done

← back to recent threads