| Message ID | 20190225234901.65277-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [FIXUP] Fixup on tip of jt/http-auth-proto-v2-fix | expand |
Hi, Jonathan Tan wrote: > Thanks, Peff, for noticing this. It's because the client sometimes sends > "0000" as a single request (that is, it flushes, and then before it > sends any data, it flushes again). And post_rpc() assumes that it can > always read something - which is usually correct, but not in this case; > we read in stateless_connect() first, and if we read "0000", we need to > tell post_rpc() to not read at all. > > This is a fixup on the tip of jt/http-auth-proto-v2-fix that fixes that. > > As for why the client sends a lone "0000", I'm not sure, but that's > outside the scope of this patch set, I think. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > remote-curl.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) Tested-by: Jonathan Nieder <jrnieder@gmail.com> Thanks for fixing it. Is there a particular patch this should be squashed into, or does it stand alone? It the latter, mind writing a commit message for it? Thanks, Jonathan
> Thanks for fixing it. > > Is there a particular patch this should be squashed into, or does it > stand alone? It the latter, mind writing a commit message for it? Not sure if I'm using "fixup" correctly in the subject, but this is meant to be squashed onto the tip of jt/http-auth-proto-v2-fix - specifically, deb7d2094a ("remote-curl: use post_rpc() for protocol v2 also", 2019-02-14). I don't think it should be a standalone commit, as the tip is buggy - which might break bisect. But if we really want a standalone commit, this commit message should work: remote-curl: handle consecutive flushes correctly When the client, using protocol v2, sends two consecutive flushes and then an EOF, remote-curl dies. This is because, at the start of a new request, stateless_connect() reads, and if no EOF is found, then stateless_connect() calls post_rpc() which reads until a flush is encountered. This is a problem when stateless_connect() reads the second consecutive flush (hence, no EOF), and then post_rpc() reads, not expecting an EOF at all. Teach stateless_connect() to inform post_rpc() to read only if what it read isn't a flush.
On Mon, Feb 25, 2019 at 03:49:01PM -0800, Jonathan Tan wrote: > Thanks, Peff, for noticing this. It's because the client sometimes sends > "0000" as a single request (that is, it flushes, and then before it > sends any data, it flushes again). And post_rpc() assumes that it can > always read something - which is usually correct, but not in this case; > we read in stateless_connect() first, and if we read "0000", we need to > tell post_rpc() to not read at all. > > This is a fixup on the tip of jt/http-auth-proto-v2-fix that fixes that. Thanks, I can confirm that this makes the problem go away (and your explanation makes perfect sense to me). > As for why the client sends a lone "0000", I'm not sure, but that's > outside the scope of this patch set, I think. Yeah, that does seem odd. I noticed it on noop fetches. So after we've done "ls-refs", would "fetch" need to send a flush to say "I don't want anything?" I guess not, since we're stateless, and it is literally making a new HTTP request just to say nothing. It does seem unique to protocol v2. -Peff
Jonathan Tan <jonathantanmy@google.com> writes: >> Thanks for fixing it. >> >> Is there a particular patch this should be squashed into, or does it >> stand alone? It the latter, mind writing a commit message for it? > > Not sure if I'm using "fixup" correctly in the subject, but this is > meant to be squashed onto the tip of jt/http-auth-proto-v2-fix - > specifically, deb7d2094a ("remote-curl: use post_rpc() for protocol v2 > also", 2019-02-14). Saying "Fixup on tip of" to mean that you are fixing the 5th one in a 5-patch series is sufficient to convey what you want between humans, but that does not help the machine to help humans reduce mistakes. It would have been even more helpful if you created this commit with git commit --fixup 0cdb2a12ad which would have produced something like the attached patch, and that would have allowed me to do git checkout jt/http-auth-proto-v2-fix git am ./+jt-http-auth-proto-v2-fix-fixup git rebase -i --autosquash HEAD~5 or somesuch. Thanks, will queue. -- >8 -- Subject: [PATCH] fixup! remote-curl: use post_rpc() for protocol v2 also Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- remote-curl.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/remote-curl.c b/remote-curl.c index 8c03c78fc6..3c3872cca6 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -758,7 +758,11 @@ static curl_off_t xcurl_off_t(size_t len) return (curl_off_t)size; } -static int post_rpc(struct rpc_state *rpc) +/* + * If flush_received is true, do not attempt to read any more; just use what's + * in rpc->buf. + */ +static int post_rpc(struct rpc_state *rpc, int flush_received) { struct active_request_slot *slot; struct curl_slist *headers = http_copy_default_headers(); @@ -773,17 +777,19 @@ static int post_rpc(struct rpc_state *rpc) * allocated buffer space we can use HTTP/1.0 and avoid the * chunked encoding mess. */ - while (1) { - size_t n; - enum packet_read_status status; - - if (!rpc_read_from_out(rpc, 0, &n, &status)) { - large_request = 1; - use_gzip = 0; - break; + if (!flush_received) { + while (1) { + size_t n; + enum packet_read_status status; + + if (!rpc_read_from_out(rpc, 0, &n, &status)) { + large_request = 1; + use_gzip = 0; + break; + } + if (status == PACKET_READ_FLUSH) + break; } - if (status == PACKET_READ_FLUSH) - break; } if (large_request) { @@ -963,7 +969,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads, break; rpc->pos = 0; rpc->len = n; - err |= post_rpc(rpc); + err |= post_rpc(rpc, 0); } close(client.in); @@ -1319,7 +1325,7 @@ static int stateless_connect(const char *service_name) BUG("The entire rpc->buf should be larger than LARGE_PACKET_MAX"); if (status == PACKET_READ_EOF) break; - if (post_rpc(&rpc)) + if (post_rpc(&rpc, status == PACKET_READ_FLUSH)) /* We would have an err here */ break; /* Reset the buffer for next request */
diff --git a/remote-curl.c b/remote-curl.c index 2394a00650..6c0207f4f9 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -747,7 +747,11 @@ static curl_off_t xcurl_off_t(size_t len) return (curl_off_t)size; } -static int post_rpc(struct rpc_state *rpc) +/* + * If flush_received is true, do not attempt to read any more; just use what's + * in rpc->buf. + */ +static int post_rpc(struct rpc_state *rpc, int flush_received) { struct active_request_slot *slot; struct curl_slist *headers = http_copy_default_headers(); @@ -762,17 +766,19 @@ static int post_rpc(struct rpc_state *rpc) * allocated buffer space we can use HTTP/1.0 and avoid the * chunked encoding mess. */ - while (1) { - size_t n; - enum packet_read_status status; - - if (!rpc_read_from_out(rpc, 0, &n, &status)) { - large_request = 1; - use_gzip = 0; - break; + if (!flush_received) { + while (1) { + size_t n; + enum packet_read_status status; + + if (!rpc_read_from_out(rpc, 0, &n, &status)) { + large_request = 1; + use_gzip = 0; + break; + } + if (status == PACKET_READ_FLUSH) + break; } - if (status == PACKET_READ_FLUSH) - break; } if (large_request) { @@ -952,7 +958,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads, break; rpc->pos = 0; rpc->len = n; - err |= post_rpc(rpc); + err |= post_rpc(rpc, 0); } close(client.in); @@ -1308,7 +1314,7 @@ static int stateless_connect(const char *service_name) BUG("The entire rpc->buf should be larger than LARGE_PACKET_MAX"); if (status == PACKET_READ_EOF) break; - if (post_rpc(&rpc)) + if (post_rpc(&rpc, status == PACKET_READ_FLUSH)) /* We would have an err here */ break; /* Reset the buffer for next request */
Thanks, Peff, for noticing this. It's because the client sometimes sends "0000" as a single request (that is, it flushes, and then before it sends any data, it flushes again). And post_rpc() assumes that it can always read something - which is usually correct, but not in this case; we read in stateless_connect() first, and if we read "0000", we need to tell post_rpc() to not read at all. This is a fixup on the tip of jt/http-auth-proto-v2-fix that fixes that. As for why the client sends a lone "0000", I'm not sure, but that's outside the scope of this patch set, I think. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- remote-curl.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)