git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Jonathan Tan <jonathantanmy@google.com>,
	Masaya Suzuki <masayasuzuki@google.com>
Subject: [PATCH 4/7] pkt-line: prepare buffer before handling ERR packets
Date: Sat, 13 Apr 2019 01:54:02 -0400
Message-ID: <20190413055401.GD19495@sigill.intra.peff.net> (raw)
In-Reply-To: <20190413055127.GA32340@sigill.intra.peff.net>

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


  parent reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-13  5:51 [PATCH 0/7] some fetch reachability-error fixes Jeff King
2019-04-13  5:52 ` [PATCH 1/7] t5516: drop ok=sigpipe from unreachable-want tests Jeff King
2019-04-13  5:53 ` [PATCH 2/7] t5530: check protocol response for "not our ref" Jeff King
2019-04-13  5:53 ` [PATCH 3/7] upload-pack: send ERR packet for non-tip objects Jeff King
2019-04-13  5:54 ` Jeff King [this message]
2019-04-13  5:54 ` [PATCH 5/7] fetch: use free_refs() Jeff King
2019-04-13  5:54 ` [PATCH 6/7] remote.c: make singular free_ref() public Jeff King
2019-04-13  5:57 ` [PATCH 7/7] fetch: do not consider peeled tags as advertised tips Jeff King
2019-04-15 21:06 ` [PATCH 0/7] some fetch reachability-error fixes Jonathan Tan

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190413055401.GD19495@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=masayasuzuki@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox