git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: git@vger.kernel.org, "Shawn O. Pearce" <spearce@spearce.org>
Subject: [PATCHv2 04/10] pkt-line: change error message for oversized packet
Date: Mon, 18 Feb 2013 04:22:21 -0500	[thread overview]
Message-ID: <20130218092221.GD5096@sigill.intra.peff.net> (raw)
In-Reply-To: <20130218091203.GB17003@sigill.intra.peff.net>

If we get a packet from the remote side that is too large to
fit in our buffer, we currently complain "protocol error:
bad line length".  This is a bit vague. The line length the
other side sent is not "bad" per se; the actual problem is
that it exceeded our expectation for buffer length.

This will generally not happen between two git-core
implementations, because the sender limits themselves to
either 1000, or to LARGE_PACKET_MAX (depending on what is
being sent, sideband-64k negotiation, etc), and the receiver
uses a buffer of the appropriate size.

The protocol document indicates the LARGE_PACKET_MAX limit
(of 65520), but does not actually specify the 1000-byte
limit for ref lines. It is likely that other implementations
just create a packet as large as they need, and this doesn't
come up often because nobody has 1000-character ref names
(or close to it, counting sha1 and other boilerplate).

We may want to increase the size of our receive buffers for
ref lines to prepare for such writers (whether they are
other implementations, or if we eventually want to bump the
write size in git-core). In the meantime, this patch tries
to give a more clear message in case it does come up.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm really tempted to bump all of our 1000-byte buffers to just use
LARGE_PACKET_MAX. If we provided a packet_read variant that used a
static buffer (which is fine for all but one or two callers), then it
would not take much memory (right now we stick some LARGE_PACKET_MAX
buffers on the stack, which is slightly questionable for
stack-restricted systems). But I left that for a different topic (and
even if we do, we would still want this message to catch anything over
the bizarre 65520 limit).

Out of curiosity, I grepped the list archives, and found only one
instance of this message. And it was somebody whose data stream was tainted
with random crud that happened to be numbers (much more common is "bad line
length character", when the crud does not look like a packet length).

 pkt-line.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/pkt-line.c b/pkt-line.c
index 62479d3..f2a7575 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -160,7 +160,8 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)
 	}
 	len -= 4;
 	if (len >= size)
-		die("protocol error: bad line length %d", len);
+		die("protocol error: line too large: (expected %u, got %d)",
+		    size, len);
 	ret = safe_read(fd, buffer, len, gently);
 	if (ret < 0)
 		return ret;
-- 
1.8.1.20.g7078b03

  parent reply	other threads:[~2013-02-18  9:22 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-02-16  6:44 [PATCH 0/3] make smart-http more robust against bogus server input Jeff King
2013-02-16  6:46 ` [PATCH 1/3] pkt-line: teach packet_get_line a no-op mode Jeff King
2013-02-17 10:41   ` Jonathan Nieder
2013-02-16  6:47 ` [PATCH 2/3] remote-curl: verify smart-http metadata lines Jeff King
2013-02-17 10:49   ` Jonathan Nieder
2013-02-17 19:14     ` Jeff King
2013-02-18  0:54       ` Jonathan Nieder
2013-02-18  8:59         ` Jeff King
2013-02-16  6:49 ` [PATCH 3/3] remote-curl: sanity check ref advertisement from server Jeff King
2013-02-17 11:05   ` Jonathan Nieder
2013-02-17 19:28     ` Jeff King
2013-02-18  1:41       ` Jonathan Nieder
2013-02-18  9:12         ` [PATCHv2 0/10] pkt-line and remote-curl cleanups server Jeff King
2013-02-18  9:14           ` [PATCHv2 01/10] pkt-line: move a misplaced comment Jeff King
2013-02-18  9:15           ` [PATCHv2 02/10] pkt-line: drop safe_write function Jeff King
2013-02-18  9:56             ` Jonathan Nieder
2013-02-18 10:24               ` Jeff King
2013-02-18  9:16           ` [PATCHv2 03/10] pkt-line: clean up "gentle" reading function Jeff King
2013-02-18 10:12             ` Jonathan Nieder
2013-02-18 10:25               ` Jeff King
2013-02-18  9:22           ` Jeff King [this message]
2013-02-18  9:40             ` [PATCHv2 04/10] pkt-line: change error message for oversized packet Junio C Hamano
2013-02-18  9:49               ` Jeff King
2013-02-18 21:25                 ` Junio C Hamano
2013-02-18 21:33                   ` Jeff King
2013-02-20  8:47                     ` Jeff King
2013-02-20  8:54                       ` Junio C Hamano
2013-02-18 10:15             ` Jonathan Nieder
2013-02-18 10:26               ` Jeff King
2013-02-18  9:22           ` [PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/ Jeff King
2013-02-18 10:19             ` Jonathan Nieder
2013-02-18 10:29               ` Jeff King
2013-02-18 11:05                 ` Jonathan Nieder
2013-02-18  9:26           ` [PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation Jeff King
2013-02-18 10:43             ` Jonathan Nieder
2013-02-18 10:48               ` Jeff King
2013-02-18 10:54                 ` Jonathan Nieder
2013-02-18  9:27           ` [PATCHv2 07/10] teach get_remote_heads to read from a memory buffer Jeff King
2013-02-18  9:29           ` [PATCHv2 08/10] remote-curl: pass buffer straight to get_remote_heads Jeff King
2013-02-18 10:47             ` Jonathan Nieder
2013-02-18  9:29           ` [PATCHv2 0/10] pkt-line and remote-curl cleanups server Junio C Hamano
2013-02-18  9:33             ` Jeff King
2013-02-18  9:49               ` Junio C Hamano
2013-02-18  9:55                 ` Jeff King
2013-02-20  7:14                 ` Shawn Pearce
2013-02-18  9:29           ` [PATCHv2 09/10] remote-curl: move ref-parsing code up in file Jeff King
2013-02-18  9:30           ` [PATCHv2 10/10] remote-curl: always parse incoming refs Jeff King
2013-02-18 10:50             ` Jonathan Nieder
2013-02-20  7:41               ` Shawn Pearce
2013-02-20  7:05           ` [PATCHv2 0/10] pkt-line and remote-curl cleanups server Shawn Pearce

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=20130218092221.GD5096@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=spearce@spearce.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).