Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines
On Sun, Feb 17, 2013 at 04:54:43PM -0800, Jonathan Nieder wrote: > > My intent was that it followed the error convention of "negative is > > error, 0 is success, and positive is not used, but reserved for > > future use". > > From a maintainability perspective, that kind of contract would be > dangerous, since some *other* caller could arrive and use the function > without a "< 0" without knowing it is doing anything wrong. When new > return values appear, the function should be renamed to help the patch > author and reviewers remember to check all callers. True. That's why I always write "< 0". :) > That is, from the point of view of maintainability, there is no > distinction between "if (read_packets_until_... < 0)" and > "if (read_packets_until_...)" and either form is fine. > > My comment was just to say the "< 0" forced me to pause a moment and > check out the implementation. This is basically a stylistic thing and > if you prefer to keep the "< 0", that's fine with me. Interesting. To me, "foo() < 0" just reads idiomatically as "error-check the foo call". Anyway, I've redone the patch series to just re-use get_remote_heads, which is more robust. So this function has gone away in the new version. -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: [PATCH 2/3] remote-curl: verify smart-http metadata lines
Jeff King wrote: > On Sun, Feb 17, 2013 at 02:49:39AM -0800, Jonathan Nieder wrote: >> Jeff King wrote: >>> --- a/remote-curl.c [...] >>> + if (read_packets_until_flush(&last->buf, &last->len) < 0) >> >> Style nit: this made me wonder "What would it mean if >> read_packets_until_flush() > 0?" [...] > My intent was that it followed the error convention of "negative is > error, 0 is success, and positive is not used, but reserved for > future use". >From a maintainability perspective, that kind of contract would be dangerous, since some *other* caller could arrive and use the function without a "< 0" without knowing it is doing anything wrong. When new return values appear, the function should be renamed to help the patch author and reviewers remember to check all callers. That is, from the point of view of maintainability, there is no distinction between "if (read_packets_until_... < 0)" and "if (read_packets_until_...)" and either form is fine. My comment was just to say the "< 0" forced me to pause a moment and check out the implementation. This is basically a stylistic thing and if you prefer to keep the "< 0", that's fine with me. > If > an implementation is producing bogus packet lines and expecting us not > to complain, it really needs to be fixed. Agreed completely. Thanks again for the patch. Jonathan -- 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: [PATCH 2/3] remote-curl: verify smart-http metadata lines
On Sun, Feb 17, 2013 at 02:49:39AM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> [...]
> > @@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char
> > *service)
> [...]
> > - strbuf_reset(&buffer);
> > - while (packet_get_line(&buffer, &last->buf, &last->len) > 0)
> > - strbuf_reset(&buffer);
> > + if (read_packets_until_flush(&last->buf, &last->len) < 0)
>
> Style nit: this made me wonder "What would it mean if
> read_packets_until_flush() > 0?" Since the convention for this
> function is "0 for success", I would personally find
>
> if (read_packets_until_flush(...))
> handle error;
>
> easier to read.
My intent was that it followed the error convention of "negative is
error, 0 is success, and positive is not used, but reserved for
future use". And I tend to think the "< 0" makes it obvious that we are
interested in error. But I don't feel that strongly, so if people would
rather see it the other way, I can live with it.
> > + die("smart-http metadata lines are invalid at %s",
> > + refs_url);
>
> Especially given that other clients would be likely to run into
> trouble in the same situation, as long as this cooks in "next" for a
> suitable amount of time to catch bad servers, it looks like a good
> idea.
Yeah, I have a slight concern that this series would break something in
another implementation, so I would like to see this cook in "next" for a
while (and would be slated for master probably not in this release, but
in the next one). But I think this change is pretty straightforward. If
an implementation is producing bogus packet lines and expecting us not
to complain, it really needs to be fixed.
-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: [PATCH 2/3] remote-curl: verify smart-http metadata lines
Jeff King wrote:
> --- a/remote-curl.c
> +++ b/remote-curl.c
[...]
> @@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char
> *service)
[...]
> - strbuf_reset(&buffer);
> - while (packet_get_line(&buffer, &last->buf, &last->len) > 0)
> - strbuf_reset(&buffer);
> + if (read_packets_until_flush(&last->buf, &last->len) < 0)
Style nit: this made me wonder "What would it mean if
read_packets_until_flush() > 0?" Since the convention for this
function is "0 for success", I would personally find
if (read_packets_until_flush(...))
handle error;
easier to read.
> + die("smart-http metadata lines are invalid at %s",
> + refs_url);
Especially given that other clients would be likely to run into
trouble in the same situation, as long as this cooks in "next" for a
suitable amount of time to catch bad servers, it looks like a good
idea.
Reviewed-by: Jonathan Nieder
Thanks.
--
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
[PATCH 2/3] remote-curl: verify smart-http metadata lines
A smart http ref advertisement starts with a packet
containing the service header, followed by an arbitrary
number of packets containing other metadata headers,
followed by a flush packet.
We don't currently recognize any other metadata headers, so
we just parse through any extra packets, throwing away their
contents. However, we don't do so very carefully, and just
stop at the first error or flush packet.
Let's flag any errors we see here, which might be a sign of
truncated or corrupted output. Since the rest of the data
should be the ref advertisement, and since we pass that
along to our helper programs (like fetch-pack), they will
probably notice the error, as whatever cruft is in the
buffer will not parse. However, it's nice to note problems
as early as possible, which can help in debugging the root
cause.
Signed-off-by: Jeff King
---
remote-curl.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 933c69a..73134f5 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -90,6 +90,17 @@ static void free_discovery(struct discovery *d)
}
}
+static int read_packets_until_flush(char **buf, size_t *len)
+{
+ while (1) {
+ int r = packet_get_line(NULL, buf, len);
+ if (r < 0)
+ return -1;
+ if (r == 0)
+ return 0;
+ }
+}
+
static struct discovery* discover_refs(const char *service)
{
struct strbuf exp = STRBUF_INIT;
@@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char
*service)
/* The header can include additional metadata lines, up
* until a packet flush marker. Ignore these now, but
-* in the future we might start to scan them.
+* in the future we might start to scan them. However, we do
+* still check to make sure we are getting valid packet lines,
+* ending with a flush.
*/
- strbuf_reset(&buffer);
- while (packet_get_line(&buffer, &last->buf, &last->len) > 0)
- strbuf_reset(&buffer);
+ if (read_packets_until_flush(&last->buf, &last->len) < 0)
+ die("smart-http metadata lines are invalid at %s",
+ refs_url);
last->proto_git = 1;
}
--
1.8.1.2.11.g1a2f572
--
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
