[PATCH 0/7] some fetch reachability-error fixes

[PATCH 0/7] some fetch reachability-error fixes

[Jeff King]

This series fixes a few minor bugs around clients trying to fetch
unreachable commits, along with a few associated cleanups.

There's a minor conflict in t5516 when merging with next, since that
adds GIT_TEST_PROTOCOL_VERSION, causing a few textual conflicts. The
resolution should be fairly clear.

Where appropriate, I took GIT_TEST_PROTOCOL_VERSION into account so that
after merging with next, all tests should still pass even with it set to
"2" (well, all the tests that do not already fail on vanilla 'next' with
that setting).

  [1/7]: t5516: drop ok=sigpipe from unreachable-want tests
  [2/7]: t5530: check protocol response for "not our ref"
  [3/7]: upload-pack: send ERR packet for non-tip objects
  [4/7]: pkt-line: prepare buffer before handling ERR packets
  [5/7]: fetch: use free_refs()
  [6/7]: remote.c: make singular free_ref() public
  [7/7]: fetch: do not consider peeled tags as advertised tips

 fetch-pack.c                 | 16 +++++++++-------
 pkt-line.c                   |  9 +++++----
 remote.c                     |  6 +++---
 remote.h                     |  4 +++-
 t/t5516-fetch-push.sh        | 18 +++++++++++++++---
 t/t5530-upload-pack-error.sh | 18 +++++++++++++++---
 upload-pack.c                | 11 ++++++++---
 7 files changed, 58 insertions(+), 24 deletions(-)

-Peff

[PATCH 1/7] t5516: drop ok=sigpipe from unreachable-want tests

[Jeff King]

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 <peff@peff.net>
---
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
-- 
2.21.0.931.gd0bc72a411

[PATCH 2/7] t5530: check protocol response for "not our ref"

[Jeff King]

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 <peff@peff.net>
---
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 . <input >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' '
-- 
2.21.0.931.gd0bc72a411

[PATCH 3/7] upload-pack: send ERR packet for non-tip objects

[Jeff King]

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 <peff@peff.net>
---
 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 . <input >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;
-- 
2.21.0.931.gd0bc72a411

[PATCH 4/7] pkt-line: prepare buffer before handling ERR packets

[Jeff King]

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 <peff@peff.net>
---
 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
-- 
2.21.0.931.gd0bc72a411

[PATCH 5/7] fetch: use free_refs()

[Jeff King]

There's no need for us to write this loop manually when a helper
function can already do it.

Signed-off-by: Jeff King <peff@peff.net>
---
 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;
 }
-- 
2.21.0.931.gd0bc72a411

[PATCH 6/7] remote.c: make singular free_ref() public

[Jeff King]

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 <peff@peff.net>
---
 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;
-- 
2.21.0.931.gd0bc72a411

[PATCH 7/7] fetch: do not consider peeled tags as advertised tips

[Jeff King]

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 <peff@peff.net>
---
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 &&
-- 
2.21.0.931.gd0bc72a411

Re: [PATCH 0/7] some fetch reachability-error fixes

[Jonathan Tan]

>   [1/7]: t5516: drop ok=sigpipe from unreachable-want tests
>   [2/7]: t5530: check protocol response for "not our ref"
>   [3/7]: upload-pack: send ERR packet for non-tip objects
>   [4/7]: pkt-line: prepare buffer before handling ERR packets
>   [5/7]: fetch: use free_refs()
>   [6/7]: remote.c: make singular free_ref() public
>   [7/7]: fetch: do not consider peeled tags as advertised tips

All these look good to me. Thanks for catching the issue that you fixed
in patch 4.