Re: [PATCH v2 1/2] dir: do not apply prefix to negative pathspecs
- From
Elijah Newren <newren@gmail.com>
- Date
- Sep 4, 2026, 05:00 UTC
- Message-ID
- <CABPp-BFJo80oE=rtWc0FRNUxVh=6NHZeQmHD2q69VGwDcrHNhw@mail.gmail.com>
- In-Reply-To
- <0617001F-13BB-4548-A10A-89877977CFB5@ytausch.de>
Hi Yannik,
On Thu, Sep 3, 2026 at 3:23 AM Yannik Tausch <dev@ytausch.de> wrote:
Show 68 quoted lines
>
> common_prefix_len() derives the common prefix solely from positive
> pathspecs, skipping those marked with PATHSPEC_EXCLUDE. However,
> match_pathspec_with_flags() also passes that prefix when matching the
> negative pathspecs.
>
> A negative pathspec may be shorter than the prefix. In that case,
> match_pathspec_item() advances item->match beyond its allocation and
> subtracts the prefix from item->len, producing a negative matchlen. It
> then dereferences the out-of-bounds pointer. If the resulting byte is
> not NUL, matchlen is converted to size_t when passed to ps_strncmp(),
> which may cause a much larger out-of-bounds read.
>
> The problem can be reproduced with AddressSanitizer:
>
> make SANITIZE=address CFLAGS="-g -O0" git
> git init test &&
> cd test &&
> DIR=$(printf "a%.0s" {1..150}) &&
> mkdir -p "$DIR" &&
> touch "$DIR/f.txt" &&
> git add -A &&
> git commit -m test &&
> ../git ls-files -- "$DIR/" ":(exclude)xy"
>
> This reports a heap-buffer-overflow. Without AddressSanitizer, the
> output may depend on the contents of memory following the negative
> pathspec.
>
> Fix the bug by using a zero prefix when matching negative pathspecs.
> Add a regression test that combines a positive pathspec with a longer
> common prefix and a shorter, unrelated negative pathspec.
>
> Signed-off-by: Yannik Tausch <dev@ytausch.de>
> ---
> dir.c | 2 +-
> t/t6132-pathspec-exclude.sh | 9 +++++++++
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/dir.c b/dir.c
> index 95d8a1cce9..7072715389 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -593,7 +593,7 @@ static int match_pathspec_with_flags(struct index_state *istate,
> if (!(ps->magic & PATHSPEC_EXCLUDE) || !positive)
> return positive;
> negative = do_match_pathspec(istate, ps, name, namelen,
> - prefix, seen,
> + 0, seen,
> flags | DO_MATCH_EXCLUDE);
> return negative ? 0 : positive;
> }
> diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
> index 9fdafeb1e9..ad919cc739 100755
> --- a/t/t6132-pathspec-exclude.sh
> +++ b/t/t6132-pathspec-exclude.sh
> @@ -183,6 +183,15 @@ EOF
> test_cmp expect actual
> '
>
> +test_expect_success 'negative pathspec shorter than positive pathspec prefix' '
> + git ls-files -- sub/sub/ ":(exclude)sub2" >actual &&
> + cat <<-\EOF >expect &&
> + sub/sub/file
> + sub/sub/sub/file
> + EOF
> + test_cmp expect actual
> +'Would it make sense to add a regression case whose failure before this patch is deterministic without ASan?
The test above advances beyond the end of "sub2", so its result depends on out-of-bounds memory. I actually saw this test pass without your fixes, when not run under ASan, which may depend on the allocator or build.
An alternative would be an exclude whose length equals the seven-byte prefix, keeping the accesses in bounds:
test_expect_success 'exclude is matched against the full path' '
git ls-files -- sub/sub/ ":(exclude)zzzzzzz" >actual &&
cat <<-\EOF >expect &&
sub/sub/file
sub/sub/sub/file
EOF
test_cmp expect actual
'Before this patch, stripping seven bytes points at the exclude string's NUL terminator, which is then treated as matching everything. I get no output before the fix, and both expected paths after your fix.
I'm not suggesting this as a replacement for your regression test; I think the out-of-bounds case is still useful. I just think this extra testcase might be a nice complement.