diff mbox series

[v3,1/3] sparse-checkout: fix segfault on malformed patterns

Message ID 1744a26845fbe4d7dbc80f387be1d842b5f8fe94.1639575968.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series sparse-checkout: fix segfault on malformed patterns | expand

Commit Message

Derrick Stolee Dec. 15, 2021, 1:46 p.m. UTC 
From: Derrick Stolee <dstolee@microsoft.com>

Then core.sparseCheckoutCone is enabled, the sparse-checkout patterns are
used to populate two hashsets that accelerate pattern matching. If the user
modifies the sparse-checkout file outside of the 'sparse-checkout' builtin,
then strange patterns can happen, triggering some error checks.

One of these error checks is possible to hit when some special characters
exist in a line. A warning message is correctly written to stderr, but then
there is additional logic that attempts to remove the line from the hashset
and free the data. This leads to a segfault in the 'git sparse-checkout
list' command because it iterates over the contents of the hashset, which is
now invalid.

The fix here is to stop trying to remove from the hashset. Better to leave
bad data in the sparse-checkout matching logic (with a warning) than to
segfault. If we are in this state, then we are already traversing into
undefined behavior, so this change to keep the entry in the hashset is no
worse than removing it.

Add a test that triggers the segfault without the code change.

Reported-by: John Burnett <johnburnett@johnburnett.com>
Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 dir.c                              |  3 ---
 t/t1091-sparse-checkout-builtin.sh | 15 +++++++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Dec. 15, 2021, 8:56 p.m. UTC | #1 
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Then core.sparseCheckoutCone is enabled, the sparse-checkout patterns are
> used to populate two hashsets that accelerate pattern matching. If the user
> modifies the sparse-checkout file outside of the 'sparse-checkout' builtin,
> then strange patterns can happen, triggering some error checks.
>
> One of these error checks is possible to hit when some special characters
> exist in a line. A warning message is correctly written to stderr, but then
> there is additional logic that attempts to remove the line from the hashset
> and free the data.

Makes sense.

> This leads to a segfault in the 'git sparse-checkout
> list' command because it iterates over the contents of the hashset, which is
> now invalid.

Understandable.

> The fix here is to stop trying to remove from the hashset. Better to leave
> bad data in the sparse-checkout matching logic (with a warning) than to
> segfault.

True, as long as it won't make the situation worse by depending on
that bad data to further damage working tree data or in-repo data
when damaged working tree data gets committed.  And "list segfaults
with freed/NULLed data---so leave the bad ones in to print these bad
ones" feels OK-ish.  

As long as the user is not transporting the listed output to another
repository, which may fall into "making the situation worse"
category by spreading an existing breakage, that is.

In other words, this may paper over the segfault, and it may be safe
only for "sparse-checkout list", but is it safe for other operations
that actually use this bad data to further affect other things in
the repository?  If not, I wonder if we want to hard die to lock the
repository down before the issue is fixed to avoid spreading the
damage?

> diff --git a/dir.c b/dir.c
> index 5aa6fbad0b7..0693c7cb3ee 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -819,9 +819,6 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern
>  		/* we already included this at the parent level */
>  		warning(_("your sparse-checkout file may have issues: pattern '%s' is repeated"),
>  			given->pattern);
> -		hashmap_remove(&pl->parent_hashmap, &translated->ent, &data);
> -		free(data);
> -		free(translated);
>  	}
>  
>  	return;
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 272ba1b566b..c72b8ee2e7b 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -708,4 +708,19 @@ test_expect_success 'cone mode clears ignored subdirectories' '
>  	test_cmp expect out
>  '
>  
> +test_expect_success 'malformed cone-mode patterns' '
> +	git -C repo sparse-checkout init --cone &&
> +	mkdir -p repo/foo/bar &&
> +	touch repo/foo/bar/x repo/foo/y &&
> +	cat >repo/.git/info/sparse-checkout <<-\EOF &&
> +	/*
> +	!/*/
> +	/foo/
> +	!/foo/*/
> +	/foo/\*/
> +	EOF
> +	cat repo/.git/info/sparse-checkout &&

Stray debugging output?

