Re: [PATCH v2 1/2] dir: do not apply prefix to negative pathspecs

[Junio C Hamano <gitster@pobox.com>]

Elijah Newren <newren@gmail.com> writes:

> Hi Yannik,
>
> On Thu, Sep 3, 2026 at 3:23 AM Yannik Tausch <dev@ytausch.de> wrote:
>>
>> 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:
> ...
> Would it make sense to add a regression case whose failure before this
> patch is deterministic without ASan?

Very good point.

Even if a negative pathspec were long enough, it would produce an
incorrect result if you strip the leading part of a negative entry.

With positive elements "a/b" and "a/c", and a negative element
"x/b", both paths "a/b/m" and "a/c/n" should match the pathspec with
these three elements, but if you incorrectly use prefix=2 to strip
the common prefix computed across positives, i.e., "a/", while
trying to see if the path "a/b/m" matches negative "x/b", we'd end
up trying to see if subpath "b/m" (in "a/b/m", after 2 leading
prefix bytes are stripped away) matches subpattern "b" (in "x/b",
after incorrectly stripping 2 leading bytes).  Yay, "b/m" begins
with "b" so it matches!  Not quite.

    $ git init
    $ mkdir -p a/b a/c
    $ >a/b/m >a/c/n
    $ git add a
    $ rungit jch ls-files a/b ':!x/b' a/c
    a/b/m
    a/c/n
    $ rungit master ls-files a/b ':!x/b' a/c
    a/c/n

So "if prefix computed across positives is longer than a negative
element" is a special case that may manifest as one extra breakage
(i.e., logically it is wrong in that it uses incorrectly shortened
pattern and path for negated matching and produce incorrect result,
but in addition to that, the negated pattern string points outside
the original string, accessing wrong piece of memory), but I tend to
agree that it is equally if not more important to demonstrate what
is broken even without that extra breakage.

Thanks.