Re: [PATCH 1/3] sparse-checkout: fix segfault on malformed patterns

[Elijah Newren <newren@gmail.com>]

On Tue, Dec 7, 2021 at 12:02 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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/ ?

>
> 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
>