Re: [PATCH 1/3] sparse-checkout: fix OOM error with mixed patterns

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 20, 2021, 18:24 UTC
Message-ID: <YUjRziaIoVtJWGsS@nand.local>
In-Reply-To: <d90937b9ac9aaa6170f56d18ddb7a327b2af27d6.1632160658.git.gitgitgadget@gmail.com>

On Mon, Sep 20, 2021 at 05:57:36PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 32 quoted lines

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Add a test to t1091-sparse-checkout-builtin.sh that would result in an
> infinite loop and out-of-memory error before this change. The issue
> relies on having non-cone-mode patterns while trying to modify the
> patterns in cone-mode.
>
> The fix is simple, allowing us to break from the loop when the input
> path does not contain a slash, as the "dir" pattern we added does not.
>
> This is only a fix to the critical out-of-memory error. A better
> response to such a strange state will follow in a later change.
>
> Reported-by: Calbabreaker <calbabreaker@gmail.com>
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/sparse-checkout.c          | 2 +-
>  t/t1091-sparse-checkout-builtin.sh | 8 ++++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 8ba9f13787b..b45fd97a98b 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -389,7 +389,7 @@ static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat
>  		char *oldpattern = e->pattern;
>  		size_t newlen;
>
> -		if (slash == e->pattern)
> +		if (!slash || slash == e->pattern)
>  			break;

If we are preparing to make it so that we do not blindly copy patterns from a sparse checkout without cone mode enabled, then wouldn't this new case be a BUG()?

Of course, users could still tweak the contents of their sparse-checkout file as they wish, but I'd expect that we'd catch those cases, too (i.e., by validating the contents of the existing sparse-checkout before calling this function).

Show 13 quoted lines

> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 38fc8340f5c..a429d2cc671 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -103,6 +103,14 @@ test_expect_success 'clone --sparse' '
>  	check_files clone a
>  '
>
> +test_expect_success 'switching to cone mode with non-cone mode patterns' '
> +	git init bad-patterns &&
> +	git -C bad-patterns sparse-checkout init &&
> +	git -C bad-patterns sparse-checkout add dir &&
> +	git -C bad-patterns config core.sparseCheckoutCone true &&

Makes sense that we'd want to update the config rather than call "init --cone" here, since a subsequent patch would change the thing that this is testing (from "doesn't OOM in the above-described situation" to "clears the existing contents of your sparse-checkout").

> +	git -C bad-patterns sparse-checkout add dir
> +'
> +

In other series I've suggested a cosmetic change to move all of these to a sub-shell that begins with "cd bad-patterns &&", but obviously that is a relatively unimportant suggestion.

Thanks, Taylor