Re: [PATCH 1/3] sparse-checkout: fix segfault on malformed patterns
- From
Elijah Newren <newren@gmail.com>
- Date
- Dec 7, 2021, 20:22 UTC
- Message-ID
- <CABPp-BF0VkL0VQYWu1SObVGVQA7TkHXT67vHQVQpkvmc3xMa8g@mail.gmail.com>
- In-Reply-To
- <becbee16d2ee503a7f698364aef672d4cadf5079.1638907336.git.gitgitgadget@gmail.com>
On Tue, Dec 7, 2021 at 12:02 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 14 quoted lines
> > 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 > no invalid.
s/no invalid/now invalid/ ?
Show 57 quoted lines
>
> 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>
> 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(-)
>
> 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
> --
> gitgitgadget
>