From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: Taylor Blau <me@ttaylorr•com>, Jonas Haag <jonas@lophus•org>,
"brian m. carlson" <sandals@crustytoothpaste•net>,
git@vger•kernel.org
Subject: Re: Infinite loop + memory leak in annotate_refs_with_symref_info
Date: Tue, 11 Apr 2023 15:52:43 -0700 [thread overview]
Message-ID: <xmqqa5ze9glg.fsf@gitster.g> (raw)
In-Reply-To: <20230411215845.GA678138@coredump.intra.peff.net> (Jeff King's message of "Tue, 11 Apr 2023 17:58:45 -0400")
Jeff King <peff@peff•net> writes:
> 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.
> 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" &&
> 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.
next prev parent reply other threads:[~2023-04-11 22:52 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-11 19:53 Infinite loop + memory leak in annotate_refs_with_symref_info Jonas Haag
2023-04-11 20:25 ` Taylor Blau
2023-04-11 23:59 ` Taylor Blau
2023-04-12 0:53 ` brian m. carlson
2023-04-11 21:06 ` Jeff King
2023-04-11 21:16 ` Jeff King
2023-04-11 21:22 ` Taylor Blau
2023-04-11 21:58 ` Jeff King
2023-04-11 22:52 ` Junio C Hamano [this message]
2023-04-12 6:23 ` [PATCH 0/7] v0 multiple-symref infinite loop fix and test cleanup Jeff King
2023-04-12 6:29 ` [PATCH 1/7] v0 protocol: fix infinite loop when parsing multi-valued capabilities Jeff King
2023-04-12 6:46 ` Jeff King
2023-04-12 7:25 ` [PATCH v2 " Jeff King
2023-04-12 7:26 ` Jeff King
2023-04-12 6:29 ` [PATCH 2/7] t5512: stop referring to "v1" protocol Jeff King
2023-04-12 6:31 ` [PATCH 3/7] t5512: stop using jgit for capabilities^{} test Jeff King
2023-04-12 9:04 ` Jeff King
2023-04-14 21:24 ` [PATCH v3 0/7] v0 multiple-symref infinite loop fix and test cleanup Jeff King
2023-04-14 21:24 ` [PATCH v3 1/7] v0 protocol: fix infinite loop when parsing multi-valued capabilities Jeff King
2023-04-14 21:24 ` [PATCH v3 2/7] t5512: stop referring to "v1" protocol Jeff King
2023-04-14 21:25 ` [PATCH v3 3/7] v0 protocol: fix sha1/sha256 confusion for capabilities^{} Jeff King
2023-04-14 21:25 ` [PATCH v3 4/7] t5512: add v2 support for "ls-remote --symref" test Jeff King
2023-04-14 21:25 ` [PATCH v3 5/7] t5512: allow any protocol version for filtered symref test Jeff King
2023-04-14 21:25 ` [PATCH v3 6/7] t5512: test "ls-remote --heads --symref" filtering with v0 and v2 Jeff King
2023-04-14 21:25 ` [PATCH v3 7/7] v0 protocol: use size_t for capability length/offset Jeff King
2023-04-17 16:06 ` [PATCH v3 0/7] v0 multiple-symref infinite loop fix and test cleanup Junio C Hamano
2023-04-12 6:34 ` [PATCH 4/7] t5512: add v2 support for "ls-remote --symref" test Jeff King
2023-04-12 6:35 ` [PATCH 5/7] t5512: allow any protocol version for filtered symref test Jeff King
2023-04-12 6:37 ` [PATCH 6/7] t5512: test "ls-remote --heads --symref" filtering with v0 and v2 Jeff King
2023-04-12 6:40 ` [PATCH 7/7] v0 protocol: use size_t for capability length/offset Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqa5ze9glg.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jonas@lophus$(echo .)org \
--cc=me@ttaylorr$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=sandals@crustytoothpaste$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox