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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 4, 2026, 14:21 UTC
Message-ID: <xmqqmrtx6qsk.fsf@gitster.g>
In-Reply-To: <CABPp-BFJo80oE=rtWc0FRNUxVh=6NHZeQmHD2q69VGwDcrHNhw@mail.gmail.com>

Elijah Newren <newren@gmail.com> writes:

Show 20 quoted lines

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