Re: [PATCH 1/2] pathspec: use match for sparse-index expansion checks

[Taylor Blau <ttaylorr@openai.com>]

On Mon, Jul 20, 2026 at 03:31:20PM -0700, tnyman@openai.com wrote:
> Using `item.original + item.nowildcard_len` in
> `pathspec_needs_expanded_index()` can therefore read past the end of
> the allocation. AddressSanitizer reports a heap-buffer-overflow for
> prefixed wildcard pathspecs passed to `git rm` and `git reset` with a
> sparse index.
>
> The mismatch dates back to 4d1cfc1351 ("reset: make --mixed
> sparse-aware", 2021-11-29), which introduced the helper using
> `item.original`. b29ad38322 ("pathspec.h: move
> pathspec_needs_expanded_index() from reset.c to here", 2022-08-07)
> later moved it to `pathspec.c` and preserved the affected comparisons.

Nice find. I can reliably reproduce the ASan failure you described above
like so:

    repo=$(mktemp -d /tmp/pathspec-asan.XXXXXX)
    trap 'rm -rf "$repo"' EXIT

    git init "$repo"

    cd "$repo"

    mkdir -p deep outside
    : >deep/a
    : >outside/file
    git add .
    git commit -q -m base

    git sparse-checkout init --cone --sparse-index
    git sparse-checkout set deep

    # From deep/, match is "deep/a*" while original is only "a*".
    git.compile -C deep reset HEAD -- 'a*'

(where 'git.compile' points at my build, which in this case was compiled
with "make SANITIZE=address"), and results in

    ==89470==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000001f36 at pc 0x000106a69430 bp 0x00016b07c530 sp 0x00016b07bce0
    READ of size 1 at 0x602000001f36 thread T0
        #0 0x000106a6942c in strspn+0x3f0 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x1942c)
        #1 0x0001054a35b0 in pathspec_needs_expanded_index pathspec.c:850
        #2 0x000104fe6c24 in read_from_tree reset.c:214
        #3 0x000104fe5774 in cmd_reset reset.c:495
    [...]

It made me wonder whether or not this bug was trigger-able back in
4d1cfc1351. After checking out that version, I re-ran the same script
and got an identical buffer overflow in the 'strspn()' call.

Applying your patch and repeating the same steps results in a clean
exit.

> diff --git a/pathspec.c b/pathspec.c
> index f78b22709ccb67..281858f21f9c59 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -847,9 +847,9 @@ int pathspec_needs_expanded_index(struct index_state *istate,
>  			 * - not-in-cone/bar*: may need expanded index
>  			 * - **.c: may need expanded index
>  			 */
> -			if (strspn(item.original + item.nowildcard_len, "*") ==
> +			if (strspn(item.match + item.nowildcard_len, "*") ==

OK. The comment above is elided from the diff context, but is useful
IMHO during review. Here we want to make sure that the remaining
wildcard-ed portion of the pathspec element is only "*", which may need
to expand the index only if we are not inside of the existing sparse
checkout.

But 'item.nowildcard_len' bytes ahead of 'item.original' may (at worst)
point into uninitialized memory, or (at best) point at a portion of the
string that is not in fact a wildcard (even if the pathspec item would
not otherwise require us to expand the sparse checkout).

So this makes sense.

>  				    (unsigned int)(item.len - item.nowildcard_len) &&
> -			    path_in_cone_mode_sparse_checkout(item.original, istate))
> +			    path_in_cone_mode_sparse_checkout(item.match, istate))

Likewise. Here I think we *might* actually be OK, but I haven't read
'path_in_cone_mode_sparse_checkout()' to know whether that's (a) true,
and (b) if so, whether it's true by accident or intention.

Regardless, 'item.match' makes sense here as well for the same reason.
Likewise with the rest of the patch.

> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 9814431cd74aff..d0b42371663f9d 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2119,6 +2119,13 @@ test_expect_success 'sparse index is not expanded: rm' '
>  	ensure_not_expanded rm -r deep
>  '
>
> +test_expect_success 'sparse index is not expanded: prefixed wildcard pathspec' '
> +	init_repos &&
> +
> +	ensure_not_expanded -C deep rm --dry-run -- "a*" &&
> +	ensure_not_expanded -C deep reset base -- "a*"
> +'

Looks good, this is effectively the same thing as I ran in the
reproduction script above.

Thanks,
Taylor