Re: [PATCH] ref-filter: fix stale parsed objects

From: Jeff King <peff@peff.net>
Date: Nov 4, 2025, 21:33 UTC
Message-ID: <20251104213332.GB2618884@coredump.intra.peff.net>
In-Reply-To: <20251104-b4-pks-ref-filter-fixup-v1-1-2fbca52d76d9@pks.im>

On Tue, Nov 04, 2025 at 03:36:13PM +0100, Patrick Steinhardt wrote:

> Fix the issue by resetting `maybe_object` in `get_object()`.

Thanks, this spot makes sense looking at the context.

Show 19 quoted lines

> +test_expect_success 'annotated tag version sort' '
> +	git tag -a -m "sample 1.0" vsample-1.0 &&
> +	git tag -a -m "sample 2.0" vsample-2.0 &&
> +	git tag -a -m "sample 10.0" vsample-10.0 &&
> +	cat >expect <<-EOF &&
> +	vsample-1.0
> +	vsample-2.0
> +	vsample-10.0
> +	EOF
> +
> +	git tag --list --sort=version:tag vsample-\* >actual &&
> +	test_cmp expect actual &&
> +
> +	# Ensure that we also handle this case alright in the case we have the
> +	# peeled values cached e.g. via the packed-refs file.
> +	git pack-refs --all &&
> +	git tag --list --sort=version:tag vsample-\* &&
> +	test_cmp expect actual
> +'

This test seems fine, though I think you can see the same thing even more easily with just:

  git for-each-ref --format='%(refname) %(tag)' refs/tags/vsample-\*

which shows each tag after the first with the same (wrong) tag.

Curiously if I run something similar in git.git like:

  git for-each-ref --format='%(refname) %(tag)'

I get garbage uninitialized data on each of the tag lines. The difference is that the first parsed object is a non-tag, so our stale parsed state didn't actually fill in the tag values. Surprisingly ASan doesn't complain, but it may be because we end up looking at memory with a bogus type-cast (get_or_parse returns the stale "struct commit", but we cast it to a "struct tag").

I didn't dig too deeply there since the fix here should make it all go away (and seems to in my testing). I wondered if there was a way for a broken repo to fool the code here (we think something is a tag, but in the odb it's really a commit or something), but I don't think so. We enter grab_tag_values() based on data->type being OBJ_TAG, and then we feed that same value to parse_object_buffer(). So it will be the same everywhere, and our cast can never do the wrong thing. And if we see corruption (e.g., a tag refers to object X as a blob, and then another tag refers to it as a commit), then lookup_commit() should return NULL for us.

So I think this fix should be sufficient.

-Peff