> +	git -C repo sparse-checkout list

And we are happy as long as the command does not segfault, and we do
not care what the output is.

> +'
> +
>  test_done

Will queue, but not convinced yet.
Derrick Stolee Dec. 16, 2021, 2:23 p.m. UTC | #2 
On 12/15/2021 3:56 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> Then core.sparseCheckoutCone is enabled, the sparse-checkout patterns are
>> used to populate two hashsets that accelerate pattern matching. If the user
>> modifies the sparse-checkout file outside of the 'sparse-checkout' builtin,
>> then strange patterns can happen, triggering some error checks.
>>
>> One of these error checks is possible to hit when some special characters
>> exist in a line. A warning message is correctly written to stderr, but then
>> there is additional logic that attempts to remove the line from the hashset
>> and free the data.
> 
> Makes sense.
> 
>> This leads to a segfault in the 'git sparse-checkout
>> list' command because it iterates over the contents of the hashset, which is
>> now invalid.
> 
> Understandable.
> 
>> The fix here is to stop trying to remove from the hashset. Better to leave
>> bad data in the sparse-checkout matching logic (with a warning) than to
>> segfault.
> 
> True, as long as it won't make the situation worse by depending on
> that bad data to further damage working tree data or in-repo data
> when damaged working tree data gets committed.  And "list segfaults
> with freed/NULLed data---so leave the bad ones in to print these bad
> ones" feels OK-ish.  
> 
> As long as the user is not transporting the listed output to another
> repository, which may fall into "making the situation worse"
> category by spreading an existing breakage, that is.
> 
> In other words, this may paper over the segfault, and it may be safe
> only for "sparse-checkout list", but is it safe for other operations
> that actually use this bad data to further affect other things in
> the repository?  If not, I wonder if we want to hard die to lock the
> repository down before the issue is fixed to avoid spreading the
> damage?

You're right. I should do what we do elsewhere in this method and
disable cone mode pattern matching, reverting to the old matching
algorithms. This will clear the hashsets and avoid using them anywhere
else.

>> +test_expect_success 'malformed cone-mode patterns' '
>> +	git -C repo sparse-checkout init --cone &&
>> +	mkdir -p repo/foo/bar &&
>> +	touch repo/foo/bar/x repo/foo/y &&
>> +	cat >repo/.git/info/sparse-checkout <<-\EOF &&
>> +	/*
>> +	!/*/
>> +	/foo/
>> +	!/foo/*/
>> +	/foo/\*/
>> +	EOF
>> +	cat repo/.git/info/sparse-checkout &&
> 
> Stray debugging output?

Thanks.

>> +	git -C repo sparse-checkout list
> 
> And we are happy as long as the command does not segfault, and we do
> not care what the output is.

And I'll be able to test that the 'list' subcommand changes behavior
when cone mode is disabled, in addition to checking stderr for the
correct warnings.

Thanks,
-Stolee
diff mbox series

Patch

diff --git a/dir.c b/dir.c
index 5aa6fbad0b7..0693c7cb3ee 100644
--- a/dir.c
+++ b/dir.c
@@ -819,9 +819,6 @@  static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern
 		/* we already included this at the parent level */
 		warning(_("your sparse-checkout file may have issues: pattern '%s' is repeated"),
 			given->pattern);
-		hashmap_remove(&pl->parent_hashmap, &translated->ent, &data);
-		free(data);
-		free(translated);
 	}
 
 	return;
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 272ba1b566b..c72b8ee2e7b 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -708,4 +708,19 @@  test_expect_success 'cone mode clears ignored subdirectories' '
 	test_cmp expect out
 '
 
+test_expect_success 'malformed cone-mode patterns' '
+	git -C repo sparse-checkout init --cone &&
+	mkdir -p repo/foo/bar &&
+	touch repo/foo/bar/x repo/foo/y &&
+	cat >repo/.git/info/sparse-checkout <<-\EOF &&
+	/*
+	!/*/
+	/foo/
+	!/foo/*/
+	/foo/\*/
+	EOF
+	cat repo/.git/info/sparse-checkout &&
+	git -C repo sparse-checkout list
+'
+
 test_done