From patchwork Sat Apr 13 05:52:18 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10899297 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 1F2A81800 for ; Sat, 13 Apr 2019 05:52:23 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0301628EA5 for ; Sat, 13 Apr 2019 05:52:23 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E7E4D28EAE; Sat, 13 Apr 2019 05:52:22 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 95F1C28EA5 for ; Sat, 13 Apr 2019 05:52:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726285AbfDMFwU (ORCPT ); Sat, 13 Apr 2019 01:52:20 -0400 Received: from cloud.peff.net ([104.130.231.41]:57176 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726175AbfDMFwT (ORCPT ); Sat, 13 Apr 2019 01:52:19 -0400 Received: (qmail 27695 invoked by uid 109); 13 Apr 2019 05:52:21 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Sat, 13 Apr 2019 05:52:21 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 12564 invoked by uid 111); 13 Apr 2019 05:52:49 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Sat, 13 Apr 2019 01:52:49 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Sat, 13 Apr 2019 01:52:18 -0400 Date: Sat, 13 Apr 2019 01:52:18 -0400 From: Jeff King To: git@vger.kernel.org Cc: Jonathan Tan Subject: [PATCH 1/7] t5516: drop ok=sigpipe from unreachable-want tests Message-ID: <20190413055217.GA19495@sigill.intra.peff.net> References: <20190413055127.GA32340@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190413055127.GA32340@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP We annotated our test_must_fail calls in 8bf4becf0c (add "ok=sigpipe" to test_must_fail and use it to fix flaky tests, 2015-11-27) because the abrupt hangup of the server meant that we'd sometimes fail on read() and sometimes get SIGPIPE on write(). But since 143588949c (fetch: ignore SIGPIPE during network operation, 2019-03-03), we make sure that we end up with a real die(), and our tests no longer need to work around the race. Signed-off-by: Jeff King --- This is actually the only user of ok=sigpipe, so we could drop the feature. It might come in handy later, though. t/t5516-fetch-push.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 37e8e80893..fc3961ec22 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1232,15 +1232,15 @@ do mk_empty shallow && ( cd shallow && - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 && - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 && + test_must_fail git fetch ../testrepo/.git $SHA1_3 && + test_must_fail git fetch ../testrepo/.git $SHA1_1 && git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true && git fetch ../testrepo/.git $SHA1_1 && git cat-file commit $SHA1_1 && test_must_fail git cat-file commit $SHA1_2 && git fetch ../testrepo/.git $SHA1_2 && git cat-file commit $SHA1_2 && - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 + test_must_fail git fetch ../testrepo/.git $SHA1_3 ) ' done From patchwork Sat Apr 13 05:53:09 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10899299 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id DD4191515 for ; Sat, 13 Apr 2019 05:53:13 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BC7F728EA5 for ; Sat, 13 Apr 2019 05:53:13 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B08A028EAE; Sat, 13 Apr 2019 05:53:13 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 57F8E28EA8 for ; Sat, 13 Apr 2019 05:53:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726918AbfDMFxL (ORCPT ); Sat, 13 Apr 2019 01:53:11 -0400 Received: from cloud.peff.net ([104.130.231.41]:57184 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726175AbfDMFxL (ORCPT ); Sat, 13 Apr 2019 01:53:11 -0400 Received: (qmail 27713 invoked by uid 109); 13 Apr 2019 05:53:12 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Sat, 13 Apr 2019 05:53:12 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 12585 invoked by uid 111); 13 Apr 2019 05:53:40 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Sat, 13 Apr 2019 01:53:40 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Sat, 13 Apr 2019 01:53:09 -0400 Date: Sat, 13 Apr 2019 01:53:09 -0400 From: Jeff King To: git@vger.kernel.org Cc: Jonathan Tan Subject: [PATCH 2/7] t5530: check protocol response for "not our ref" Message-ID: <20190413055309.GB19495@sigill.intra.peff.net> References: <20190413055127.GA32340@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190413055127.GA32340@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Back in 9f9aa76130 (upload-pack: Improve error message when bad ref requested, 2010-07-31), we added a test to make sure that we die with a sensible message when the client asks for an object we don't have. Much later, in bdb31eada7 (upload-pack: report "not our ref" to client, 2017-02-23), we started reporting that information via an "ERR" line in the protocol. Let's check that part, as well. While we're touching this test, let's drop the "-q" on the grep calls. Our usual test style just relies on --verbose to control output. Signed-off-by: Jeff King --- If like me, you are scratching your head over the grep for multi_ack_detailed, it is from 9f9aa76130, which made sure we did not spew extra cruft as part of the die message. t/t5530-upload-pack-error.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh index 4f6e32b04c..295bd0c83c 100755 --- a/t/t5530-upload-pack-error.sh +++ b/t/t5530-upload-pack-error.sh @@ -62,8 +62,9 @@ test_expect_success 'upload-pack error message when bad ref requested' ' printf "0045want %s multi_ack_detailed\n00000009done\n0000" \ "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" >input && test_must_fail git upload-pack . output 2>output.err && - grep -q "not our ref" output.err && - ! grep -q multi_ack_detailed output.err + grep "not our ref" output.err && + grep "ERR" output && + ! grep multi_ack_detailed output.err ' test_expect_success 'upload-pack fails due to error in pack-objects enumeration' ' From patchwork Sat Apr 13 05:53:34 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10899301 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 619021515 for ; Sat, 13 Apr 2019 05:53:39 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3FC2028EA5 for ; Sat, 13 Apr 2019 05:53:39 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3374928EAE; Sat, 13 Apr 2019 05:53:39 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A089228EA5 for ; Sat, 13 Apr 2019 05:53:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726922AbfDMFxh (ORCPT ); Sat, 13 Apr 2019 01:53:37 -0400 Received: from cloud.peff.net ([104.130.231.41]:57192 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726175AbfDMFxh (ORCPT ); Sat, 13 Apr 2019 01:53:37 -0400 Received: (qmail 27724 invoked by uid 109); 13 Apr 2019 05:53:37 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Sat, 13 Apr 2019 05:53:37 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 12602 invoked by uid 111); 13 Apr 2019 05:54:05 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Sat, 13 Apr 2019 01:54:05 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Sat, 13 Apr 2019 01:53:34 -0400 Date: Sat, 13 Apr 2019 01:53:34 -0400 From: Jeff King To: git@vger.kernel.org Cc: Jonathan Tan Subject: [PATCH 3/7] upload-pack: send ERR packet for non-tip objects Message-ID: <20190413055334.GC19495@sigill.intra.peff.net> References: <20190413055127.GA32340@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190413055127.GA32340@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Commit bdb31eada7 (upload-pack: report "not our ref" to client, 2017-02-23) catches the case where a client asks for an object we don't have, and issues a message that the client can show to the user (in addition to dying and writing to stderr). There's a similar case (with the same message) when the client asks for an object which we _do_ have, but which isn't a ref tip (or isn't reachable, when uploadpack.allowReachableSHA1InWant is true). Let's give that one the same treatment, for the same reason (namely that it's more informative to the client than just hanging up, since they won't see our stderr over some protocols). There are two tests here. We cover it most directly in t5530 by invoking upload-pack, which matches the existing "not our ref" test. But a more end-to-end check is that "git fetch" actually shows the message to the client. We're already checking in t5516 that this case fails, so we can just check stderr there, too. Note that even after we started ignoring SIGPIPE in 8bf4becf0c, this could in theory still be racy as described in that commit (because we die() on write failures before pumping the connection for any ERR packets). In practice this should be OK for this case. The server will not actually check reachability until it has received our whole group of "want" lines. And since we have no objects in the repository, we won't send any "have" lines, meaning we're always waiting to read the server response. Note also that this case cannot happen in the v2 protocol, since it allows any available object to be requested. However, we don't have to take any steps to protect against the upcoming GIT_TEST_PROTOCOL_VERSION in our tests: - the tests in t5516 would already need to be skipped under v2, and that is covered by ab0c5f5096 (tests: always test fetch of unreachable with v0, 2019-02-25) - the tests in t5530 invoke upload-pack directly, which will continue to default to v0. Eventually we may have a test setting which uses v2 even for bare upload-pack calls, but we can't override it here until we know what the setting looks like. Signed-off-by: Jeff King --- t/t5516-fetch-push.sh | 3 ++- t/t5530-upload-pack-error.sh | 13 ++++++++++++- upload-pack.c | 11 ++++++++--- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index fc3961ec22..747dc4c31d 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1240,7 +1240,8 @@ do test_must_fail git cat-file commit $SHA1_2 && git fetch ../testrepo/.git $SHA1_2 && git cat-file commit $SHA1_2 && - test_must_fail git fetch ../testrepo/.git $SHA1_3 + test_must_fail git fetch ../testrepo/.git $SHA1_3 2>err && + test_i18ngrep "remote error:.*not our ref" err ) ' done diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh index 295bd0c83c..a1d3031d40 100755 --- a/t/t5530-upload-pack-error.sh +++ b/t/t5530-upload-pack-error.sh @@ -57,7 +57,7 @@ test_expect_success 'upload-pack fails due to error in rev-list' ' grep "bad tree object" output.err ' -test_expect_success 'upload-pack error message when bad ref requested' ' +test_expect_success 'upload-pack fails due to bad want (no object)' ' printf "0045want %s multi_ack_detailed\n00000009done\n0000" \ "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" >input && @@ -67,6 +67,17 @@ test_expect_success 'upload-pack error message when bad ref requested' ' ! grep multi_ack_detailed output.err ' +test_expect_success 'upload-pack fails due to bad want (not tip)' ' + + oid=$(echo an object we have | git hash-object -w --stdin) && + printf "0045want %s multi_ack_detailed\n00000009done\n0000" \ + "$oid" >input && + test_must_fail git upload-pack . output 2>output.err && + grep "not our ref" output.err && + grep "ERR" output && + ! grep multi_ack_detailed output.err +' + test_expect_success 'upload-pack fails due to error in pack-objects enumeration' ' printf "0032want %s\n00000009done\n0000" \ diff --git a/upload-pack.c b/upload-pack.c index d098ef5982..cb603a6d8a 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -592,7 +592,8 @@ static int has_unreachable(struct object_array *src) return 1; } -static void check_non_tip(struct object_array *want_obj) +static void check_non_tip(struct object_array *want_obj, + struct packet_writer *writer) { int i; @@ -611,9 +612,13 @@ static void check_non_tip(struct object_array *want_obj) /* Pick one of them (we know there at least is one) */ for (i = 0; i < want_obj->nr; i++) { struct object *o = want_obj->objects[i].item; - if (!is_our_ref(o)) + if (!is_our_ref(o)) { + packet_writer_error(writer, + "upload-pack: not our ref %s", + oid_to_hex(&o->oid)); die("git upload-pack: not our ref %s", oid_to_hex(&o->oid)); + } } } @@ -936,7 +941,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan * by another process that handled the initial request. */ if (has_non_tip) - check_non_tip(want_obj); + check_non_tip(want_obj, &writer); if (!use_sideband && daemon_mode) no_progress = 1; From patchwork Sat Apr 13 05:54:02 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10899303 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 8EAFF1515 for ; Sat, 13 Apr 2019 05:54:06 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6F5EF28EA5 for ; Sat, 13 Apr 2019 05:54:06 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 5E97228EB3; Sat, 13 Apr 2019 05:54:06 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DFC9928EA5 for ; Sat, 13 Apr 2019 05:54:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726932AbfDMFyE (ORCPT ); Sat, 13 Apr 2019 01:54:04 -0400 Received: from cloud.peff.net ([104.130.231.41]:57200 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726175AbfDMFyE (ORCPT ); Sat, 13 Apr 2019 01:54:04 -0400 Received: (qmail 27743 invoked by uid 109); 13 Apr 2019 05:54:05 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Sat, 13 Apr 2019 05:54:05 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 12619 invoked by uid 111); 13 Apr 2019 05:54:33 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Sat, 13 Apr 2019 01:54:33 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Sat, 13 Apr 2019 01:54:02 -0400 Date: Sat, 13 Apr 2019 01:54:02 -0400 From: Jeff King To: git@vger.kernel.org Cc: Jonathan Tan , Masaya Suzuki Subject: [PATCH 4/7] pkt-line: prepare buffer before handling ERR packets Message-ID: <20190413055401.GD19495@sigill.intra.peff.net> References: <20190413055127.GA32340@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190413055127.GA32340@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Since 2d103c31c2 (pack-protocol.txt: accept error packets in any context, 2018-12-29), the pktline code will detect an ERR packet and die automatically, saving the caller from dealing with it. But we do so too early in the function, before we have terminated the buffer with a NUL. As a result, passing the ERR message to die() may result in us printing random cruft from a previous packet. This doesn't trigger memory tools like ASan because we reuse the same buffer over and over (so the contents are valid and initialized; they're just stale). We can see demonstrate this by tightening the regex we use to match the error message in t5516; without this patch, git-fetch will accidentally print the capabilities from the (much longer) initial packet we received. By moving the ERR code later in the function we get a few other benefits, too: - we'll now chomp any newline sent by the other side (which is what we want, since die() will add its own newline) - we'll now mention the ERR packet with GIT_TRACE_PACKET Signed-off-by: Jeff King --- pkt-line.c | 9 +++++---- t/t5516-fetch-push.sh | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/pkt-line.c b/pkt-line.c index ffd7220544..c9ed780d0b 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -350,16 +350,17 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer, return PACKET_READ_EOF; } - if ((options & PACKET_READ_DIE_ON_ERR_PACKET) && - starts_with(buffer, "ERR ")) - die(_("remote error: %s"), buffer + 4); - if ((options & PACKET_READ_CHOMP_NEWLINE) && len && buffer[len-1] == '\n') len--; buffer[len] = 0; packet_trace(buffer, len, 0); + + if ((options & PACKET_READ_DIE_ON_ERR_PACKET) && + starts_with(buffer, "ERR ")) + die(_("remote error: %s"), buffer + 4); + *pktlen = len; return PACKET_READ_NORMAL; } diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 747dc4c31d..98ef71b48c 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1241,7 +1241,7 @@ do git fetch ../testrepo/.git $SHA1_2 && git cat-file commit $SHA1_2 && test_must_fail git fetch ../testrepo/.git $SHA1_3 2>err && - test_i18ngrep "remote error:.*not our ref" err + test_i18ngrep "remote error:.*not our ref.*$SHA1_3\$" err ) ' done From patchwork Sat Apr 13 05:54:09 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10899305 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id ECFF21800 for ; Sat, 13 Apr 2019 05:54:13 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D182928EA5 for ; Sat, 13 Apr 2019 05:54:13 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C4C8F28EAE; Sat, 13 Apr 2019 05:54:13 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6902828EA5 for ; Sat, 13 Apr 2019 05:54:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726937AbfDMFyM (ORCPT ); Sat, 13 Apr 2019 01:54:12 -0400 Received: from cloud.peff.net ([104.130.231.41]:57210 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726175AbfDMFyL (ORCPT ); Sat, 13 Apr 2019 01:54:11 -0400 Received: (qmail 27752 invoked by uid 109); 13 Apr 2019 05:54:13 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Sat, 13 Apr 2019 05:54:13 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 12637 invoked by uid 111); 13 Apr 2019 05:54:41 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Sat, 13 Apr 2019 01:54:41 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Sat, 13 Apr 2019 01:54:09 -0400 Date: Sat, 13 Apr 2019 01:54:09 -0400 From: Jeff King To: git@vger.kernel.org Cc: Jonathan Tan Subject: [PATCH 5/7] fetch: use free_refs() Message-ID: <20190413055409.GE19495@sigill.intra.peff.net> References: <20190413055127.GA32340@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190413055127.GA32340@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP There's no need for us to write this loop manually when a helper function can already do it. Signed-off-by: Jeff King --- fetch-pack.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index e69993b2eb..a181d3401d 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -630,10 +630,7 @@ static void filter_refs(struct fetch_pack_args *args, } oidset_clear(&tip_oids); - for (ref = unmatched; ref; ref = next) { - next = ref->next; - free(ref); - } + free_refs(unmatched); *refs = newlist; } From patchwork Sat Apr 13 05:54:31 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10899307 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id CEB771515 for ; Sat, 13 Apr 2019 05:54:34 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B376B28EA5 for ; Sat, 13 Apr 2019 05:54:34 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A7C0328EAE; Sat, 13 Apr 2019 05:54:34 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4D8A928EA5 for ; Sat, 13 Apr 2019 05:54:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726953AbfDMFyd (ORCPT ); Sat, 13 Apr 2019 01:54:33 -0400 Received: from cloud.peff.net ([104.130.231.41]:57218 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726944AbfDMFyc (ORCPT ); Sat, 13 Apr 2019 01:54:32 -0400 Received: (qmail 27766 invoked by uid 109); 13 Apr 2019 05:54:34 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Sat, 13 Apr 2019 05:54:34 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 12654 invoked by uid 111); 13 Apr 2019 05:55:02 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Sat, 13 Apr 2019 01:55:02 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Sat, 13 Apr 2019 01:54:31 -0400 Date: Sat, 13 Apr 2019 01:54:31 -0400 From: Jeff King To: git@vger.kernel.org Cc: Jonathan Tan Subject: [PATCH 6/7] remote.c: make singular free_ref() public Message-ID: <20190413055430.GF19495@sigill.intra.peff.net> References: <20190413055127.GA32340@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190413055127.GA32340@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP We provide a free_refs() function to free a list, but there's no easy way for a caller to free a single ref. Let's make our singular free_ref() function public. Since its name is so similar to the list-freeing free_refs(), and because both of those functions have the same signature, it might be easy to accidentally use the wrong one. Let's call the singular version the more verbose "free_one_ref()" to distinguish it. Signed-off-by: Jeff King --- remote.c | 6 +++--- remote.h | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/remote.c b/remote.c index 9cc3b07d21..3fe34eae85 100644 --- a/remote.c +++ b/remote.c @@ -820,11 +820,11 @@ struct ref *copy_ref_list(const struct ref *ref) return ret; } -static void free_ref(struct ref *ref) +void free_one_ref(struct ref *ref) { if (!ref) return; - free_ref(ref->peer_ref); + free_one_ref(ref->peer_ref); free(ref->remote_status); free(ref->symref); free(ref); @@ -835,7 +835,7 @@ void free_refs(struct ref *ref) struct ref *next; while (ref) { next = ref->next; - free_ref(ref); + free_one_ref(ref); ref = next; } } diff --git a/remote.h b/remote.h index da53ad570b..f58332a27e 100644 --- a/remote.h +++ b/remote.h @@ -131,8 +131,10 @@ int ref_compare_name(const void *, const void *); int check_ref_type(const struct ref *ref, int flags); /* - * Frees the entire list and peers of elements. + * Free a single ref and its peer, or an entire list of refs and their peers, + * respectively. */ +void free_one_ref(struct ref *ref); void free_refs(struct ref *ref); struct oid_array; From patchwork Sat Apr 13 05:57:37 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10899311 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 14EF71515 for ; Sat, 13 Apr 2019 05:57:42 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C056228EA5 for ; Sat, 13 Apr 2019 05:57:41 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B0CF728EAE; Sat, 13 Apr 2019 05:57:41 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 385F928EA5 for ; Sat, 13 Apr 2019 05:57:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726278AbfDMF5j (ORCPT ); Sat, 13 Apr 2019 01:57:39 -0400 Received: from cloud.peff.net ([104.130.231.41]:57224 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1725776AbfDMF5j (ORCPT ); Sat, 13 Apr 2019 01:57:39 -0400 Received: (qmail 27797 invoked by uid 109); 13 Apr 2019 05:57:40 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Sat, 13 Apr 2019 05:57:40 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 12692 invoked by uid 111); 13 Apr 2019 05:58:08 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Sat, 13 Apr 2019 01:58:08 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Sat, 13 Apr 2019 01:57:37 -0400 Date: Sat, 13 Apr 2019 01:57:37 -0400 From: Jeff King To: git@vger.kernel.org Cc: Jonathan Tan Subject: [PATCH 7/7] fetch: do not consider peeled tags as advertised tips Message-ID: <20190413055736.GG19495@sigill.intra.peff.net> References: <20190413055127.GA32340@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190413055127.GA32340@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Our filter_refs() function accidentally considers the target of a peeled tag to be advertised by the server, even though upload-pack on the server side does not consider it so. This can result in the client making a bogus fetch to the server, which will end with the server complaining "not our ref". Whereas the correct behavior is for the client to notice that the server will not allow the request and error out immediately. So as bugs go, this is not very serious (the outcome is the same either way -- the fetch fails). But it's worth making the logic here correct and consistent with other related cases (e.g., fetching an oid that the server did not mention at all). The crux of the issue comes from fdb69d33c4 (fetch-pack: always allow fetching of literal SHA1s, 2017-05-15). After that, the strategy of filter_refs() is basically: - for each advertised ref, try to match it with a "sought" ref provided by the user. Skip any malformed refs (which includes peeled values like "refs/tags/foo^{}"), and place any unmatched items onto the unmatched list. - if there are unmatched sought refs, then put all of the advertised tips into an oidset, including the unmatched ones. - for each sought ref, see if it's in the oidset, in which case it's legal for us to ask the server for it The problem is in the second step. Our list of unmatched refs includes the peeled refs, even though upload-pack does not allow them to be directly fetched. So the simplest fix would be to exclude them during that step. However, we can observe that the unmatched list isn't used for anything else, and is freed at the end. We can just free those malformed refs immediately. That saves us having to check each ref a second time to see if it's malformed. Note that this code only kicks in when "strict" is in effect. I.e., if we are using the v0 protocol and uploadpack.allowReachableSHA1InWant is not in effect. With v2, all oids are allowed, and we do not bother creating or consulting the oidset at all. To future-proof our test against the upcoming GIT_TEST_PROTOCOL_VERSION flag, we'll manually mark it as a v0-only test. Signed-off-by: Jeff King --- We actually discussed this a while ago in: https://public-inbox.org/git/20180611044710.GB31642@sigill.intra.peff.net/ and the surrounding thread. I'm not convinced that upload-pack shouldn't be considering the peeled values as ref tips, but given that we have never done so, it's probably not worth making that change now. Clients couldn't rely on it without a capability marker, and all of this goes away in the v2 protocol anyway. fetch-pack.c | 11 ++++++++--- t/t5516-fetch-push.sh | 11 +++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index a181d3401d..bb8eac8126 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -573,9 +573,14 @@ static void filter_refs(struct fetch_pack_args *args, next = ref->next; if (starts_with(ref->name, "refs/") && - check_refname_format(ref->name, 0)) - ; /* trash */ - else { + check_refname_format(ref->name, 0)) { + /* + * trash or a peeled value; do not even add it to + * unmatched list + */ + free_one_ref(ref); + continue; + } else { while (i < nr_sought) { int cmp = strcmp(ref->name, sought[i]->name); if (cmp < 0) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 98ef71b48c..4f065212b8 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1273,6 +1273,17 @@ test_expect_success 'fetch follows tags by default' ' test_cmp expect actual ' +test_expect_success 'peeled advertisements are not considered ref tips' ' + mk_empty testrepo && + git -C testrepo commit --allow-empty -m one && + git -C testrepo commit --allow-empty -m two && + git -C testrepo tag -m foo mytag HEAD^ && + oid=$(git -C testrepo rev-parse mytag^{commit}) && + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \ + git fetch testrepo $oid 2>err && + test_i18ngrep "Server does not allow request for unadvertised object" err +' + test_expect_success 'pushing a specific ref applies remote.$name.push as refmap' ' mk_test testrepo heads/master && rm -fr src dst &&