Re: Infinite loop + memory leak in annotate_refs_with_symref_info

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 11, 2023, 22:52 UTC
Message-ID: <xmqqa5ze9glg.fsf@gitster.g>
In-Reply-To: <20230411215845.GA678138@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

> Yep. And Jonas's suggested fix is the right thing. Assigning offset
> directly _would_ be the right thing, since we are taking the distance
> back to the beginning of the feature_list string. Except that earlier in
> the function we incremented feature_list by the incoming value of
> the offset!

Sigh. Thanks for finding the problem with a fix. The data flow in this function is horrible, but yes, "found + len - feature_list" is smaller than the code expects to be because feature_list is moved forward before entering the loop, and I can see how the patch fixes the problem.

Show 13 quoted lines

> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 20d063fb9a..c8422d66e7 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -360,4 +360,39 @@ test_expect_success 'ls-remote prefixes work with all protocol versions' '
>  	test_cmp expect actual.v2
>  '
>  
> +test_expect_success 'v0 clients can handle multiple symrefs' '
> +	# Git will not generate multiple symref entries for v0 these days, but it
> +	# is technically allowed, and we did so until d007dbf7d6 (Revert
> +	# "upload-pack: send non-HEAD symbolic refs", 2013-11-18). Test the
> +	# client handling here by faking that older behavior.

Yeah, I remember that fix where somebody had tons of symbolic refs and busted the protocol limit. Is "multiple symref" used here because it is the easiest to reproduce the issue, or have we saw such a potentially broken server in the wild?

> +	# Note that our oid is hard-coded to always be sha1, and not using
> +	# test_oid. Since our fake capabilities line does not have an
> +	# object-format entry, the client will always use sha1 mode.

It probably is OK to run the test in that "undeclared - assume SHA-1" mode, even though I think we give an explicit "object-format" capability even when talking from the SHA-1 repository these days.

> +	oid=1234567890123456789012345678901234567890 &&
> +	symrefs="symref=refs/remotes/origin/HEAD:refs/remotes/origin/main" &&
> +	symrefs="$symrefs symref=HEAD:refs/heads/main" &&

Show 8 quoted lines

> I also wondered if we tested this multiple-symref case for protocol v2
> (where it works fine), but it looks like we may not. There are earlier
> tests which _would_ trigger it, but we force them into v0 mode, due to
> b2f73b70b2 (t5512: compensate for v0 only sending HEAD symrefs,
> 2019-02-25). I think we really should be letting ls-remote use the
> protocol it prefers (v2 by default, and v0 if the suite is run in that
> mode), and the expected output should be adjusted based on the mode.
> I'll see if I can do that as well, to make this a two-patch series.

Thanks. I really appreciate your being almost always thorough and wish more contributors took inspirations.