Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet
On Mon, Feb 18, 2013 at 04:33:31PM -0500, Jeff King wrote: On Mon, Feb 18, 2013 at 01:25:35PM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: But it's easy to do (1), and it starts the clock ticking for the 1000-byte readers to become obsolete. Yup, I agree with that goal. Having just looked at the pkt-line callers a lot, I think most of them could go for something like: [...] That would actually simplify the callers a bit, and would harmonize the buffer sizes at the same time. I'll look into doing a series tonight. Just a quick update on this series. It ended up taking more nights than I thought. :) The result looks much better than what I posted before, and I found several other corner cases and bugs in packet parsing, too. I'm going to hold off on posting it tonight, as I'm now up to 19 patches, and after several rounds of rebase -i, I need to give it a final read-through myself before inflicting it on anyone else. I'll do that and post it tomorrow. In the meantime, please hold off on what I've posted so far (that includes the jk/smart-http-robustify topic). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet
Jeff King p...@peff.net wrote: In the meantime, please hold off on what I've posted so far (that includes the jk/smart-http-robustify topic). Surely. I'm done for the night already. Looking forward to see the reroll tomorrow. Thanks. -- Pardon terseness, typo and HTML from a tablet. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 04/10] pkt-line: change error message for oversized packet
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 p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet
Jeff King p...@peff.net writes: 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... I thought that 1000-byte limit was kept when we introduced the 64k interface to interoperate with other side that does not yet support the 64k interface. Is your justification that such an old version of Git no longer matters in the real world (which is true, I think), or we use 1000-byte limit in some codepaths even when we know that we are talking with a 64k-capable version of Git on the other side? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet
On Mon, Feb 18, 2013 at 01:40:17AM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: 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... I thought that 1000-byte limit was kept when we introduced the 64k interface to interoperate with other side that does not yet support the 64k interface. Is your justification that such an old version of Git no longer matters in the real world (which is true, I think), or we use 1000-byte limit in some codepaths even when we know that we are talking with a 64k-capable version of Git on the other side? I should have been more clear that I want to bump only the _reading_ side, not the writing. The sideband-64k capability bumps the sideband chunk size. But the size for packetized ref advertisement lines has remained at 1000. Which it must, since we start outputting them before doing capability negotiation. The sender will die before writing a longer ref line (see pkg-line.c:format_packet), and most of the reading callsites feed a 1000-byte buffer to packet_read_line (which will die if we get a larger packet). So the upgrade path for that would be: 1. Git bumps up all reading buffers to LARGE_PACKET_MAX, just in case. This immediately covers us for any alternate implementations that send larger ref packets (I do not know if any exist, but the documentation does not mention this limitation at all, so I would not be surprised if other implementations just use LARGE_PACKET_MAX as a maximum). 2. Many years pass. We decide that Git v1.8.2 and older are ancient history and not worth caring about. 3. We bump the 1000-byte limit for format_packet to LARGE_PACKET_MAX. This is not pressing at all; I wouldn't have even noticed it if I hadn't been wondering how large to make the new buffer I was adding in a later patch. I have not heard of anyone running up against this limit in practice. But it's easy to do (1), and it starts the clock ticking for the 1000-byte readers to become obsolete. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet
On Mon, Feb 18, 2013 at 02:15:23AM -0800, Jonathan Nieder wrote: Jeff King wrote: --- 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); Makes sense. I think this should say expected %u, got %d, since we don't actually expect most lines to be 1004 bytes in practice. Yeah, I had toyed with writing expected max %u for the same reason. I'll tweak it in the re-roll. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet
Jeff King p...@peff.net writes: But it's easy to do (1), and it starts the clock ticking for the 1000-byte readers to become obsolete. Yup, I agree with that goal. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 04/10] pkt-line: change error message for oversized packet
On Mon, Feb 18, 2013 at 01:25:35PM -0800, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
But it's easy to do (1), and it starts the clock ticking for
the 1000-byte readers to become obsolete.
Yup, I agree with that goal.
Having just looked at the pkt-line callers a lot, I think most of them
could go for something like:
char *packet_read(int fd, unsigned *len_p)
{
static char buffer[LARGE_PACKET_MAX];
int len;
len = packet_read_to_buf(fd, buffer, sizeof(buffer));
if (len 0)
return NULL;
*len_p = len;
return buffer;
}
That would actually simplify the callers a bit, and would harmonize the
buffer sizes at the same time. I'll look into doing a series tonight.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
