From patchwork Wed Apr 12 06:29:24 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13208550 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 685ABC77B6E for ; Wed, 12 Apr 2023 06:29:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229536AbjDLG32 (ORCPT ); Wed, 12 Apr 2023 02:29:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46956 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229459AbjDLG31 (ORCPT ); Wed, 12 Apr 2023 02:29:27 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9BA324C24 for ; Tue, 11 Apr 2023 23:29:25 -0700 (PDT) Received: (qmail 17837 invoked by uid 109); 12 Apr 2023 06:29:25 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 12 Apr 2023 06:29:25 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 17071 invoked by uid 111); 12 Apr 2023 06:29:24 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 12 Apr 2023 02:29:24 -0400 Authentication-Results: peff.net; auth=none Date: Wed, 12 Apr 2023 02:29:24 -0400 From: Jeff King To: Junio C Hamano Cc: Taylor Blau , Jonas Haag , "brian m. carlson" , git@vger.kernel.org Subject: [PATCH 1/7] v0 protocol: fix infinite loop when parsing multi-valued capabilities Message-ID: <20230412062924.GA1681312@coredump.intra.peff.net> References: <20230412062300.GA838367@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230412062300.GA838367@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org If Git's client-side parsing of an upload-pack response (so git-fetch or ls-remote) sees multiple instances of a single capability, it can enter an infinite loop due to a bug in advancing the "offset" parameter in the parser. This bug can't happen between a client and server of the same Git version. The client bug is in parse_feature_value() when the caller passes in an offset parameter. And that only happens when the v0 protocol is parsing "symref" and "object-format" capabilities, via next_server_feature_value(). But Git has never produced multiple object-format capabilities, and it stopped producing multiple symref values in d007dbf7d6 (Revert "upload-pack: send non-HEAD symbolic refs", 2013-11-18). However, upload-pack did produce multiple symref entries for a while, and they are valid. Plus other implementations, such as Dulwich will still do so. So we should handle them. And even if we do not expect it, it is obviously a bug for the parser to enter an infinite loop. The bug itself is pretty simple. Commit 2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25) added the "offset" parameter, which is used as both an in- and out-parameter. When parsing the first "symref" capability, *offset will be 0 on input, and after parsing the capability, we set *offset to an index just past the value by taking a pointer difference "(value + end) - feature_list". But on the second call, now *offset is set to that larger index, which lets us skip past the first "symref" capability. However, we do so by incrementing feature_list. That means our pointer difference is now too small; it is counting from where we resumed parsing, not from the start of the original feature_list pointer. And because we incremented feature_list only inside our function, and not the caller, that increment is lost next time the function is called. The simplest solution is to account for those skipped bytes by incrementing *offset, rather than assigning to it. (The other possible solution is to add an extra level of pointer indirection to feature_list so that the caller knows we moved it forward, but that seems more complicated). Since the bug can't be reproduced using the current version of git-upload-pack, we'll instead hard-code an input which triggers the problem. Before this patch it loops forever re-parsing the second symref entry. Now we check both that it finishes, and that it parses both entries correctly (a case we could not test at all before). We don't need to worry about testing v2 here; it communicates the capabilities in a completely different way, and doesn't use this code at all. There are tests earlier in t5512 that are meant to cover this (they don't, but we'll address that in a future patch). Reported-by: Jonas Haag Signed-off-by: Jeff King --- connect.c | 4 ++-- t/t5512-ls-remote.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/connect.c b/connect.c index c0c8a38178..5e265ba9d7 100644 --- a/connect.c +++ b/connect.c @@ -616,7 +616,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i if (lenp) *lenp = 0; if (offset) - *offset = found + len - feature_list; + *offset += found + len - feature_list; return value; } /* feature with a value (e.g., "agent=git/1.2.3") */ @@ -628,7 +628,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i if (lenp) *lenp = end; if (offset) - *offset = value + end - feature_list; + *offset += value + end - feature_list; return value; } /* diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh index 20d063fb9a..64dc6fa91c 100755 --- a/t/t5512-ls-remote.sh +++ b/t/t5512-ls-remote.sh @@ -15,6 +15,19 @@ generate_references () { done } +test_expect_success 'set up fake upload-pack' ' + # This can be used to simulate an upload-pack that just shows the + # contents of the "input" file (prepared with the test-tool pkt-line + # helper), and does not do any negotiation (since ls-remote does not + # need it). + write_script cat-input <<-\EOF + # send our initial advertisement/response + cat input + # soak up the flush packet from the client + cat + EOF +' + test_expect_success 'dies when no remote found' ' test_must_fail git ls-remote ' @@ -360,4 +373,35 @@ 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' ' + # Modern versions of Git will not return multiple symref capabilities + # for v0, so we have to hard-code the response. Note that we will + # always use both v0 and object-format=sha1 here, as the hard-coded + # response reflects a server that only supports those. + oid=1234567890123456789012345678901234567890 && + symrefs="symref=refs/remotes/origin/HEAD:refs/remotes/origin/main" && + symrefs="$symrefs symref=HEAD:refs/heads/main" && + + test-tool pkt-line pack >input.q <<-EOF && + $oid HEADQ$symrefs + $oid refs/heads/main + $oid refs/remotes/origin/HEAD + $oid refs/remotes/origin/main + 0000 + EOF + q_to_nul input && + + cat >expect <<-EOF && + ref: refs/heads/main HEAD + $oid HEAD + $oid refs/heads/main + ref: refs/remotes/origin/main refs/remotes/origin/HEAD + $oid refs/remotes/origin/HEAD + $oid refs/remotes/origin/main + EOF + + git ls-remote --symref --upload-pack=./cat-input . >actual && + test_cmp expect actual +' + test_done From patchwork Wed Apr 12 06:29:54 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13208551 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 960E6C77B6E for ; Wed, 12 Apr 2023 06:29:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229585AbjDLG35 (ORCPT ); Wed, 12 Apr 2023 02:29:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47086 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229459AbjDLG34 (ORCPT ); Wed, 12 Apr 2023 02:29:56 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DBF4E4ECD for ; Tue, 11 Apr 2023 23:29:55 -0700 (PDT) Received: (qmail 17856 invoked by uid 109); 12 Apr 2023 06:29:55 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 12 Apr 2023 06:29:55 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 17081 invoked by uid 111); 12 Apr 2023 06:29:54 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 12 Apr 2023 02:29:54 -0400 Authentication-Results: peff.net; auth=none Date: Wed, 12 Apr 2023 02:29:54 -0400 From: Jeff King To: Junio C Hamano Cc: Taylor Blau , Jonas Haag , "brian m. carlson" , git@vger.kernel.org Subject: [PATCH 2/7] t5512: stop referring to "v1" protocol Message-ID: <20230412062954.GB1681312@coredump.intra.peff.net> References: <20230412062300.GA838367@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230412062300.GA838367@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org There really isn't a "v1" Git protocol. It's just v0 with an extra probe which we used to test compatibility in preparation for v2. Any tests that are looking for before/after behavior for v2 really care about "v0". Mentioning "v1" in these tests is just making things more confusing, because we don't care about that probe; we're really testing v0. So let's say so. Signed-off-by: Jeff King --- t/t5512-ls-remote.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh index 64dc6fa91c..ce7a9f1594 100755 --- a/t/t5512-ls-remote.sh +++ b/t/t5512-ls-remote.sh @@ -358,17 +358,17 @@ test_expect_success 'ls-remote --sort fails gracefully outside repository' ' test_expect_success 'ls-remote patterns work with all protocol versions' ' git for-each-ref --format="%(objectname) %(refname)" \ refs/heads/main refs/remotes/origin/main >expect && - git -c protocol.version=1 ls-remote . main >actual.v1 && - test_cmp expect actual.v1 && + git -c protocol.version=0 ls-remote . main >actual.v0 && + test_cmp expect actual.v0 && git -c protocol.version=2 ls-remote . main >actual.v2 && test_cmp expect actual.v2 ' test_expect_success 'ls-remote prefixes work with all protocol versions' ' git for-each-ref --format="%(objectname) %(refname)" \ refs/heads/ refs/tags/ >expect && - git -c protocol.version=1 ls-remote --heads --tags . >actual.v1 && - test_cmp expect actual.v1 && + git -c protocol.version=0 ls-remote --heads --tags . >actual.v0 && + test_cmp expect actual.v0 && git -c protocol.version=2 ls-remote --heads --tags . >actual.v2 && test_cmp expect actual.v2 ' From patchwork Wed Apr 12 06:31:18 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13208552 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id AC240C7619A for ; Wed, 12 Apr 2023 06:31:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229707AbjDLGbX (ORCPT ); Wed, 12 Apr 2023 02:31:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47270 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229459AbjDLGbU (ORCPT ); Wed, 12 Apr 2023 02:31:20 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C061A4C24 for ; Tue, 11 Apr 2023 23:31:19 -0700 (PDT) Received: (qmail 17885 invoked by uid 109); 12 Apr 2023 06:31:19 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 12 Apr 2023 06:31:19 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 17136 invoked by uid 111); 12 Apr 2023 06:31:18 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 12 Apr 2023 02:31:18 -0400 Authentication-Results: peff.net; auth=none Date: Wed, 12 Apr 2023 02:31:18 -0400 From: Jeff King To: Junio C Hamano Cc: Taylor Blau , Jonas Haag , "brian m. carlson" , git@vger.kernel.org Subject: [PATCH 3/7] t5512: stop using jgit for capabilities^{} test Message-ID: <20230412063118.GC1681312@coredump.intra.peff.net> References: <20230412062300.GA838367@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230412062300.GA838367@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Commit eb398797cd (connect: advertized capability is not a ref, 2016-09-09) added support for an upload-pack server responding with: 0000000000000000000000000000000000000000 capabilities^{} followed by a NUL and capabilities. This is not something Git itself has ever produced for upload-pack, but JGit does. And hence the test used JGit to reproduce the real-world situation. That was good for verifying that the incompatibility was fixed, but it's a lousy regression test for a few reasons: - hardly anybody runs it, because you have to have jgit installed - we're depending on jgit's behavior for the test to do anything useful. In particular, this behavior is only relevant to the v0 protocol, but these days we ask for the v2 protocol by default. So for modern jgit, this is probably testing nothing. - it's complicated and slow. We had to do some fifo trickery to handle races, and this one test makes up 40% of the runtime of the total script. Instead, let's just hard-code the response that's of interest to us. That will test exactly what we want for every run. Signed-off-by: Jeff King --- t/t5512-ls-remote.sh | 51 +++++++++++++------------------------------- 1 file changed, 15 insertions(+), 36 deletions(-) diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh index ce7a9f1594..4e21ab60bf 100755 --- a/t/t5512-ls-remote.sh +++ b/t/t5512-ls-remote.sh @@ -300,44 +300,23 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' ' test_cmp expect actual ' -test_lazy_prereq GIT_DAEMON ' - test_bool_env GIT_TEST_GIT_DAEMON true -' - -# This test spawns a daemon, so run it only if the user would be OK with -# testing with git-daemon. -test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' ' - test_set_port JGIT_DAEMON_PORT && - JGIT_DAEMON_PID= && - git init --bare empty.git && - >empty.git/git-daemon-export-ok && - mkfifo jgit_daemon_output && - { - jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output & - JGIT_DAEMON_PID=$! - } && - test_when_finished kill "$JGIT_DAEMON_PID" && - { - read line && - case $line in - Exporting*) - ;; - *) - echo "Expected: Exporting" && - false;; - esac && - read line && - case $line in - "Listening on"*) - ;; - *) - echo "Expected: Listening on" && - false;; - esac - } input.q <<-EOF && + $oid capabilities^{}Qcaps-go-here + 0000 + EOF + q_to_nul input && + # --exit-code asks the command to exit with 2 when no # matching refs are found. - test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git + test_expect_code 2 git ls-remote --exit-code --upload-pack=./cat-input . ' test_expect_success 'ls-remote works outside repository' ' From patchwork Wed Apr 12 06:34:33 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13208557 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EFE64C7619A for ; Wed, 12 Apr 2023 06:34:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229670AbjDLGeh (ORCPT ); Wed, 12 Apr 2023 02:34:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49468 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229575AbjDLGeg (ORCPT ); Wed, 12 Apr 2023 02:34:36 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 446103ABC for ; Tue, 11 Apr 2023 23:34:35 -0700 (PDT) Received: (qmail 17919 invoked by uid 109); 12 Apr 2023 06:34:35 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 12 Apr 2023 06:34:35 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 17144 invoked by uid 111); 12 Apr 2023 06:34:34 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 12 Apr 2023 02:34:34 -0400 Authentication-Results: peff.net; auth=none Date: Wed, 12 Apr 2023 02:34:33 -0400 From: Jeff King To: Junio C Hamano Cc: Taylor Blau , Jonas Haag , "brian m. carlson" , git@vger.kernel.org Subject: [PATCH 4/7] t5512: add v2 support for "ls-remote --symref" test Message-ID: <20230412063433.GD1681312@coredump.intra.peff.net> References: <20230412062300.GA838367@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230412062300.GA838367@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Commit b2f73b70b2 (t5512: compensate for v0 only sending HEAD symrefs, 2019-02-25) configured this test to always run with protocol v0, since the output is different for v2. But that means we are not getting any test coverage of the feature with v2 at all. We could obviously switch to using and expecting v2, but then that leaves v0 behind (and while we don't use it by default, it's still important for testing interoperability with older servers). Likewise, we could switch the expected output based on $GIT_TEST_PROTOCOL_VERSION, but hardly anybody runs the tests for v0 these days. Instead, let's explicitly run it for both protocol versions to make sure they're well behaved. This matches other similar tests added later in 6a139cdd74 (ls-remote: pass heads/tags prefixes to transport, 2018-10-31), etc. Signed-off-by: Jeff King --- I wondered briefly if anybody ever runs with GIT_TEST_PROTOCOL_VERSION=0, but I'm pretty sure the answer is "no", because a bunch of test scripts fail. Those are almost certainly all non-bugs, but rather just tests that assume modern v2 behavior. I'm not sure if it's worth putting any effort into fixing. I didn't do so here (though before and after my patches t5512 itself runs OK in either mode). t/t5512-ls-remote.sh | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh index 4e21ab60bf..74f0204c5b 100755 --- a/t/t5512-ls-remote.sh +++ b/t/t5512-ls-remote.sh @@ -244,22 +244,25 @@ test_expect_success 'protocol v2 supports hiderefs' ' test_expect_success 'ls-remote --symref' ' git fetch origin && - echo "ref: refs/heads/main HEAD" >expect && + echo "ref: refs/heads/main HEAD" >expect.v2 && generate_references \ HEAD \ - refs/heads/main >>expect && + refs/heads/main >>expect.v2 && + echo "ref: refs/remotes/origin/main refs/remotes/origin/HEAD" >>expect.v2 && oid=$(git rev-parse HEAD) && - echo "$oid refs/remotes/origin/HEAD" >>expect && + echo "$oid refs/remotes/origin/HEAD" >>expect.v2 && generate_references \ refs/remotes/origin/main \ refs/tags/mark \ refs/tags/mark1.1 \ refs/tags/mark1.10 \ - refs/tags/mark1.2 >>expect && - # Protocol v2 supports sending symrefs for refs other than HEAD, so use - # protocol v0 here. - GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref >actual && - test_cmp expect actual + refs/tags/mark1.2 >>expect.v2 && + # v0 does not show non-HEAD symrefs + grep -v "ref: refs/remotes" expect.v0 && + git -c protocol.version=0 ls-remote --symref >actual.v0 && + test_cmp expect.v0 actual.v0 && + git -c protocol.version=2 ls-remote --symref >actual.v2 && + test_cmp expect.v2 actual.v2 ' test_expect_success 'ls-remote with filtered symref (refname)' ' From patchwork Wed Apr 12 06:35:43 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13208558 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1447EC77B6E for ; Wed, 12 Apr 2023 06:35:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229742AbjDLGfq (ORCPT ); Wed, 12 Apr 2023 02:35:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49762 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229531AbjDLGfp (ORCPT ); Wed, 12 Apr 2023 02:35:45 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 63C323ABC for ; Tue, 11 Apr 2023 23:35:44 -0700 (PDT) Received: (qmail 17938 invoked by uid 109); 12 Apr 2023 06:35:44 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 12 Apr 2023 06:35:44 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 17179 invoked by uid 111); 12 Apr 2023 06:35:43 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 12 Apr 2023 02:35:43 -0400 Authentication-Results: peff.net; auth=none Date: Wed, 12 Apr 2023 02:35:43 -0400 From: Jeff King To: Junio C Hamano Cc: Taylor Blau , Jonas Haag , "brian m. carlson" , git@vger.kernel.org Subject: [PATCH 5/7] t5512: allow any protocol version for filtered symref test Message-ID: <20230412063543.GE1681312@coredump.intra.peff.net> References: <20230412062300.GA838367@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230412062300.GA838367@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org We have a test that checks that ls-remote, when asked only about HEAD, will report the HEAD symref, and not others. This was marked to always run with the v0 protocol by b2f73b70b2 (t5512: compensate for v0 only sending HEAD symrefs, 2019-02-25). But in v0 this test is doing nothing! For v0, upload-pack only reports the HEAD symref anyway, so we'd never have any other symref to report. For v2, it is useful; we learn about all symrefs (and the test repo has multiple), so this demonstrates that we correctly avoid showing them. We could perhaps mark this to test explicitly with v2, but since that is the default these days, it's sufficient to just run ls-remote without any protocol specification. It still passes if somebody does an explicit GIT_TEST_PROTOCOL_VERSION=0; it's just testing less. Signed-off-by: Jeff King --- t/t5512-ls-remote.sh | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh index 74f0204c5b..1ac3b50ca5 100755 --- a/t/t5512-ls-remote.sh +++ b/t/t5512-ls-remote.sh @@ -271,9 +271,7 @@ test_expect_success 'ls-remote with filtered symref (refname)' ' ref: refs/heads/main HEAD $rev HEAD EOF - # Protocol v2 supports sending symrefs for refs other than HEAD, so use - # protocol v0 here. - GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref . HEAD >actual && + git ls-remote --symref . HEAD >actual && test_cmp expect actual ' From patchwork Wed Apr 12 06:37:19 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13208559 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id BEC9EC77B6E for ; Wed, 12 Apr 2023 06:37:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229769AbjDLGhY (ORCPT ); Wed, 12 Apr 2023 02:37:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50590 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229486AbjDLGhW (ORCPT ); Wed, 12 Apr 2023 02:37:22 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 33AAD3ABC for ; Tue, 11 Apr 2023 23:37:20 -0700 (PDT) Received: (qmail 17962 invoked by uid 109); 12 Apr 2023 06:37:20 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 12 Apr 2023 06:37:20 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 17186 invoked by uid 111); 12 Apr 2023 06:37:20 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 12 Apr 2023 02:37:20 -0400 Authentication-Results: peff.net; auth=none Date: Wed, 12 Apr 2023 02:37:19 -0400 From: Jeff King To: Junio C Hamano Cc: Taylor Blau , Jonas Haag , "brian m. carlson" , git@vger.kernel.org Subject: [PATCH 6/7] t5512: test "ls-remote --heads --symref" filtering with v0 and v2 Message-ID: <20230412063719.GF1681312@coredump.intra.peff.net> References: <20230412062300.GA838367@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230412062300.GA838367@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org We have two overlapping tests for checking the behavior of "ls-remote --symref" when filtering output. The first test checks that using "--heads" will omit the symref for HEAD (since we don't print anything about HEAD at all), but still prints other symrefs. This has been marked as expecting failure since it was added in 99c08d4eb2 (ls-remote: add support for showing symrefs, 2016-01-19). That's because back then, we only had the v0 protocol, and it only reported on the HEAD symref, not others. But these days we have v2, which does exactly what the test wants. It would even have started unexpectedly passing when we switched to v2 by default, except that b2f73b70b2 (t5512: compensate for v0 only sending HEAD symrefs, 2019-02-25) over-zealously marked it to run only in v0 mode. So let's run it with both protocol versions, and adjust the expected output for each. It passes in v2 without modification. In v0 mode, we'll drop the extra symref, but this is still testing something useful: it ensures that we do omit HEAD. The test after this checks "--heads" again, this time using the expected v0 output. That's now redundant. It also checks that limiting with a pattern like "refs/heads/*" works similarly, but that's redundant with a test earlier in the script which limits by HEAD (again, back then the "HEAD" test was less interesting because there were no other symrefs to omit, but in a modern v2 world, there are). So we can just delete that second test entirely. Signed-off-by: Jeff King --- t/t5512-ls-remote.sh | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh index 1ac3b50ca5..92cb9fed78 100755 --- a/t/t5512-ls-remote.sh +++ b/t/t5512-ls-remote.sh @@ -275,30 +275,18 @@ test_expect_success 'ls-remote with filtered symref (refname)' ' test_cmp expect actual ' -test_expect_failure 'ls-remote with filtered symref (--heads)' ' +test_expect_success 'ls-remote with filtered symref (--heads)' ' git symbolic-ref refs/heads/foo refs/tags/mark && - cat >expect <<-EOF && + cat >expect.v2 <<-EOF && ref: refs/tags/mark refs/heads/foo $rev refs/heads/foo $rev refs/heads/main EOF - # Protocol v2 supports sending symrefs for refs other than HEAD, so use - # protocol v0 here. - GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual && - test_cmp expect actual -' - -test_expect_success 'ls-remote --symref omits filtered-out matches' ' - cat >expect <<-EOF && - $rev refs/heads/foo - $rev refs/heads/main - EOF - # Protocol v2 supports sending symrefs for refs other than HEAD, so use - # protocol v0 here. - GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual && - test_cmp expect actual && - GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref . "refs/heads/*" >actual && - test_cmp expect actual + grep -v "^ref: refs/tags/" expect.v0 && + git -c protocol.version=0 ls-remote --symref --heads . >actual.v0 && + test_cmp expect.v0 actual.v0 && + git -c protocol.version=2 ls-remote --symref --heads . >actual.v2 && + test_cmp expect.v2 actual.v2 ' test_expect_success 'indicate no refs in v0 standards-compliant empty remote' ' From patchwork Wed Apr 12 06:40:45 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13208569 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7545DC77B6F for ; Wed, 12 Apr 2023 06:40:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229773AbjDLGkt (ORCPT ); Wed, 12 Apr 2023 02:40:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52536 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229573AbjDLGks (ORCPT ); Wed, 12 Apr 2023 02:40:48 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1BD532683 for ; Tue, 11 Apr 2023 23:40:46 -0700 (PDT) Received: (qmail 17991 invoked by uid 109); 12 Apr 2023 06:40:46 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Wed, 12 Apr 2023 06:40:46 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 17219 invoked by uid 111); 12 Apr 2023 06:40:45 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Wed, 12 Apr 2023 02:40:45 -0400 Authentication-Results: peff.net; auth=none Date: Wed, 12 Apr 2023 02:40:45 -0400 From: Jeff King To: Junio C Hamano Cc: Taylor Blau , Jonas Haag , "brian m. carlson" , git@vger.kernel.org Subject: [PATCH 7/7] v0 protocol: use size_t for capability length/offset Message-ID: <20230412064045.GA1681575@coredump.intra.peff.net> References: <20230412062300.GA838367@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230412062300.GA838367@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org When parsing server capabilities, we use "int" to store lengths and offsets. At first glance this seems like a spot where our parser may be confused by integer overflow if somebody sent us a malicious response. In practice these strings are all bounded by the 64k limit of a pkt-line, so using "int" is OK. However, it makes the code simpler to audit if they just use size_t everywhere. Note that because we take these parameters as pointers, this also forces many callers to update their declared types. Signed-off-by: Jeff King --- I guess you could argue that "int" is fine here, given the context. Mostly I just did a double-take while looking at the code, so I thought this might save the next person from doing the same. builtin/receive-pack.c | 2 +- connect.c | 22 +++++++++++----------- connect.h | 4 ++-- fetch-pack.c | 4 ++-- send-pack.c | 2 +- transport.c | 2 +- upload-pack.c | 2 +- 7 files changed, 19 insertions(+), 19 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 9109552533..3b495ecc84 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -2093,7 +2093,7 @@ static struct command *read_head_info(struct packet_reader *reader, const char *feature_list = reader->line + linelen + 1; const char *hash = NULL; const char *client_sid; - int len = 0; + size_t len = 0; if (parse_feature_request(feature_list, "report-status")) report_status = 1; if (parse_feature_request(feature_list, "report-status-v2")) diff --git a/connect.c b/connect.c index 5e265ba9d7..5685551cf0 100644 --- a/connect.c +++ b/connect.c @@ -22,7 +22,7 @@ static char *server_capabilities_v1; static struct strvec server_capabilities_v2 = STRVEC_INIT; -static const char *next_server_feature_value(const char *feature, int *len, int *offset); +static const char *next_server_feature_value(const char *feature, size_t *len, size_t *offset); static int check_ref(const char *name, unsigned int flags) { @@ -205,10 +205,10 @@ static void parse_one_symref_info(struct string_list *symref, const char *val, i static void annotate_refs_with_symref_info(struct ref *ref) { struct string_list symref = STRING_LIST_INIT_DUP; - int offset = 0; + size_t offset = 0; while (1) { - int len; + size_t len; const char *val; val = next_server_feature_value("symref", &len, &offset); @@ -231,7 +231,7 @@ static void annotate_refs_with_symref_info(struct ref *ref) static void process_capabilities(struct packet_reader *reader, int *linelen) { const char *feat_val; - int feat_len; + size_t feat_len; const char *line = reader->line; int nul_location = strlen(line); if (nul_location == *linelen) @@ -595,9 +595,9 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader, return list; } -const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset) +const char *parse_feature_value(const char *feature_list, const char *feature, size_t *lenp, size_t *offset) { - int len; + size_t len; if (!feature_list) return NULL; @@ -621,7 +621,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i } /* feature with a value (e.g., "agent=git/1.2.3") */ else if (*value == '=') { - int end; + size_t end; value++; end = strcspn(value, " \t\n"); @@ -643,8 +643,8 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i int server_supports_hash(const char *desired, int *feature_supported) { - int offset = 0; - int len; + size_t offset = 0; + size_t len; const char *hash; hash = next_server_feature_value("object-format", &len, &offset); @@ -668,12 +668,12 @@ int parse_feature_request(const char *feature_list, const char *feature) return !!parse_feature_value(feature_list, feature, NULL, NULL); } -static const char *next_server_feature_value(const char *feature, int *len, int *offset) +static const char *next_server_feature_value(const char *feature, size_t *len, size_t *offset) { return parse_feature_value(server_capabilities_v1, feature, len, offset); } -const char *server_feature_value(const char *feature, int *len) +const char *server_feature_value(const char *feature, size_t *len) { return parse_feature_value(server_capabilities_v1, feature, len, NULL); } diff --git a/connect.h b/connect.h index f41a0b4c1f..1645126c17 100644 --- a/connect.h +++ b/connect.h @@ -12,14 +12,14 @@ int finish_connect(struct child_process *conn); int git_connection_is_socket(struct child_process *conn); int server_supports(const char *feature); int parse_feature_request(const char *features, const char *feature); -const char *server_feature_value(const char *feature, int *len_ret); +const char *server_feature_value(const char *feature, size_t *len_ret); int url_is_local_not_ssh(const char *url); struct packet_reader; enum protocol_version discover_version(struct packet_reader *reader); int server_supports_hash(const char *desired, int *feature_supported); -const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset); +const char *parse_feature_value(const char *feature_list, const char *feature, size_t *lenp, size_t *offset); int server_supports_v2(const char *c); void ensure_server_supports_v2(const char *c); int server_feature_v2(const char *c, const char **v); diff --git a/fetch-pack.c b/fetch-pack.c index 368f2ed25a..97a44ed582 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1099,7 +1099,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args, struct ref *ref = copy_ref_list(orig_ref); struct object_id oid; const char *agent_feature; - int agent_len; + size_t agent_len; struct fetch_negotiator negotiator_alloc; struct fetch_negotiator *negotiator; @@ -1117,7 +1117,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args, agent_supported = 1; if (agent_len) print_verbose(args, _("Server version is %.*s"), - agent_len, agent_feature); + (int)agent_len, agent_feature); } if (!server_supports("session-id")) diff --git a/send-pack.c b/send-pack.c index f81bbb28d7..97344b629e 100644 --- a/send-pack.c +++ b/send-pack.c @@ -538,7 +538,7 @@ int send_pack(struct send_pack_args *args, die(_("the receiving end does not support this repository's hash algorithm")); if (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) { - int len; + size_t len; push_cert_nonce = server_feature_value("push-cert", &len); if (push_cert_nonce) { reject_invalid_nonce(push_cert_nonce, len); diff --git a/transport.c b/transport.c index 89a220425e..6223dc3de2 100644 --- a/transport.c +++ b/transport.c @@ -317,7 +317,7 @@ static struct ref *handshake(struct transport *transport, int for_push, struct git_transport_data *data = transport->data; struct ref *refs = NULL; struct packet_reader reader; - int sid_len; + size_t sid_len; const char *server_sid; connect_setup(transport, for_push); diff --git a/upload-pack.c b/upload-pack.c index e23f16dfdd..565e46058f 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1067,7 +1067,7 @@ static void receive_needs(struct upload_pack_data *data, const char *features; struct object_id oid_buf; const char *arg; - int feature_len; + size_t feature_len; reset_timeout(data->timeout); if (packet_reader_read(reader) != PACKET_READ_NORMAL)