* [PATCH v2 1/7] remote-curl: fix typo
2020-05-18 15:47 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Denton Liu
@ 2020-05-18 15:47 ` Denton Liu
2020-05-18 15:47 ` [PATCH v2 2/7] remote-curl: remove label indentation Denton Liu
` (7 subsequent siblings)
8 siblings, 0 replies; 62+ messages in thread
From: Denton Liu @ 2020-05-18 15:47 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
remote-curl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/remote-curl.c b/remote-curl.c
index 1c9aa3d0ab..6844708f38 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -643,7 +643,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,
return 0;
}
/*
- * If avail is non-zerp, the line length for the flush still
+ * If avail is non-zero, the line length for the flush still
* hasn't been fully sent. Proceed with sending the line
* length.
*/
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH v2 2/7] remote-curl: remove label indentation
2020-05-18 15:47 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Denton Liu
2020-05-18 15:47 ` [PATCH v2 1/7] remote-curl: fix typo Denton Liu
@ 2020-05-18 15:47 ` Denton Liu
2020-05-18 18:37 ` Junio C Hamano
2020-05-18 15:47 ` [PATCH v2 3/7] transport: extract common fetch_pack() call Denton Liu
` (6 subsequent siblings)
8 siblings, 1 reply; 62+ messages in thread
From: Denton Liu @ 2020-05-18 15:47 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine
In the codebase, labels are aligned to the leftmost column. Remove the
space-indentation from `free_specs:` to conform to this.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
remote-curl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/remote-curl.c b/remote-curl.c
index 6844708f38..da3e07184a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1276,7 +1276,7 @@ static void parse_push(struct strbuf *buf)
if (ret)
exit(128); /* error already reported */
- free_specs:
+free_specs:
argv_array_clear(&specs);
}
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* Re: [PATCH v2 2/7] remote-curl: remove label indentation
2020-05-18 15:47 ` [PATCH v2 2/7] remote-curl: remove label indentation Denton Liu
@ 2020-05-18 18:37 ` Junio C Hamano
0 siblings, 0 replies; 62+ messages in thread
From: Junio C Hamano @ 2020-05-18 18:37 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Jeff King, Eric Sunshine
Denton Liu <liu.denton@gmail.com> writes:
> In the codebase, labels are aligned to the leftmost column. Remove the
> space-indentation from `free_specs:` to conform to this.
Even though there are 50+ remaining labels that tells us otherwise,
we have 300+ labels that support the position.
;-)
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> remote-curl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 6844708f38..da3e07184a 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -1276,7 +1276,7 @@ static void parse_push(struct strbuf *buf)
> if (ret)
> exit(128); /* error already reported */
>
> - free_specs:
> +free_specs:
> argv_array_clear(&specs);
> }
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v2 3/7] transport: extract common fetch_pack() call
2020-05-18 15:47 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Denton Liu
2020-05-18 15:47 ` [PATCH v2 1/7] remote-curl: fix typo Denton Liu
2020-05-18 15:47 ` [PATCH v2 2/7] remote-curl: remove label indentation Denton Liu
@ 2020-05-18 15:47 ` Denton Liu
2020-05-18 18:40 ` Junio C Hamano
2020-05-18 15:47 ` [PATCH v2 4/7] pkt-line: extern packet_length() Denton Liu
` (5 subsequent siblings)
8 siblings, 1 reply; 62+ messages in thread
From: Denton Liu @ 2020-05-18 15:47 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine
In the switch statement, the difference between the `protocol_v2` and
`protocol_v{1,0}` arms is a preparatory call to die_if_server_options() in
the latter. The fetch_pack() call is identical in both arms. However,
since this fetch_pack() call has so many parameters, it is not
immediately obvious that the call is identical in both cases.
Extract the common fetch_pack() call out of the switch statement so that
code duplication is reduced and the logic is more clear for future
readers.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
transport.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/transport.c b/transport.c
index 15f5ba4e8f..a6002e502f 100644
--- a/transport.c
+++ b/transport.c
@@ -371,22 +371,19 @@ static int fetch_refs_via_pack(struct transport *transport,
switch (data->version) {
case protocol_v2:
- refs = fetch_pack(&args, data->fd,
- refs_tmp ? refs_tmp : transport->remote_refs,
- to_fetch, nr_heads, &data->shallow,
- &transport->pack_lockfile, data->version);
+ /* do nothing */
break;
case protocol_v1:
case protocol_v0:
die_if_server_options(transport);
- refs = fetch_pack(&args, data->fd,
- refs_tmp ? refs_tmp : transport->remote_refs,
- to_fetch, nr_heads, &data->shallow,
- &transport->pack_lockfile, data->version);
break;
case protocol_unknown_version:
BUG("unknown protocol version");
}
+ refs = fetch_pack(&args, data->fd,
+ refs_tmp ? refs_tmp : transport->remote_refs,
+ to_fetch, nr_heads, &data->shallow,
+ &transport->pack_lockfile, data->version);
close(data->fd[0]);
close(data->fd[1]);
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* Re: [PATCH v2 3/7] transport: extract common fetch_pack() call
2020-05-18 15:47 ` [PATCH v2 3/7] transport: extract common fetch_pack() call Denton Liu
@ 2020-05-18 18:40 ` Junio C Hamano
0 siblings, 0 replies; 62+ messages in thread
From: Junio C Hamano @ 2020-05-18 18:40 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Jeff King, Eric Sunshine
Denton Liu <liu.denton@gmail.com> writes:
> In the switch statement, the difference between the `protocol_v2` and
> `protocol_v{1,0}` arms is a preparatory call to die_if_server_options() in
> the latter. The fetch_pack() call is identical in both arms. However,
> since this fetch_pack() call has so many parameters, it is not
> immediately obvious that the call is identical in both cases.
Sure.
if (data->version < protocol_v2)
die_if_server_options(transport);
else if (data->version == protocol_unknown_version)
BUG("...");
refs = fetch_pack(...);
might have been even easier to follow, but that's OK.
> switch (data->version) {
> case protocol_v2:
> - refs = fetch_pack(&args, data->fd,
> - refs_tmp ? refs_tmp : transport->remote_refs,
> - to_fetch, nr_heads, &data->shallow,
> - &transport->pack_lockfile, data->version);
> + /* do nothing */
> break;
> case protocol_v1:
> case protocol_v0:
> die_if_server_options(transport);
> - refs = fetch_pack(&args, data->fd,
> - refs_tmp ? refs_tmp : transport->remote_refs,
> - to_fetch, nr_heads, &data->shallow,
> - &transport->pack_lockfile, data->version);
> break;
> case protocol_unknown_version:
> BUG("unknown protocol version");
> }
> + refs = fetch_pack(&args, data->fd,
> + refs_tmp ? refs_tmp : transport->remote_refs,
> + to_fetch, nr_heads, &data->shallow,
> + &transport->pack_lockfile, data->version);
>
> close(data->fd[0]);
> close(data->fd[1]);
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v2 4/7] pkt-line: extern packet_length()
2020-05-18 15:47 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Denton Liu
` (2 preceding siblings ...)
2020-05-18 15:47 ` [PATCH v2 3/7] transport: extract common fetch_pack() call Denton Liu
@ 2020-05-18 15:47 ` Denton Liu
2020-05-18 16:04 ` Jeff King
2020-05-18 15:47 ` [PATCH v2 5/7] remote-curl: error on incomplete packet Denton Liu
` (4 subsequent siblings)
8 siblings, 1 reply; 62+ messages in thread
From: Denton Liu @ 2020-05-18 15:47 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine
In a future commit, we will be manually processing packets and we will
need to access the length header. In order to simplify this, extern
packet_length() so that the logic can be reused.
Change the function parameter from a `const char *` to
`const char linelen[4]`. Even though these two types behave identically
as function parameters, use the array notation to semantically indicate
exactly what this function is expecting as an argument.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
pkt-line.c | 2 +-
pkt-line.h | 9 +++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/pkt-line.c b/pkt-line.c
index a0e87b1e81..5c3b4539b5 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -306,7 +306,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,
return ret;
}
-static int packet_length(const char *linelen)
+int packet_length(const char linelen[4])
{
int val = hex2chr(linelen);
return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
diff --git a/pkt-line.h b/pkt-line.h
index fef3a0d792..de81776a7a 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -74,6 +74,15 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
int packet_read(int fd, char **src_buffer, size_t *src_len, char
*buffer, unsigned size, int options);
+/*
+ * Convert a four hex digit packet line length header into its numeric
+ * representation. linelen should not be null-terminated.
+ *
+ * If linelen contains non-hex characters, return -1. Otherwise, return the
+ * numeric value of the length header.
+ */
+int packet_length(const char linelen[4]);
+
/*
* Read a packetized line into a buffer like the 'packet_read()' function but
* returns an 'enum packet_read_status' which indicates the status of the read.
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* Re: [PATCH v2 4/7] pkt-line: extern packet_length()
2020-05-18 15:47 ` [PATCH v2 4/7] pkt-line: extern packet_length() Denton Liu
@ 2020-05-18 16:04 ` Jeff King
2020-05-18 17:50 ` Eric Sunshine
2020-05-18 18:44 ` Junio C Hamano
0 siblings, 2 replies; 62+ messages in thread
From: Jeff King @ 2020-05-18 16:04 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Eric Sunshine
On Mon, May 18, 2020 at 11:47:21AM -0400, Denton Liu wrote:
> In a future commit, we will be manually processing packets and we will
> need to access the length header. In order to simplify this, extern
> packet_length() so that the logic can be reused.
>
> Change the function parameter from a `const char *` to
> `const char linelen[4]`. Even though these two types behave identically
> as function parameters, use the array notation to semantically indicate
> exactly what this function is expecting as an argument.
OK. I double-checked that we will not run into any problems with passing
pointers or arrays of other sizes (sadly compilers would not tell us if
we passed a too-small array, but at least it's documented for humans).
> +/*
> + * Convert a four hex digit packet line length header into its numeric
> + * representation. linelen should not be null-terminated.
Minor nit, but it is perfectly fine if there is a NUL. Maybe "linelen
does not need to be..."?
-Peff
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2 4/7] pkt-line: extern packet_length()
2020-05-18 16:04 ` Jeff King
@ 2020-05-18 17:50 ` Eric Sunshine
2020-05-18 20:08 ` Jeff King
2020-05-18 18:44 ` Junio C Hamano
1 sibling, 1 reply; 62+ messages in thread
From: Eric Sunshine @ 2020-05-18 17:50 UTC (permalink / raw)
To: Jeff King; +Cc: Denton Liu, Git Mailing List
On Mon, May 18, 2020 at 12:04 PM Jeff King <peff@peff.net> wrote:
> On Mon, May 18, 2020 at 11:47:21AM -0400, Denton Liu wrote:
> > Change the function parameter from a `const char *` to
> > `const char linelen[4]`. Even though these two types behave identically
> > as function parameters, use the array notation to semantically indicate
> > exactly what this function is expecting as an argument.
>
> > +/*
> > + * Convert a four hex digit packet line length header into its numeric
> > + * representation. linelen should not be null-terminated.
>
> Minor nit, but it is perfectly fine if there is a NUL. Maybe "linelen
> does not need to be..."?
I had the exact same reaction when reading "...should not be
null-terminated", however, I'd prefer to drop mention of
NUL-termination altogether since talking about it merely confuses the
issue since it is quite clear both from the declaration (const
char[4]) and the documentation "four hex digits" that 'linelen' is
expected to contain exactly four hex digits and no NUL character(s).
By the way, I find the argument name "linelen" highly confusing; every
time I read it, I think it is an integral type, not a string or
character array. I'd very much prefer to see it renamed to "s" (or
something) or dropped altogether:
int packet_length(const char[4]);
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2 4/7] pkt-line: extern packet_length()
2020-05-18 17:50 ` Eric Sunshine
@ 2020-05-18 20:08 ` Jeff King
0 siblings, 0 replies; 62+ messages in thread
From: Jeff King @ 2020-05-18 20:08 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Denton Liu, Git Mailing List
On Mon, May 18, 2020 at 01:50:52PM -0400, Eric Sunshine wrote:
> On Mon, May 18, 2020 at 12:04 PM Jeff King <peff@peff.net> wrote:
> > On Mon, May 18, 2020 at 11:47:21AM -0400, Denton Liu wrote:
> > > Change the function parameter from a `const char *` to
> > > `const char linelen[4]`. Even though these two types behave identically
> > > as function parameters, use the array notation to semantically indicate
> > > exactly what this function is expecting as an argument.
> >
> > > +/*
> > > + * Convert a four hex digit packet line length header into its numeric
> > > + * representation. linelen should not be null-terminated.
> >
> > Minor nit, but it is perfectly fine if there is a NUL. Maybe "linelen
> > does not need to be..."?
>
> I had the exact same reaction when reading "...should not be
> null-terminated", however, I'd prefer to drop mention of
> NUL-termination altogether since talking about it merely confuses the
> issue since it is quite clear both from the declaration (const
> char[4]) and the documentation "four hex digits" that 'linelen' is
> expected to contain exactly four hex digits and no NUL character(s).
Yeah, I think I have a slight preference for that, too.
> By the way, I find the argument name "linelen" highly confusing; every
> time I read it, I think it is an integral type, not a string or
> character array. I'd very much prefer to see it renamed to "s" (or
> something) or dropped altogether:
>
> int packet_length(const char[4]);
I think giving it _some_ name is useful. Maybe "const char
lenbuf_hex[4]".
-Peff
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2 4/7] pkt-line: extern packet_length()
2020-05-18 16:04 ` Jeff King
2020-05-18 17:50 ` Eric Sunshine
@ 2020-05-18 18:44 ` Junio C Hamano
1 sibling, 0 replies; 62+ messages in thread
From: Junio C Hamano @ 2020-05-18 18:44 UTC (permalink / raw)
To: Jeff King; +Cc: Denton Liu, Git Mailing List, Eric Sunshine
Jeff King <peff@peff.net> writes:
>> +/*
>> + * Convert a four hex digit packet line length header into its numeric
>> + * representation. linelen should not be null-terminated.
>
> Minor nit, but it is perfectly fine if there is a NUL. Maybe "linelen
> does not need to be..."?
Yup, I was wondering about the same thing. I actually would go
stronger than "does not need to be", as the byte after these four
would never be taken as "the terminator for the linelen bytes", even
it happens to be '\0'.
Just getting rid of the extra sentence would suffice for that, I
think. The first sentence makes it clear that it is about
interpreting the 4 bytes we are given, and those 4 bytes come from
the 'packet line length header' the caller has.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v2 5/7] remote-curl: error on incomplete packet
2020-05-18 15:47 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Denton Liu
` (3 preceding siblings ...)
2020-05-18 15:47 ` [PATCH v2 4/7] pkt-line: extern packet_length() Denton Liu
@ 2020-05-18 15:47 ` Denton Liu
2020-05-18 16:22 ` Jeff King
2020-05-18 15:47 ` [PATCH v2 6/7] pkt-line: PACKET_READ_RESPONSE_END Denton Liu
` (3 subsequent siblings)
8 siblings, 1 reply; 62+ messages in thread
From: Denton Liu @ 2020-05-18 15:47 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine
Currently, remote-curl acts as a proxy and blindly forwards packets
between an HTTP server and fetch-pack. In the case of a stateless RPC
connection where the connection is terminated with a partially written
packet, remote-curl will blindly send the partially written packet
before waiting on more input from fetch-pack. Meanwhile, fetch-pack will
read the partial packet and continue reading, expecting more input. This
results in a deadlock between the two processes.
For a stateless connection, inspect packets before sending them and
error out if a a packet line packet is incomplete.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
remote-curl.c | 59 ++++++++++++++++++-
t/lib-httpd.sh | 2 +
t/lib-httpd/apache.conf | 8 +++
.../incomplete-body-upload-pack-v2-http.sh | 3 +
.../incomplete-length-upload-pack-v2-http.sh | 3 +
t/t5702-protocol-v2.sh | 34 +++++++++++
6 files changed, 106 insertions(+), 3 deletions(-)
create mode 100644 t/lib-httpd/incomplete-body-upload-pack-v2-http.sh
create mode 100644 t/lib-httpd/incomplete-length-upload-pack-v2-http.sh
diff --git a/remote-curl.c b/remote-curl.c
index da3e07184a..e020140092 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -679,9 +679,53 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
}
#endif
+struct check_pktline_state {
+ char len_buf[4];
+ int len_filled;
+ int remaining;
+};
+
+static void check_pktline(struct check_pktline_state *state, const char *ptr, size_t size)
+{
+ while (size) {
+ if (!state->remaining) {
+ int digits_remaining = 4 - state->len_filled;
+ if (digits_remaining > size)
+ digits_remaining = size;
+ memcpy(&state->len_buf[state->len_filled], ptr, digits_remaining);
+ state->len_filled += digits_remaining;
+ ptr += digits_remaining;
+ size -= digits_remaining;
+
+ if (state->len_filled == 4) {
+ state->remaining = packet_length(state->len_buf);
+ if (state->remaining < 0) {
+ die(_("remote-curl: bad line length character: %.4s"), state->len_buf);
+ } else if (state->remaining < 4) {
+ state->remaining = 0;
+ } else {
+ state->remaining -= 4;
+ }
+ state->len_filled = 0;
+ }
+ }
+
+ if (state->remaining) {
+ int remaining = state->remaining;
+ if (remaining > size)
+ remaining = size;
+ ptr += remaining;
+ size -= remaining;
+ state->remaining -= remaining;
+ }
+ }
+}
+
struct rpc_in_data {
struct rpc_state *rpc;
struct active_request_slot *slot;
+ int check_pktline;
+ struct check_pktline_state pktline_state;
};
/*
@@ -702,6 +746,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,
return size;
if (size)
data->rpc->any_written = 1;
+ if (data->check_pktline)
+ check_pktline(&data->pktline_state, ptr, size);
write_or_die(data->rpc->in, ptr, size);
return size;
}
@@ -778,7 +824,7 @@ static curl_off_t xcurl_off_t(size_t len)
* 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)
+static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_received)
{
struct active_request_slot *slot;
struct curl_slist *headers = http_copy_default_headers();
@@ -920,6 +966,8 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
rpc_in_data.rpc = rpc;
rpc_in_data.slot = slot;
+ rpc_in_data.check_pktline = stateless_connect;
+ memset(&rpc_in_data.pktline_state, 0, sizeof(rpc_in_data.pktline_state));
curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
@@ -936,6 +984,11 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)
if (!rpc->any_written)
err = -1;
+ if (rpc_in_data.pktline_state.len_filled)
+ err = error(_("%d bytes of length header were received"), rpc_in_data.pktline_state.len_filled);
+ if (rpc_in_data.pktline_state.remaining)
+ err = error(_("%d bytes of body are still expected"), rpc_in_data.pktline_state.remaining);
+
curl_slist_free_all(headers);
free(gzip_body);
return err;
@@ -985,7 +1038,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,
break;
rpc->pos = 0;
rpc->len = n;
- err |= post_rpc(rpc, 0);
+ err |= post_rpc(rpc, 0, 0);
}
close(client.in);
@@ -1342,7 +1395,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, status == PACKET_READ_FLUSH))
+ if (post_rpc(&rpc, 1, status == PACKET_READ_FLUSH))
/* We would have an err here */
break;
/* Reset the buffer for next request */
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 1449ee95e9..d2edfa4c50 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -129,6 +129,8 @@ install_script () {
prepare_httpd() {
mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
+ install_script incomplete-length-upload-pack-v2-http.sh
+ install_script incomplete-body-upload-pack-v2-http.sh
install_script broken-smart-http.sh
install_script error-smart-http.sh
install_script error.sh
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 994e5290d6..afa91e38b0 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -117,6 +117,8 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
</LocationMatch>
+ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/
+ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/
ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
@@ -126,6 +128,12 @@ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1
<Directory ${GIT_EXEC_PATH}>
Options FollowSymlinks
</Directory>
+<Files incomplete-length-upload-pack-v2-http.sh>
+ Options ExecCGI
+</Files>
+<Files incomplete-body-upload-pack-v2-http.sh>
+ Options ExecCGI
+</Files>
<Files broken-smart-http.sh>
Options ExecCGI
</Files>
diff --git a/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh
new file mode 100644
index 0000000000..2f5ed9fcf6
--- /dev/null
+++ b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh
@@ -0,0 +1,3 @@
+printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result"
+echo
+printf "%s%s\n" "0079" "45"
diff --git a/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh
new file mode 100644
index 0000000000..86c6e648c9
--- /dev/null
+++ b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh
@@ -0,0 +1,3 @@
+printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result"
+echo
+printf "%s\n" "00"
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 5039e66dc4..4eb81ba2d4 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -586,6 +586,40 @@ test_expect_success 'clone with http:// using protocol v2' '
! grep "Send header: Transfer-Encoding: chunked" log
'
+test_expect_success 'clone repository with http:// using protocol v2 with incomplete pktline length' '
+ test_when_finished "rm -f log" &&
+
+ git init "$HTTPD_DOCUMENT_ROOT_PATH/incomplete_length" &&
+ test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/incomplete_length" file &&
+
+ test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" git -c protocol.version=2 \
+ clone "$HTTPD_URL/smart/incomplete_length" incomplete_length_child 2>err &&
+
+ # Client requested to use protocol v2
+ grep "Git-Protocol: version=2" log &&
+ # Server responded using protocol v2
+ grep "git< version 2" log &&
+ # Client reported appropriate failure
+ test_i18ngrep "bytes of length header were received" err
+'
+
+test_expect_success 'clone repository with http:// using protocol v2 with incomplete pktline body' '
+ test_when_finished "rm -f log" &&
+
+ git init "$HTTPD_DOCUMENT_ROOT_PATH/incomplete_body" &&
+ test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/incomplete_body" file &&
+
+ test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" git -c protocol.version=2 \
+ clone "$HTTPD_URL/smart/incomplete_body" incomplete_body_child 2>err &&
+
+ # Client requested to use protocol v2
+ grep "Git-Protocol: version=2" log &&
+ # Server responded using protocol v2
+ grep "git< version 2" log &&
+ # Client reported appropriate failure
+ test_i18ngrep "bytes of body are still expected" err
+'
+
test_expect_success 'clone big repository with http:// using protocol v2' '
test_when_finished "rm -f log" &&
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* Re: [PATCH v2 5/7] remote-curl: error on incomplete packet
2020-05-18 15:47 ` [PATCH v2 5/7] remote-curl: error on incomplete packet Denton Liu
@ 2020-05-18 16:22 ` Jeff King
2020-05-18 16:51 ` Denton Liu
0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-18 16:22 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Eric Sunshine
On Mon, May 18, 2020 at 11:47:22AM -0400, Denton Liu wrote:
> +struct check_pktline_state {
> + char len_buf[4];
> + int len_filled;
> + int remaining;
> +};
> +
> +static void check_pktline(struct check_pktline_state *state, const char *ptr, size_t size)
Thanks for converting this. I think having this broken out makes it a
bit easier to reason about, and it should be much easier to reuse if we
need it elsewhere.
> +{
> + while (size) {
> + if (!state->remaining) {
> + int digits_remaining = 4 - state->len_filled;
> + if (digits_remaining > size)
> + digits_remaining = size;
> + memcpy(&state->len_buf[state->len_filled], ptr, digits_remaining);
> + state->len_filled += digits_remaining;
> + ptr += digits_remaining;
> + size -= digits_remaining;
Having personally written and screwed up this kind of parsing state
machine before, I read over this logic quite carefully. ;) I believe
it's correct.
Another way would be to loop by single characters:
while (state->len_filled < 4 && size) {
state->len_buf[state->len_filled++] = *ptr;
ptr++;
size--;
}
which I _think_ is equivalent, and is a bit shorter. I'm OK with either
(see below, especially).
I'm not sure if it's worth replacing "4" with ARRAY_SIZE(state->len_buf).
I generally try to avoid magic numbers, but it's certainly not like one
could change the size of len_buf and this code would still be useful. :)
> + if (state->len_filled == 4) {
> + state->remaining = packet_length(state->len_buf);
> + if (state->remaining < 0) {
> + die(_("remote-curl: bad line length character: %.4s"), state->len_buf);
> + } else if (state->remaining < 4) {
> + state->remaining = 0;
> + } else {
> + state->remaining -= 4;
> + }
> + state->len_filled = 0;
> + }
> + }
This part makes sense. We'll either leave len_filled as 1-3 (incomplete),
or we'll read a whole packet (for a flush), or we'll be waiting to read
the rest of the packet.
> + if (state->remaining) {
> + int remaining = state->remaining;
> + if (remaining > size)
> + remaining = size;
> + ptr += remaining;
> + size -= remaining;
> + state->remaining -= remaining;
> + }
> + }
> +}
And here we most certainly don't want to read character-by-character,
because we're not doing anything with each one, and we expect there to be
a lot more of them. Having the earlier loop match the form of this one is
perhaps a good reason to leave it alone.
> [...]
> @@ -936,6 +984,11 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)
> if (!rpc->any_written)
> err = -1;
>
> + if (rpc_in_data.pktline_state.len_filled)
> + err = error(_("%d bytes of length header were received"), rpc_in_data.pktline_state.len_filled);
> + if (rpc_in_data.pktline_state.remaining)
> + err = error(_("%d bytes of body are still expected"), rpc_in_data.pktline_state.remaining);
And here's the payoff for all of the state machine checks. Makes sense.
> @@ -702,6 +746,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,
> return size;
> if (size)
> data->rpc->any_written = 1;
> + if (data->check_pktline)
> + check_pktline(&data->pktline_state, ptr, size);
> write_or_die(data->rpc->in, ptr, size);
> return size;
> }
And this is now conditional. Good...
> @@ -920,6 +966,8 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)
> curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
> rpc_in_data.rpc = rpc;
> rpc_in_data.slot = slot;
> + rpc_in_data.check_pktline = stateless_connect;
> + memset(&rpc_in_data.pktline_state, 0, sizeof(rpc_in_data.pktline_state));
And we enable it only for stateless-connect. Makes perfect sense.
> --- /dev/null
> +++ b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh
> @@ -0,0 +1,3 @@
> +printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result"
> +echo
> +printf "%s%s\n" "0079" "45"
Nice. Just having a deterministic half-written packet is way easier to
reason about than my "truncating proxy" suggestion.
> --- /dev/null
> +++ b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh
> @@ -0,0 +1,3 @@
> +printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result"
> +echo
> +printf "%s\n" "00"
Thanks for covering this case, too.
I confirmed (as I'm sure you did) they both cause git-remote-http to hang
without the fix in this patch.
-Peff
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2 5/7] remote-curl: error on incomplete packet
2020-05-18 16:22 ` Jeff King
@ 2020-05-18 16:51 ` Denton Liu
0 siblings, 0 replies; 62+ messages in thread
From: Denton Liu @ 2020-05-18 16:51 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Eric Sunshine
Hi Peff,
Thanks for reading over the patch carefully. It is much appreciated :)
On Mon, May 18, 2020 at 12:22:25PM -0400, Jeff King wrote:
> I'm not sure if it's worth replacing "4" with ARRAY_SIZE(state->len_buf).
> I generally try to avoid magic numbers, but it's certainly not like one
> could change the size of len_buf and this code would still be useful. :)
Yeah, I'm a bit sad about all the magic 4's that are strewn in the code
too. I was thinking about consolidating all of the magic 4's into a
constant in pkt-line as a preparatory patch but I didn't want to risk
holding up this series. I think once the dust settles here, I'll go
ahead and do that. For now, though, I think I'll leave it as is.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v2 6/7] pkt-line: PACKET_READ_RESPONSE_END
2020-05-18 15:47 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Denton Liu
` (4 preceding siblings ...)
2020-05-18 15:47 ` [PATCH v2 5/7] remote-curl: error on incomplete packet Denton Liu
@ 2020-05-18 15:47 ` Denton Liu
2020-05-18 15:47 ` [PATCH v2 7/7] stateless-connect: send response end packet Denton Liu
` (2 subsequent siblings)
8 siblings, 0 replies; 62+ messages in thread
From: Denton Liu @ 2020-05-18 15:47 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine
In a future commit, we will use PACKET_READ_RESPONSE_END to separate
messages proxied by remote-curl. To prepare for this, add the
PACKET_READ_RESPONSE_END enum value.
In switch statements that need a case added, die() or BUG() when a
PACKET_READ_RESPONSE_END is unexpected. Otherwise, mirror how
PACKET_READ_DELIM is implemented (especially in cases where packets are
being forwarded).
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
connect.c | 2 ++
pkt-line.c | 11 +++++++++++
pkt-line.h | 2 ++
remote-curl.c | 2 ++
serve.c | 2 ++
t/helper/test-pkt-line.c | 4 ++++
6 files changed, 23 insertions(+)
diff --git a/connect.c b/connect.c
index 23013c6344..11c6ec70a0 100644
--- a/connect.c
+++ b/connect.c
@@ -127,6 +127,7 @@ enum protocol_version discover_version(struct packet_reader *reader)
die_initial_contact(0);
case PACKET_READ_FLUSH:
case PACKET_READ_DELIM:
+ case PACKET_READ_RESPONSE_END:
version = protocol_v0;
break;
case PACKET_READ_NORMAL:
@@ -310,6 +311,7 @@ struct ref **get_remote_heads(struct packet_reader *reader,
state = EXPECTING_DONE;
break;
case PACKET_READ_DELIM:
+ case PACKET_READ_RESPONSE_END:
die(_("invalid packet"));
}
diff --git a/pkt-line.c b/pkt-line.c
index 5c3b4539b5..fdd6b90e68 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -99,6 +99,13 @@ void packet_delim(int fd)
die_errno(_("unable to write delim packet"));
}
+void packet_response_end(int fd)
+{
+ packet_trace("0002", 4, 1);
+ if (write_in_full(fd, "0002", 4) < 0)
+ die_errno(_("unable to write stateless separator packet"));
+}
+
int packet_flush_gently(int fd)
{
packet_trace("0000", 4, 1);
@@ -337,6 +344,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
packet_trace("0001", 4, 0);
*pktlen = 0;
return PACKET_READ_DELIM;
+ } else if (len == 2) {
+ packet_trace("0002", 4, 0);
+ *pktlen = 0;
+ return PACKET_READ_RESPONSE_END;
} else if (len < 4) {
die(_("protocol error: bad line length %d"), len);
}
diff --git a/pkt-line.h b/pkt-line.h
index de81776a7a..8156445873 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -22,6 +22,7 @@
*/
void packet_flush(int fd);
void packet_delim(int fd);
+void packet_response_end(int fd);
void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
void packet_buf_flush(struct strbuf *buf);
void packet_buf_delim(struct strbuf *buf);
@@ -94,6 +95,7 @@ enum packet_read_status {
PACKET_READ_NORMAL,
PACKET_READ_FLUSH,
PACKET_READ_DELIM,
+ PACKET_READ_RESPONSE_END,
};
enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
size_t *src_len, char *buffer,
diff --git a/remote-curl.c b/remote-curl.c
index e020140092..d02cb547e9 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -601,6 +601,8 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,
case PACKET_READ_FLUSH:
memcpy(buf - 4, "0000", 4);
break;
+ case PACKET_READ_RESPONSE_END:
+ die(_("remote server sent stateless separator"));
}
}
diff --git a/serve.c b/serve.c
index 317256c1a4..c046926ba1 100644
--- a/serve.c
+++ b/serve.c
@@ -217,6 +217,8 @@ static int process_request(void)
state = PROCESS_REQUEST_DONE;
break;
+ case PACKET_READ_RESPONSE_END:
+ BUG("unexpected stateless separator packet");
}
}
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 12ca698e17..69152958e5 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -46,6 +46,9 @@ static void unpack(void)
case PACKET_READ_DELIM:
printf("0001\n");
break;
+ case PACKET_READ_RESPONSE_END:
+ printf("0002\n");
+ break;
}
}
}
@@ -75,6 +78,7 @@ static void unpack_sideband(void)
case PACKET_READ_FLUSH:
return;
case PACKET_READ_DELIM:
+ case PACKET_READ_RESPONSE_END:
break;
}
}
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH v2 7/7] stateless-connect: send response end packet
2020-05-18 15:47 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Denton Liu
` (5 preceding siblings ...)
2020-05-18 15:47 ` [PATCH v2 6/7] pkt-line: PACKET_READ_RESPONSE_END Denton Liu
@ 2020-05-18 15:47 ` Denton Liu
2020-05-18 16:43 ` Jeff King
2020-05-18 16:50 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
2020-05-19 10:53 ` [PATCH v3 " Denton Liu
8 siblings, 1 reply; 62+ messages in thread
From: Denton Liu @ 2020-05-18 15:47 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine, Force Charlie
Currently, remote-curl acts as a proxy and blindly forwards packets
between an HTTP server and fetch-pack. In the case of a stateless RPC
connection where the connection is terminated before the transaction is
complete, remote-curl will blindly forward the packets before waiting on
more input from fetch-pack. Meanwhile, fetch-pack will read the
transaction and continue reading, expecting more input to continue the
transaction. This results in a deadlock between the two processes.
This can be seen in the following command which does not terminate:
$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012
Cloning into 'git'...
whereas the v1 version does terminate as expected:
$ git -c protocol.version=1 clone https://github.com/git/git.git --shallow-since=20151012
Cloning into 'git'...
fatal: the remote end hung up unexpectedly
Instead of blindly forwarding packets, make remote-curl insert response
end and flush packets after proxying the responses from the remote
server when using stateless_connect(). On the RPC client side, ensure
that each response ends as described.
A separate control packet is chosen because we need to be able to
differentiate between what the remote server sends and remote-curl's
control packets. By ensuring in the remote-curl code that a server
cannot send response end packets, we prevent a malicious server from
being able to perform a denial of service attack in which they spoof a
response end packet and cause the described deadlock to happen.
Reported-by: Force Charlie <charlieio@outlook.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/gitremote-helpers.txt | 4 +++-
Documentation/technical/protocol-v2.txt | 2 ++
builtin/fetch-pack.c | 2 +-
connect.c | 10 +++++++++-
fetch-pack.c | 12 ++++++++++++
remote-curl.c | 7 +++++++
remote.h | 3 ++-
t/t5702-protocol-v2.sh | 13 +++++++++++++
transport.c | 3 ++-
9 files changed, 51 insertions(+), 5 deletions(-)
diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index f48a031dc3..84f8e92b23 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -405,7 +405,9 @@ Supported if the helper has the "connect" capability.
trying to fall back). After line feed terminating the positive
(empty) response, the output of the service starts. Messages
(both request and response) must consist of zero or more
- PKT-LINEs, terminating in a flush packet. The client must not
+ PKT-LINEs, terminating in a flush packet. Response messages will
+ have a response end packet before the flush packet to indicate
+ the end of a response. The client must not
expect the server to store any state in between request-response
pairs. After the connection ends, the remote helper exits.
+
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 7e3766cafb..3996d70891 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -33,6 +33,8 @@ In protocol v2 these special packets will have the following semantics:
* '0000' Flush Packet (flush-pkt) - indicates the end of a message
* '0001' Delimiter Packet (delim-pkt) - separates sections of a message
+ * '0002' Message Packet (response-end-pkt) - indicates the end of a response
+ for stateless connections
Initial Client Request
----------------------
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 4771100072..94b0c89b82 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -224,7 +224,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
version = discover_version(&reader);
switch (version) {
case protocol_v2:
- get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
+ get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL, args.stateless_rpc);
break;
case protocol_v1:
case protocol_v0:
diff --git a/connect.c b/connect.c
index 11c6ec70a0..12b57f5c0a 100644
--- a/connect.c
+++ b/connect.c
@@ -409,7 +409,8 @@ static int process_ref_v2(const char *line, struct ref ***list)
struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
struct ref **list, int for_push,
const struct argv_array *ref_prefixes,
- const struct string_list *server_options)
+ const struct string_list *server_options,
+ int stateless_rpc)
{
int i;
*list = NULL;
@@ -446,6 +447,13 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
if (reader->status != PACKET_READ_FLUSH)
die(_("expected flush after ref listing"));
+ if (stateless_rpc) {
+ if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)
+ die(_("expected response end packet after ref listing"));
+ if (packet_reader_read(reader) != PACKET_READ_FLUSH)
+ die(_("expected flush packet after response end"));
+ }
+
return list;
}
diff --git a/fetch-pack.c b/fetch-pack.c
index f73a2ce6cb..bcbbb7e2fb 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1468,6 +1468,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
struct fetch_negotiator negotiator_alloc;
struct fetch_negotiator *negotiator;
int seen_ack = 0;
+ int check_http_delimiter;
if (args->no_dependents) {
negotiator = NULL;
@@ -1486,6 +1487,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
}
while (state != FETCH_DONE) {
+ check_http_delimiter = 0;
+
switch (state) {
case FETCH_CHECK_LOCAL:
sort_ref_list(&ref, ref_compare_name);
@@ -1542,6 +1545,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
/* fallthrough */
case NO_COMMON_FOUND:
state = FETCH_SEND_REQUEST;
+ check_http_delimiter = 1;
break;
}
break;
@@ -1562,10 +1566,18 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
die(_("git fetch-pack: fetch failed."));
state = FETCH_DONE;
+ check_http_delimiter = 1;
break;
case FETCH_DONE:
continue;
}
+
+ if (args->stateless_rpc && check_http_delimiter) {
+ if (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)
+ die(_("git fetch-pack: expected response end packet"));
+ if (packet_reader_read(&reader) != PACKET_READ_FLUSH)
+ die(_("git fetch-pack: expected flush packet"));
+ }
}
if (negotiator)
diff --git a/remote-curl.c b/remote-curl.c
index d02cb547e9..8a72b5ee7a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -703,6 +703,8 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si
state->remaining = packet_length(state->len_buf);
if (state->remaining < 0) {
die(_("remote-curl: bad line length character: %.4s"), state->len_buf);
+ } else if (state->remaining == 2) {
+ die(_("remote-curl: unexpected response end packet"));
} else if (state->remaining < 4) {
state->remaining = 0;
} else {
@@ -991,6 +993,11 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
if (rpc_in_data.pktline_state.remaining)
err = error(_("%d bytes of body are still expected"), rpc_in_data.pktline_state.remaining);
+ if (stateless_connect) {
+ packet_response_end(rpc->in);
+ packet_flush(rpc->in);
+ }
+
curl_slist_free_all(headers);
free(gzip_body);
return err;
diff --git a/remote.h b/remote.h
index 11d8719b58..5cc26c1b3b 100644
--- a/remote.h
+++ b/remote.h
@@ -179,7 +179,8 @@ struct ref **get_remote_heads(struct packet_reader *reader,
struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
struct ref **list, int for_push,
const struct argv_array *ref_prefixes,
- const struct string_list *server_options);
+ const struct string_list *server_options,
+ int stateless_rpc);
int resolve_remote_symref(struct ref *ref, struct ref *list);
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 4eb81ba2d4..8da65e60de 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -620,6 +620,19 @@ test_expect_success 'clone repository with http:// using protocol v2 with incomp
test_i18ngrep "bytes of body are still expected" err
'
+test_expect_success 'clone with http:// using protocol v2 and invalid parameters' '
+ test_when_finished "rm -f log" &&
+
+ test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" \
+ git -c protocol.version=2 \
+ clone --shallow-since=20151012 "$HTTPD_URL/smart/http_parent" http_child_invalid &&
+
+ # Client requested to use protocol v2
+ grep "Git-Protocol: version=2" log &&
+ # Server responded using protocol v2
+ grep "git< version 2" log
+'
+
test_expect_success 'clone big repository with http:// using protocol v2' '
test_when_finished "rm -f log" &&
diff --git a/transport.c b/transport.c
index a6002e502f..182978f4be 100644
--- a/transport.c
+++ b/transport.c
@@ -297,7 +297,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
if (must_list_refs)
get_remote_refs(data->fd[1], &reader, &refs, for_push,
ref_prefixes,
- transport->server_options);
+ transport->server_options,
+ transport->stateless_rpc);
break;
case protocol_v1:
case protocol_v0:
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* Re: [PATCH v2 7/7] stateless-connect: send response end packet
2020-05-18 15:47 ` [PATCH v2 7/7] stateless-connect: send response end packet Denton Liu
@ 2020-05-18 16:43 ` Jeff King
2020-05-18 17:12 ` Denton Liu
0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-18 16:43 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Eric Sunshine, Force Charlie
On Mon, May 18, 2020 at 11:47:24AM -0400, Denton Liu wrote:
> A separate control packet is chosen because we need to be able to
> differentiate between what the remote server sends and remote-curl's
> control packets. By ensuring in the remote-curl code that a server
> cannot send response end packets, we prevent a malicious server from
> being able to perform a denial of service attack in which they spoof a
> response end packet and cause the described deadlock to happen.
Thanks for thinking about this. I was wondering what mischief a server
might be able to cause by sending such a packet.
> @@ -446,6 +447,13 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
> if (reader->status != PACKET_READ_FLUSH)
> die(_("expected flush after ref listing"));
>
> + if (stateless_rpc) {
> + if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)
> + die(_("expected response end packet after ref listing"));
> + if (packet_reader_read(reader) != PACKET_READ_FLUSH)
> + die(_("expected flush packet after response end"));
> + }
Having two packets here surprised me. We'd have already received the
actual flush from the server (as you can see in the context above).
Wouldn't a single RESPONSE_END be enough to signal the reader?
> diff --git a/fetch-pack.c b/fetch-pack.c
> index f73a2ce6cb..bcbbb7e2fb 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1468,6 +1468,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> struct fetch_negotiator negotiator_alloc;
> struct fetch_negotiator *negotiator;
> int seen_ack = 0;
> + int check_http_delimiter;
This flag was more complicated than I expected, and I'm not sure how we
can easily be certain that all necessary paths are covered.
E.g., in FETCH_PROCESS_ACKS, we'll always be reading a response via
process_acks(). Why do COMMON_FOUND and NO_COMMON_FOUND check for
end-of-response, but READY doesn't? I think the answer is that we'd
continue to read the same response via FETCH_GET_PACK in this instance.
I just wonder if there is a better place to put this logic that would be
more certain to catch every place we'd expect to read to the end of a
response. But I suppose not. We could push it down into process_acks(),
but it would have the same READY logic that's here. I'll admit part of
my complaint is that the existing do_fetch_pack_v2 state-machine switch
is kind of hard to follow, but that's not your fault.
Two things that might help:
- put a comment in READY to indicate why we don't need to check
end-of-response (but do for its sibling cases)
- instead of setting and clearing a variable and then using it to
check at the end of the loop, call check_http_delimiter(&reader,
args->stateless_rpc) at the moment we're done with the packet. The
actual executed code would have the same behavior, I think, but it
might be easier to understand that we're trying to hit the end of
each response.
Also, it probably should be check_stateless_delimiter() or something.
There could be other helpers that support stateless-connect besides
http.
Speaking of which: this is a change to the remote-helper protocol, since
we're now expecting stateless-connect helpers to produce these delim
packets (and failing if they don't). I kind of doubt that anybody but
remote-curl has implemented v2 stateless-connect, but should we be
marking this with an extra capability to be on the safe side?
> + if (args->stateless_rpc && check_http_delimiter) {
> + if (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)
> + die(_("git fetch-pack: expected response end packet"));
> + if (packet_reader_read(&reader) != PACKET_READ_FLUSH)
> + die(_("git fetch-pack: expected flush packet"));
> + }
I wondered also if stateless_rpc might catch cases besides
stateless-connect. But I guess we're in the v2 fetch-pack path, and that
_only_ understands stateless-connect.
-Peff
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2 7/7] stateless-connect: send response end packet
2020-05-18 16:43 ` Jeff King
@ 2020-05-18 17:12 ` Denton Liu
2020-05-18 17:26 ` Jeff King
0 siblings, 1 reply; 62+ messages in thread
From: Denton Liu @ 2020-05-18 17:12 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Eric Sunshine, Force Charlie
Hi Peff,
On Mon, May 18, 2020 at 12:43:08PM -0400, Jeff King wrote:
> On Mon, May 18, 2020 at 11:47:24AM -0400, Denton Liu wrote:
[...]
> > @@ -446,6 +447,13 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
> > if (reader->status != PACKET_READ_FLUSH)
> > die(_("expected flush after ref listing"));
> >
> > + if (stateless_rpc) {
> > + if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)
> > + die(_("expected response end packet after ref listing"));
> > + if (packet_reader_read(reader) != PACKET_READ_FLUSH)
> > + die(_("expected flush packet after response end"));
> > + }
>
> Having two packets here surprised me. We'd have already received the
> actual flush from the server (as you can see in the context above).
> Wouldn't a single RESPONSE_END be enough to signal the reader?
Yes, having a single RESPONSE_END is enough. I sent a flush packet
because it seemed appropriate when I was writing the patch to end all
messages with a flush but I suppose that's just cargo culting. I'll
remove it in the next iteration.
> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index f73a2ce6cb..bcbbb7e2fb 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -1468,6 +1468,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> > struct fetch_negotiator negotiator_alloc;
> > struct fetch_negotiator *negotiator;
> > int seen_ack = 0;
> > + int check_http_delimiter;
>
> This flag was more complicated than I expected, and I'm not sure how we
> can easily be certain that all necessary paths are covered.
Essentially, any time we're in a path where state is set to
FETCH_SEND_REQUEST or FETCH_DONE, we know that we've finished processing
the current request and we can check for response end packets. Of course,
the one exception to this is the first iteration of the loop when we're
transitioning from FETCH_CHECK_LOCAL to FETCH_SEND_REQUEST where we
can't check for response end packets since nothing has been requested
yet.
> E.g., in FETCH_PROCESS_ACKS, we'll always be reading a response via
> process_acks(). Why do COMMON_FOUND and NO_COMMON_FOUND check for
> end-of-response, but READY doesn't? I think the answer is that we'd
> continue to read the same response via FETCH_GET_PACK in this instance.
>
> I just wonder if there is a better place to put this logic that would be
> more certain to catch every place we'd expect to read to the end of a
> response. But I suppose not. We could push it down into process_acks(),
> but it would have the same READY logic that's here. I'll admit part of
> my complaint is that the existing do_fetch_pack_v2 state-machine switch
> is kind of hard to follow, but that's not your fault.
I debated between the current implementation and doing something like
int first_iteration = 1;
...
while (state != FETCH_DONE) {
switch (...) {
...
}
if (args->stateless_rpc && !first_iteration && (state == FETCH_SEND_REQUEST || state == FETCH_DONE)) {
if (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)
die(_("git fetch-pack: expected response end packet"));
if (packet_reader_read(&reader) != PACKET_READ_FLUSH)
die(_("git fetch-pack: expected flush packet"));
}
first_iteration = 0;
}
I think that this catches _all_ the cases without fiddling with any of
the state machine logic.
[...]
> Also, it probably should be check_stateless_delimiter() or something.
> There could be other helpers that support stateless-connect besides
> http.
Yeah... I forgot to change the check_http_delimiter name when I was
doing cleanup. Whoops.
> Speaking of which: this is a change to the remote-helper protocol, since
> we're now expecting stateless-connect helpers to produce these delim
> packets (and failing if they don't). I kind of doubt that anybody but
> remote-curl has implemented v2 stateless-connect, but should we be
> marking this with an extra capability to be on the safe side?
I think that we're probably safe from breaking anything external.
According to the gitremote-helpers documentation,
'stateless-connect'::
Experimental; for internal use only.
so I think that gives us a bit of leeway in terms of the changes we can
make to the stateless-connect protocol. They've been warned ;)
Thanks,
Denton
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2 7/7] stateless-connect: send response end packet
2020-05-18 17:12 ` Denton Liu
@ 2020-05-18 17:26 ` Jeff King
0 siblings, 0 replies; 62+ messages in thread
From: Jeff King @ 2020-05-18 17:26 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Eric Sunshine, Force Charlie
On Mon, May 18, 2020 at 01:12:49PM -0400, Denton Liu wrote:
> > I just wonder if there is a better place to put this logic that would be
> > more certain to catch every place we'd expect to read to the end of a
> > response. But I suppose not. We could push it down into process_acks(),
> > but it would have the same READY logic that's here. I'll admit part of
> > my complaint is that the existing do_fetch_pack_v2 state-machine switch
> > is kind of hard to follow, but that's not your fault.
>
> I debated between the current implementation and doing something like
>
> int first_iteration = 1;
>
> ...
>
> while (state != FETCH_DONE) {
> switch (...) {
> ...
> }
>
> if (args->stateless_rpc && !first_iteration && (state == FETCH_SEND_REQUEST || state == FETCH_DONE)) {
> if (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)
> die(_("git fetch-pack: expected response end packet"));
> if (packet_reader_read(&reader) != PACKET_READ_FLUSH)
> die(_("git fetch-pack: expected flush packet"));
> }
> first_iteration = 0;
> }
>
> I think that this catches _all_ the cases without fiddling with any of
> the state machine logic.
I think you're right that it does, but TBH I find it harder to follow,
because now we're even further away from the actual response reads. What
I most want to verify is:
- there is no response-reading code path that does not check the
delimiters
- there is no code path where we check delimiters but did not actually
read a response
So to me, putting the check as close to the response-reads as possible
makes sense. I think the patch below is equivalent and easier to verify,
but I admit it's somewhat subjective:
diff --git a/fetch-pack.c b/fetch-pack.c
index 5325649eda..0bf1e5760c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1443,6 +1443,17 @@ static void receive_wanted_refs(struct packet_reader *reader,
die(_("error processing wanted refs: %d"), reader->status);
}
+static void check_stateless_delimiter(struct fetch_pack_args *args,
+ struct packet_reader *reader)
+{
+ if (!args->stateless_rpc)
+ return; /* not in stateless mode, no delimiter expected */
+ if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)
+ die(_("git fetch-pack: expected response end packet"));
+ if (packet_reader_read(reader) != PACKET_READ_FLUSH)
+ die(_("git fetch-pack: expected flush packet"));
+}
+
enum fetch_state {
FETCH_CHECK_LOCAL = 0,
FETCH_SEND_REQUEST,
@@ -1469,7 +1480,6 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
struct fetch_negotiator negotiator_alloc;
struct fetch_negotiator *negotiator;
int seen_ack = 0;
- int check_http_delimiter;
if (args->no_dependents) {
negotiator = NULL;
@@ -1488,8 +1498,6 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
}
while (state != FETCH_DONE) {
- check_http_delimiter = 0;
-
switch (state) {
case FETCH_CHECK_LOCAL:
sort_ref_list(&ref, ref_compare_name);
@@ -1538,15 +1546,19 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
/* Process ACKs/NAKs */
switch (process_acks(negotiator, &reader, &common)) {
case READY:
+ /*
+ * Don't check for response delimiter; get_pack() will
+ * read the rest of this response.
+ */
state = FETCH_GET_PACK;
break;
case COMMON_FOUND:
in_vain = 0;
seen_ack = 1;
/* fallthrough */
case NO_COMMON_FOUND:
state = FETCH_SEND_REQUEST;
- check_http_delimiter = 1;
+ check_stateless_delimiter(args, &reader);
break;
}
break;
@@ -1565,20 +1577,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
process_section_header(&reader, "packfile", 0);
if (get_pack(args, fd, pack_lockfile, sought, nr_sought))
die(_("git fetch-pack: fetch failed."));
+ check_stateless_delimiter(args, &reader);
state = FETCH_DONE;
- check_http_delimiter = 1;
break;
case FETCH_DONE:
continue;
}
-
- if (args->stateless_rpc && check_http_delimiter) {
- if (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)
- die(_("git fetch-pack: expected response end packet"));
- if (packet_reader_read(&reader) != PACKET_READ_FLUSH)
- die(_("git fetch-pack: expected flush packet"));
- }
}
if (negotiator)
> > Speaking of which: this is a change to the remote-helper protocol, since
> > we're now expecting stateless-connect helpers to produce these delim
> > packets (and failing if they don't). I kind of doubt that anybody but
> > remote-curl has implemented v2 stateless-connect, but should we be
> > marking this with an extra capability to be on the safe side?
>
> I think that we're probably safe from breaking anything external.
> According to the gitremote-helpers documentation,
>
> 'stateless-connect'::
> Experimental; for internal use only.
>
> so I think that gives us a bit of leeway in terms of the changes we can
> make to the stateless-connect protocol. They've been warned ;)
OK, I didn't realize we had explicitly marked it as experimental. I
agree we're probably fine here, then (now that v2 is starting to settle
a bit more, we might think about lifting that warning, but this patch
series shows we're not quite there yet :) ).
-Peff
^ permalink raw reply related [flat|nested] 62+ messages in thread
* Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects
2020-05-18 15:47 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Denton Liu
` (6 preceding siblings ...)
2020-05-18 15:47 ` [PATCH v2 7/7] stateless-connect: send response end packet Denton Liu
@ 2020-05-18 16:50 ` Jeff King
2020-05-18 17:36 ` Denton Liu
2020-05-18 19:36 ` Junio C Hamano
2020-05-19 10:53 ` [PATCH v3 " Denton Liu
8 siblings, 2 replies; 62+ messages in thread
From: Jeff King @ 2020-05-18 16:50 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Eric Sunshine
On Mon, May 18, 2020 at 11:47:17AM -0400, Denton Liu wrote:
> Changes since v1:
>
> * Remove fallthrough in switch in favour of just extracting the common
> call out of the switch in patch 3
>
> * Add more detail in function comment and use `const char linelen[4]` in
> patch 4
>
> * Implement most of Peff's suggestions[0] in patch 5
>
> * Only operate on stateless_connect() in patch 5
>
> * Add tests in patch 5
>
> * Drop "remote-curl: ensure last packet is a flush" in favour of
> "stateless-connect: send response end packet"
Overall this looks pretty cleanly done. I left a few minor comments
throughout, but the real question is whether we prefer the "0002" packet
in the last one, or if we instead insist that the response end in a
flush.
At first glance, the "0002" seems like it's more flexible, because we're
making fewer assumptions about what's being transferred over the
stateless-connect channel. But in reality it still has to be pktlines
(because we're checking them for incomplete or invalid packets already).
So all it really buys us is that the server response doesn't have to end
with a flush packet.
So I dunno. The "0002" solution is slightly more flexible, but I'm not
sure it helps in practice. And it does eat up one of our two remaining
special packet markers.
There is another solution, which would allow arbitrary data over
stateless-connect: adding an extra level of pktline framing between the
helper and the parent process. But that's rather ugly (inner pktlines
may be split across outer ones, so you have to do a bunch of buffer
reassembly). I think that's actually how v0 http works, IIRC.
IMHO it probably isn't worth pursuing. That extra layer wrecks the
elegance to the v2 stateless-connect approach, and we really do expect
only pktlines to go over it.
So I think either of your solutions (enforcing a final flush, or the
0002 packet) is preferable. I'm on the fence between them.
-Peff
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects
2020-05-18 16:50 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
@ 2020-05-18 17:36 ` Denton Liu
2020-05-18 20:58 ` Jeff King
2020-05-18 19:36 ` Junio C Hamano
1 sibling, 1 reply; 62+ messages in thread
From: Denton Liu @ 2020-05-18 17:36 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Eric Sunshine
Hi Peff,
On Mon, May 18, 2020 at 12:50:56PM -0400, Jeff King wrote:
> On Mon, May 18, 2020 at 11:47:17AM -0400, Denton Liu wrote:
>
> > Changes since v1:
> >
> > * Remove fallthrough in switch in favour of just extracting the common
> > call out of the switch in patch 3
> >
> > * Add more detail in function comment and use `const char linelen[4]` in
> > patch 4
> >
> > * Implement most of Peff's suggestions[0] in patch 5
> >
> > * Only operate on stateless_connect() in patch 5
> >
> > * Add tests in patch 5
> >
> > * Drop "remote-curl: ensure last packet is a flush" in favour of
> > "stateless-connect: send response end packet"
>
> Overall this looks pretty cleanly done. I left a few minor comments
> throughout, but the real question is whether we prefer the "0002" packet
> in the last one, or if we instead insist that the response end in a
> flush.
Thanks for the prompt review!
> At first glance, the "0002" seems like it's more flexible, because we're
> making fewer assumptions about what's being transferred over the
> stateless-connect channel. But in reality it still has to be pktlines
> (because we're checking them for incomplete or invalid packets already).
> So all it really buys us is that the server response doesn't have to end
> with a flush packet.
>
> So I dunno. The "0002" solution is slightly more flexible, but I'm not
> sure it helps in practice. And it does eat up one of our two remaining
> special packet markers.
Yeah, I was worried about consuming a special packet. One alternative
that I considered but is kind of gross is sending something like
"0028gitremote-helper: response complete\n" instead of "0002". Then,
instead of "0002" checks, we can check for that special string instead.
I don't _think_ that stateless-connect currently allows for completely
arbitrary data but I might be mistaken.
> There is another solution, which would allow arbitrary data over
> stateless-connect: adding an extra level of pktline framing between the
> helper and the parent process. But that's rather ugly (inner pktlines
> may be split across outer ones, so you have to do a bunch of buffer
> reassembly). I think that's actually how v0 http works, IIRC.
> IMHO it probably isn't worth pursuing. That extra layer wrecks the
> elegance to the v2 stateless-connect approach, and we really do expect
> only pktlines to go over it.
This was the original approach that I was working on but I found it to
be much too invasive for my liking. (Also, I never actually managed to
get it working ;) ) I think I gave up when I realised I had to insert
reframing logic into index-pack and unpack-objects.
> So I think either of your solutions (enforcing a final flush, or the
> 0002 packet) is preferable. I'm on the fence between them.
I'm mostly on the fence too. One advantage of 0002, however, is that a
malicious server can't end a request with 0002 as that's explicitly
prevented. If a malicious server closes a connection after sending a
0000, I think that they could cause a deadlock to happen if there are
multiple flush packets expected in a response. You mentioned in a
sibling email that this currently doesn't happen wrt stateless-connect
although I'm not sure if in the future, this is something that might
change. I dunno.
> -Peff
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects
2020-05-18 17:36 ` Denton Liu
@ 2020-05-18 20:58 ` Jeff King
2020-05-18 22:52 ` Junio C Hamano
0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-18 20:58 UTC (permalink / raw)
To: Denton Liu; +Cc: Junio C Hamano, Git Mailing List, Eric Sunshine
On Mon, May 18, 2020 at 01:36:52PM -0400, Denton Liu wrote:
> > So I dunno. The "0002" solution is slightly more flexible, but I'm not
> > sure it helps in practice. And it does eat up one of our two remaining
> > special packet markers.
>
> Yeah, I was worried about consuming a special packet. One alternative
> that I considered but is kind of gross is sending something like
> "0028gitremote-helper: response complete\n" instead of "0002". Then,
> instead of "0002" checks, we can check for that special string instead.
> I don't _think_ that stateless-connect currently allows for completely
> arbitrary data but I might be mistaken.
Yeah, I think we should avoid using any packet that could be confused
with regular output. A well-functioning server should have sent a flush
already, and so the worst case would probably be that we would mistake
their "0028" packet for ours if the network happens to cut out right at
that moment (and even that is obviously exceedingly unlikely). But it
just seems easier to reason about if we know we can't get confused.
> This was the original approach that I was working on but I found it to
> be much too invasive for my liking. (Also, I never actually managed to
> get it working ;) ) I think I gave up when I realised I had to insert
> reframing logic into index-pack and unpack-objects.
Yuck. :) I think v0 does it by unwrapping the extra layer in fetch-pack,
but I'd have to double check. But I'm not going to, because I think we
both agree that your other approaches are way nicer.
> > So I think either of your solutions (enforcing a final flush, or the
> > 0002 packet) is preferable. I'm on the fence between them.
>
> I'm mostly on the fence too. One advantage of 0002, however, is that a
> malicious server can't end a request with 0002 as that's explicitly
> prevented. If a malicious server closes a connection after sending a
> 0000, I think that they could cause a deadlock to happen if there are
> multiple flush packets expected in a response. You mentioned in a
> sibling email that this currently doesn't happen wrt stateless-connect
> although I'm not sure if in the future, this is something that might
> change. I dunno.
Yeah, agreed that the efficacy of the "must end in flush" strategy
depends on there not being internal flushes. At least against a
determined attacker. But it may also be less random than you might
think, if the pattern is to send a flush, then do a bunch of work, etc.
A "random" hangup or error might be more likely to happen at the flush
points then (from the client's perspective).
So let's see if we can answer that question with less hand-waving.
The outer v2 capabilities conversation only writes out the capabilities
list, followed by a single flush (the packet_flush() call in
advertise_capabilities()). So far so good.
The only two v2 commands defined in serve.c are "ls-refs" and "fetch".
For "ls-refs", we end up in ls_refs(). That writes only regular packets
via send_ref(), and then concludes with a single flush (we don't even
send a delim; the client does that to specify options). Good.
For "fetch", we end up in upload_pack_v2(). We write via
process_haves_and_send_acks(), which will either:
1. Write nothing and return 1.
2. Write a delim and return 1.
3. Write a flush packet and return 0.
In the final case, we jump to FETCH_DONE. So this really is the final
thing we say. Good.
In the first two cases, we jump to FETCH_SEND_PACK. We may write out
wanted-ref info, followed by a delim. OK. Then shallow info, followed by
a delim. OK. Then the actual packfile via create_pack_file(). There our
write behavior depends on use_sideband. If not set, we just dump data
directly. If set, then we packetize. And it will always be set for v2
(we do it unconditionally in the beginning of upload_pack_v2()).
So we only need to care about use_sideband cases. And there we may send
keepalives, but those are "0005\1" empty data packets. OK. We may send
data via send_client_data(), but those will be real data packets. And
then finally we give a single packet_flush(). Good.
So I think we can say that yes, the protocol as designed will send a
flush at the end of every response, and will not ever send another
flush.
_But_ that's if all goes well. If upload-pack sees an error, it may:
- write an ERR packet in the earlier non-sideband steps, and then die
- write a sideband 3 packet in later steps, and then die
- just die() in some cases (e.g., pack-objects failed to start)
Obviously these are all an error on the client. And that third one
especially is probably a hanging case that we'd like to fix with this
series. But in the other two, we'd definitely want to make sure that
remote-curl doesn't complain too early or too loudly, and that the error
is shown to the user.
I think if remote-curl is just asking "did we see a flush packet at the
end" then it is likely to complain unnecessarily in that case. And I
don't think it should be inspecting packets for ERR or sideband-3. It
doesn't know enough about the rest of the conversation to know which is
correct. Though I guess we'd really only have to inspect the final
packet.
I really wish we had converted all ERRs into packet 0002 as part of the
v2 conversion. Then somebody reading only at the pktline level could
truly understand or proxy a full conversation without understanding
anything about what's in the pktlines. But it's too late for that now.
So I think our options are probably:
1. detect flush packets in remote-curl, and either:
a. don't print an error, just hang up. That prevents a hang in the
caller and produces no extra message on a real error. It may be
less informative than it could be if the connection hangs up
(though we may print a curl error message, and the caller will
at least say "the helper hung up")
b. like (a), but always print an error; this is your original
patch, but I _suspect_ (but didn't test) that it would produce
extra useless messages for errors the server reports
c. between the two: inspect the final packet data for evidence of
ERR/sideband 3 and suppress any message if found
2. helper signals end-of-response to caller (then it never produces a
message itself; only the caller does, and it would abort on an ERR
packet before then)
a. using a special pktline (your "0002" patch)
b. some other out-of-band mechanism (e.g., could be another fd)
I think this is pushing me towards 2a, your "0002" patch. It sidesteps
the error-message questions entirely (and I think 2b is too convoluted
to be worth pursuing, especially on Windows where setting up extra pipes
is tricky). But I'd also be OK with 1a or 1c.
-Peff
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects
2020-05-18 20:58 ` Jeff King
@ 2020-05-18 22:52 ` Junio C Hamano
2020-05-19 2:38 ` Jeff King
0 siblings, 1 reply; 62+ messages in thread
From: Junio C Hamano @ 2020-05-18 22:52 UTC (permalink / raw)
To: Jeff King; +Cc: Denton Liu, Git Mailing List, Eric Sunshine
Jeff King <peff@peff.net> writes:
> So I think our options are probably:
>
> 1. detect flush packets in remote-curl, and either:
>
> a. don't print an error, just hang up. That prevents a hang in the
> caller and produces no extra message on a real error. It may be
> less informative than it could be if the connection hangs up
> (though we may print a curl error message, and the caller will
> at least say "the helper hung up")
>
> b. like (a), but always print an error; this is your original
> patch, but I _suspect_ (but didn't test) that it would produce
> extra useless messages for errors the server reports
>
> c. between the two: inspect the final packet data for evidence of
> ERR/sideband 3 and suppress any message if found
>
> 2. helper signals end-of-response to caller (then it never produces a
> message itself; only the caller does, and it would abort on an ERR
> packet before then)
>
> a. using a special pktline (your "0002" patch)
>
> b. some other out-of-band mechanism (e.g., could be another fd)
>
> I think this is pushing me towards 2a, your "0002" patch. It sidesteps
> the error-message questions entirely (and I think 2b is too convoluted
> to be worth pursuing, especially on Windows where setting up extra pipes
> is tricky). But I'd also be OK with 1a or 1c.
Thanks for a detailed analysis. I guess we'd take 0002, then?
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects
2020-05-18 22:52 ` Junio C Hamano
@ 2020-05-19 2:38 ` Jeff King
0 siblings, 0 replies; 62+ messages in thread
From: Jeff King @ 2020-05-19 2:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Denton Liu, Git Mailing List, Eric Sunshine
On Mon, May 18, 2020 at 03:52:42PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > So I think our options are probably:
> >
> > 1. detect flush packets in remote-curl, and either:
> >
> > a. don't print an error, just hang up. That prevents a hang in the
> > caller and produces no extra message on a real error. It may be
> > less informative than it could be if the connection hangs up
> > (though we may print a curl error message, and the caller will
> > at least say "the helper hung up")
> >
> > b. like (a), but always print an error; this is your original
> > patch, but I _suspect_ (but didn't test) that it would produce
> > extra useless messages for errors the server reports
> >
> > c. between the two: inspect the final packet data for evidence of
> > ERR/sideband 3 and suppress any message if found
> >
> > 2. helper signals end-of-response to caller (then it never produces a
> > message itself; only the caller does, and it would abort on an ERR
> > packet before then)
> >
> > a. using a special pktline (your "0002" patch)
> >
> > b. some other out-of-band mechanism (e.g., could be another fd)
> >
> > I think this is pushing me towards 2a, your "0002" patch. It sidesteps
> > the error-message questions entirely (and I think 2b is too convoluted
> > to be worth pursuing, especially on Windows where setting up extra pipes
> > is tricky). But I'd also be OK with 1a or 1c.
>
> Thanks for a detailed analysis. I guess we'd take 0002, then?
Yeah, that's how I lean. I think I did have a few comments on the patch
that I'd like Denton to consider, so hopefully we'll see a re-roll.
-Peff
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects
2020-05-18 16:50 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
2020-05-18 17:36 ` Denton Liu
@ 2020-05-18 19:36 ` Junio C Hamano
1 sibling, 0 replies; 62+ messages in thread
From: Junio C Hamano @ 2020-05-18 19:36 UTC (permalink / raw)
To: Jeff King; +Cc: Denton Liu, Git Mailing List, Eric Sunshine
Jeff King <peff@peff.net> writes:
> Overall this looks pretty cleanly done. I left a few minor comments
> throughout, but the real question is whether we prefer the "0002" packet
> in the last one, or if we instead insist that the response end in a
> flush.
Thanks for a review. I was reading the series through and I found
it a reasonably pleasant read.
> So I think either of your solutions (enforcing a final flush, or the
> 0002 packet) is preferable. I'm on the fence between them.
I am undecided, too X-<.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects
2020-05-18 15:47 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Denton Liu
` (7 preceding siblings ...)
2020-05-18 16:50 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
@ 2020-05-19 10:53 ` Denton Liu
2020-05-19 10:53 ` [PATCH v3 1/7] remote-curl: fix typo Denton Liu
` (9 more replies)
8 siblings, 10 replies; 62+ messages in thread
From: Denton Liu @ 2020-05-19 10:53 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine, Junio C Hamano
The following command hangs forever:
$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012
Cloning into 'git'...
This occurs because the --shallow-since arg is incorrect and the server
dies early. However, remote-curl does not realise that the server
errored out and just faithfully forwards the packets to fetch-pack
before waiting on more input from fetch-pack. Meanwhile, fetch-pack
keeps reading as it still expects more input. As a result, the processes
deadlock. Original analysis by Peff:
https://lore.kernel.org/git/20200328154936.GA1217052@coredump.intra.peff.net/
Changes since v2:
* Use if-else tower in "transport: extract common fetch_pack() call"
* Rename to `lenbuf_hex` and remove null-terminator sentence in
"pkt-line: extern packet_length()"
* Fix "a a" typo in "remote-curl: error on incomplete packet"
* Don't send flush packet after response end packet
* Move stateless delimiter checks closer to where message processing
happens in do_fetch_pack_v2()
Changes since v1:
* Remove fallthrough in switch in favour of just extracting the common
call out of the switch in patch 3
* Add more detail in function comment and use `const char linelen[4]` in
patch 4
* Implement most of Peff's suggestions[0] in patch 5
* Only operate on stateless_connect() in patch 5
* Add tests in patch 5
* Drop "remote-curl: ensure last packet is a flush" in favour of
"stateless-connect: send response end packet"
[0]: https://lore.kernel.org/git/20200515213844.GD115445@coredump.intra.peff.net/
Denton Liu (7):
remote-curl: fix typo
remote-curl: remove label indentation
transport: extract common fetch_pack() call
pkt-line: extern packet_length()
remote-curl: error on incomplete packet
pkt-line: define PACKET_READ_RESPONSE_END
stateless-connect: send response end packet
Documentation/gitremote-helpers.txt | 4 +-
Documentation/technical/protocol-v2.txt | 2 +
builtin/fetch-pack.c | 2 +-
connect.c | 18 ++++-
connect.h | 4 ++
fetch-pack.c | 13 ++++
pkt-line.c | 17 ++++-
pkt-line.h | 11 +++
remote-curl.c | 70 +++++++++++++++++--
remote.h | 3 +-
serve.c | 2 +
t/helper/test-pkt-line.c | 4 ++
t/lib-httpd.sh | 2 +
t/lib-httpd/apache.conf | 8 +++
.../incomplete-body-upload-pack-v2-http.sh | 3 +
.../incomplete-length-upload-pack-v2-http.sh | 3 +
t/t5702-protocol-v2.sh | 47 +++++++++++++
transport.c | 28 +++-----
18 files changed, 211 insertions(+), 30 deletions(-)
create mode 100644 t/lib-httpd/incomplete-body-upload-pack-v2-http.sh
create mode 100644 t/lib-httpd/incomplete-length-upload-pack-v2-http.sh
Range-diff against v2:
1: b390875f87 = 1: b390875f87 remote-curl: fix typo
2: a2b28c0b28 = 2: a2b28c0b28 remote-curl: remove label indentation
3: 3a42575bd5 < -: ---------- transport: extract common fetch_pack() call
-: ---------- > 3: c118baa5a2 transport: extract common fetch_pack() call
4: c2b9d033bb ! 4: 36885943b2 pkt-line: extern packet_length()
@@ Commit message
need to access the length header. In order to simplify this, extern
packet_length() so that the logic can be reused.
- Change the function parameter from a `const char *` to
- `const char linelen[4]`. Even though these two types behave identically
- as function parameters, use the array notation to semantically indicate
- exactly what this function is expecting as an argument.
+ Change the function parameter from `const char *linelen` to
+ `const char lenbuf_hex[4]`. Even though these two types behave
+ identically as function parameters, use the array notation to
+ semantically indicate exactly what this function is expecting as an
+ argument. Also, rename it from linelen to lenbuf_hex as the former
+ sounds like it should be an integral type which is misleading.
## pkt-line.c ##
@@ pkt-line.c: static int get_packet_data(int fd, char **src_buf, size_t *src_size,
@@ pkt-line.c: static int get_packet_data(int fd, char **src_buf, size_t *src_size,
}
-static int packet_length(const char *linelen)
-+int packet_length(const char linelen[4])
++int packet_length(const char lenbuf_hex[4])
{
- int val = hex2chr(linelen);
- return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
+- int val = hex2chr(linelen);
+- return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
++ int val = hex2chr(lenbuf_hex);
++ return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
+ }
+
+ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
## pkt-line.h ##
@@ pkt-line.h: int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
@@ pkt-line.h: int write_packetized_from_buf(const char *src_in, size_t len, int fd
+/*
+ * Convert a four hex digit packet line length header into its numeric
-+ * representation. linelen should not be null-terminated.
++ * representation.
+ *
+ * If linelen contains non-hex characters, return -1. Otherwise, return the
+ * numeric value of the length header.
+ */
-+int packet_length(const char linelen[4]);
++int packet_length(const char lenbuf_hex[4]);
+
/*
* Read a packetized line into a buffer like the 'packet_read()' function but
5: 52ce5fdffd ! 5: 91d330620a remote-curl: error on incomplete packet
@@ Commit message
results in a deadlock between the two processes.
For a stateless connection, inspect packets before sending them and
- error out if a a packet line packet is incomplete.
+ error out if a packet line packet is incomplete.
Helped-by: Jeff King <peff@peff.net>
6: 744b078324 ! 6: ff83344e9e pkt-line: PACKET_READ_RESPONSE_END
@@ Metadata
Author: Denton Liu <liu.denton@gmail.com>
## Commit message ##
- pkt-line: PACKET_READ_RESPONSE_END
+ pkt-line: define PACKET_READ_RESPONSE_END
In a future commit, we will use PACKET_READ_RESPONSE_END to separate
messages proxied by remote-curl. To prepare for this, add the
7: 4b079bcd83 ! 7: c26e160fbc stateless-connect: send response end packet
@@ Commit message
Cloning into 'git'...
fatal: the remote end hung up unexpectedly
- Instead of blindly forwarding packets, make remote-curl insert response
- end and flush packets after proxying the responses from the remote
- server when using stateless_connect(). On the RPC client side, ensure
- that each response ends as described.
+ Instead of blindly forwarding packets, make remote-curl insert a
+ response end packet after proxying the responses from the remote server
+ when using stateless_connect(). On the RPC client side, ensure that each
+ response ends as described.
A separate control packet is chosen because we need to be able to
differentiate between what the remote server sends and remote-curl's
@@ Documentation/gitremote-helpers.txt: Supported if the helper has the "connect" c
(both request and response) must consist of zero or more
- PKT-LINEs, terminating in a flush packet. The client must not
+ PKT-LINEs, terminating in a flush packet. Response messages will
-+ have a response end packet before the flush packet to indicate
-+ the end of a response. The client must not
++ then have a response end packet after the flush packet to
++ indicate the end of a response. The client must not
expect the server to store any state in between request-response
pairs. After the connection ends, the remote helper exits.
+
@@ builtin/fetch-pack.c: int cmd_fetch_pack(int argc, const char **argv, const char
## connect.c ##
@@ connect.c: static int process_ref_v2(const char *line, struct ref ***list)
+ return ret;
+ }
+
++void check_stateless_delimiter(int stateless_rpc,
++ struct packet_reader *reader,
++ const char *error)
++{
++ if (!stateless_rpc)
++ return; /* not in stateless mode, no delimiter expected */
++ if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)
++ die("%s", error);
++}
++
struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
struct ref **list, int for_push,
const struct argv_array *ref_prefixes,
@@ connect.c: struct ref **get_remote_refs(int fd_out, struct packet_reader *reader
if (reader->status != PACKET_READ_FLUSH)
die(_("expected flush after ref listing"));
-+ if (stateless_rpc) {
-+ if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)
-+ die(_("expected response end packet after ref listing"));
-+ if (packet_reader_read(reader) != PACKET_READ_FLUSH)
-+ die(_("expected flush packet after response end"));
-+ }
++ check_stateless_delimiter(stateless_rpc, reader,
++ _("expected response end packet after ref listing"));
+
return list;
}
- ## fetch-pack.c ##
-@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
- struct fetch_negotiator negotiator_alloc;
- struct fetch_negotiator *negotiator;
- int seen_ack = 0;
-+ int check_http_delimiter;
+ ## connect.h ##
+@@ connect.h: int server_supports_v2(const char *c, int die_on_error);
+ int server_supports_feature(const char *c, const char *feature,
+ int die_on_error);
- if (args->no_dependents) {
- negotiator = NULL;
-@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
- }
-
- while (state != FETCH_DONE) {
-+ check_http_delimiter = 0;
++void check_stateless_delimiter(int stateless_rpc,
++ struct packet_reader *reader,
++ const char *error);
+
- switch (state) {
- case FETCH_CHECK_LOCAL:
- sort_ref_list(&ref, ref_compare_name);
+ #endif
+
+ ## fetch-pack.c ##
+@@ fetch-pack.c: enum fetch_state {
+ FETCH_DONE,
+ };
+
++static void do_check_stateless_delimiter(const struct fetch_pack_args *args,
++ struct packet_reader *reader)
++{
++ check_stateless_delimiter(args->stateless_rpc, reader,
++ _("git fetch-pack: expected response end packet"));
++}
++
+ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
+ int fd[2],
+ const struct ref *orig_ref,
@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
+ /* Process ACKs/NAKs */
+ switch (process_acks(negotiator, &reader, &common)) {
+ case READY:
++ /*
++ * Don't check for response delimiter; get_pack() will
++ * read the rest of this response.
++ */
+ state = FETCH_GET_PACK;
+ break;
+ case COMMON_FOUND:
+@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
+ seen_ack = 1;
/* fallthrough */
case NO_COMMON_FOUND:
++ do_check_stateless_delimiter(args, &reader);
state = FETCH_SEND_REQUEST;
-+ check_http_delimiter = 1;
break;
}
- break;
@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
+ process_section_header(&reader, "packfile", 0);
+ if (get_pack(args, fd, pack_lockfile, sought, nr_sought))
die(_("git fetch-pack: fetch failed."));
++ do_check_stateless_delimiter(args, &reader);
state = FETCH_DONE;
-+ check_http_delimiter = 1;
break;
- case FETCH_DONE:
- continue;
- }
-+
-+ if (args->stateless_rpc && check_http_delimiter) {
-+ if (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)
-+ die(_("git fetch-pack: expected response end packet"));
-+ if (packet_reader_read(&reader) != PACKET_READ_FLUSH)
-+ die(_("git fetch-pack: expected flush packet"));
-+ }
- }
-
- if (negotiator)
## remote-curl.c ##
@@ remote-curl.c: static void check_pktline(struct check_pktline_state *state, const char *ptr, si
@@ remote-curl.c: static int post_rpc(struct rpc_state *rpc, int stateless_connect,
if (rpc_in_data.pktline_state.remaining)
err = error(_("%d bytes of body are still expected"), rpc_in_data.pktline_state.remaining);
-+ if (stateless_connect) {
++ if (stateless_connect)
+ packet_response_end(rpc->in);
-+ packet_flush(rpc->in);
-+ }
+
curl_slist_free_all(headers);
free(gzip_body);
--
2.26.2.706.g87896c9627
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v3 1/7] remote-curl: fix typo
2020-05-19 10:53 ` [PATCH v3 " Denton Liu
@ 2020-05-19 10:53 ` Denton Liu
2020-05-19 10:53 ` [PATCH v3 2/7] remote-curl: remove label indentation Denton Liu
` (8 subsequent siblings)
9 siblings, 0 replies; 62+ messages in thread
From: Denton Liu @ 2020-05-19 10:53 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine, Junio C Hamano
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
remote-curl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/remote-curl.c b/remote-curl.c
index 1c9aa3d0ab..6844708f38 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -643,7 +643,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,
return 0;
}
/*
- * If avail is non-zerp, the line length for the flush still
+ * If avail is non-zero, the line length for the flush still
* hasn't been fully sent. Proceed with sending the line
* length.
*/
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH v3 2/7] remote-curl: remove label indentation
2020-05-19 10:53 ` [PATCH v3 " Denton Liu
2020-05-19 10:53 ` [PATCH v3 1/7] remote-curl: fix typo Denton Liu
@ 2020-05-19 10:53 ` Denton Liu
2020-05-19 10:53 ` [PATCH v3 3/7] transport: extract common fetch_pack() call Denton Liu
` (7 subsequent siblings)
9 siblings, 0 replies; 62+ messages in thread
From: Denton Liu @ 2020-05-19 10:53 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine, Junio C Hamano
In the codebase, labels are aligned to the leftmost column. Remove the
space-indentation from `free_specs:` to conform to this.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
remote-curl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/remote-curl.c b/remote-curl.c
index 6844708f38..da3e07184a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1276,7 +1276,7 @@ static void parse_push(struct strbuf *buf)
if (ret)
exit(128); /* error already reported */
- free_specs:
+free_specs:
argv_array_clear(&specs);
}
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH v3 3/7] transport: extract common fetch_pack() call
2020-05-19 10:53 ` [PATCH v3 " Denton Liu
2020-05-19 10:53 ` [PATCH v3 1/7] remote-curl: fix typo Denton Liu
2020-05-19 10:53 ` [PATCH v3 2/7] remote-curl: remove label indentation Denton Liu
@ 2020-05-19 10:53 ` Denton Liu
2020-05-19 10:53 ` [PATCH v3 4/7] pkt-line: extern packet_length() Denton Liu
` (6 subsequent siblings)
9 siblings, 0 replies; 62+ messages in thread
From: Denton Liu @ 2020-05-19 10:53 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine, Junio C Hamano
In the switch statement, the difference between the `protocol_v2` and
`protocol_v{1,0}` arms is a preparatory call to die_if_server_options() in
the latter. The fetch_pack() call is identical in both arms. However,
since this fetch_pack() call has so many parameters, it is not
immediately obvious that the call is identical in both cases.
Extract the common fetch_pack() call out of the switch statement so that
code duplication is reduced and the logic is more clear for future
readers. While we're at it, rewrite the switch statement as an if-else
tower for increased clarity.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
transport.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/transport.c b/transport.c
index 15f5ba4e8f..431a93caef 100644
--- a/transport.c
+++ b/transport.c
@@ -369,24 +369,15 @@ static int fetch_refs_via_pack(struct transport *transport,
refs_tmp = handshake(transport, 0, NULL, must_list_refs);
}
- switch (data->version) {
- case protocol_v2:
- refs = fetch_pack(&args, data->fd,
- refs_tmp ? refs_tmp : transport->remote_refs,
- to_fetch, nr_heads, &data->shallow,
- &transport->pack_lockfile, data->version);
- break;
- case protocol_v1:
- case protocol_v0:
- die_if_server_options(transport);
- refs = fetch_pack(&args, data->fd,
- refs_tmp ? refs_tmp : transport->remote_refs,
- to_fetch, nr_heads, &data->shallow,
- &transport->pack_lockfile, data->version);
- break;
- case protocol_unknown_version:
+ if (data->version == protocol_unknown_version)
BUG("unknown protocol version");
- }
+ else if (data->version <= protocol_v1)
+ die_if_server_options(transport);
+
+ refs = fetch_pack(&args, data->fd,
+ refs_tmp ? refs_tmp : transport->remote_refs,
+ to_fetch, nr_heads, &data->shallow,
+ &transport->pack_lockfile, data->version);
close(data->fd[0]);
close(data->fd[1]);
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH v3 4/7] pkt-line: extern packet_length()
2020-05-19 10:53 ` [PATCH v3 " Denton Liu
` (2 preceding siblings ...)
2020-05-19 10:53 ` [PATCH v3 3/7] transport: extract common fetch_pack() call Denton Liu
@ 2020-05-19 10:53 ` Denton Liu
2020-05-19 16:23 ` Eric Sunshine
2020-05-19 10:53 ` [PATCH v3 5/7] remote-curl: error on incomplete packet Denton Liu
` (5 subsequent siblings)
9 siblings, 1 reply; 62+ messages in thread
From: Denton Liu @ 2020-05-19 10:53 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine, Junio C Hamano
In a future commit, we will be manually processing packets and we will
need to access the length header. In order to simplify this, extern
packet_length() so that the logic can be reused.
Change the function parameter from `const char *linelen` to
`const char lenbuf_hex[4]`. Even though these two types behave
identically as function parameters, use the array notation to
semantically indicate exactly what this function is expecting as an
argument. Also, rename it from linelen to lenbuf_hex as the former
sounds like it should be an integral type which is misleading.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
pkt-line.c | 6 +++---
pkt-line.h | 9 +++++++++
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index a0e87b1e81..3beab1dc6b 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -306,10 +306,10 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,
return ret;
}
-static int packet_length(const char *linelen)
+int packet_length(const char lenbuf_hex[4])
{
- int val = hex2chr(linelen);
- return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
+ int val = hex2chr(lenbuf_hex);
+ return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
}
enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
diff --git a/pkt-line.h b/pkt-line.h
index fef3a0d792..0d92c5e17f 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -74,6 +74,15 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
int packet_read(int fd, char **src_buffer, size_t *src_len, char
*buffer, unsigned size, int options);
+/*
+ * Convert a four hex digit packet line length header into its numeric
+ * representation.
+ *
+ * If linelen contains non-hex characters, return -1. Otherwise, return the
+ * numeric value of the length header.
+ */
+int packet_length(const char lenbuf_hex[4]);
+
/*
* Read a packetized line into a buffer like the 'packet_read()' function but
* returns an 'enum packet_read_status' which indicates the status of the read.
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* Re: [PATCH v3 4/7] pkt-line: extern packet_length()
2020-05-19 10:53 ` [PATCH v3 4/7] pkt-line: extern packet_length() Denton Liu
@ 2020-05-19 16:23 ` Eric Sunshine
0 siblings, 0 replies; 62+ messages in thread
From: Eric Sunshine @ 2020-05-19 16:23 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Jeff King, Junio C Hamano
On Tue, May 19, 2020 at 6:54 AM Denton Liu <liu.denton@gmail.com> wrote:
> [...]
> Change the function parameter from `const char *linelen` to
> `const char lenbuf_hex[4]`. Even though these two types behave
> identically as function parameters, use the array notation to
> semantically indicate exactly what this function is expecting as an
> argument. Also, rename it from linelen to lenbuf_hex as the former
> sounds like it should be an integral type which is misleading.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/pkt-line.h b/pkt-line.h
> @@ -74,6 +74,15 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
> +/*
> + * Convert a four hex digit packet line length header into its numeric
> + * representation.
> + *
> + * If linelen contains non-hex characters, return -1. Otherwise, return the
s/linelen/lenbuf_hex/
...or...
s/lenbuf_hex/input argument/
> + * numeric value of the length header.
> + */
> +int packet_length(const char lenbuf_hex[4]);
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v3 5/7] remote-curl: error on incomplete packet
2020-05-19 10:53 ` [PATCH v3 " Denton Liu
` (3 preceding siblings ...)
2020-05-19 10:53 ` [PATCH v3 4/7] pkt-line: extern packet_length() Denton Liu
@ 2020-05-19 10:53 ` Denton Liu
2020-05-19 10:53 ` [PATCH v3 6/7] pkt-line: define PACKET_READ_RESPONSE_END Denton Liu
` (4 subsequent siblings)
9 siblings, 0 replies; 62+ messages in thread
From: Denton Liu @ 2020-05-19 10:53 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine, Junio C Hamano
Currently, remote-curl acts as a proxy and blindly forwards packets
between an HTTP server and fetch-pack. In the case of a stateless RPC
connection where the connection is terminated with a partially written
packet, remote-curl will blindly send the partially written packet
before waiting on more input from fetch-pack. Meanwhile, fetch-pack will
read the partial packet and continue reading, expecting more input. This
results in a deadlock between the two processes.
For a stateless connection, inspect packets before sending them and
error out if a packet line packet is incomplete.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
remote-curl.c | 59 ++++++++++++++++++-
t/lib-httpd.sh | 2 +
t/lib-httpd/apache.conf | 8 +++
.../incomplete-body-upload-pack-v2-http.sh | 3 +
.../incomplete-length-upload-pack-v2-http.sh | 3 +
t/t5702-protocol-v2.sh | 34 +++++++++++
6 files changed, 106 insertions(+), 3 deletions(-)
create mode 100644 t/lib-httpd/incomplete-body-upload-pack-v2-http.sh
create mode 100644 t/lib-httpd/incomplete-length-upload-pack-v2-http.sh
diff --git a/remote-curl.c b/remote-curl.c
index da3e07184a..e020140092 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -679,9 +679,53 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
}
#endif
+struct check_pktline_state {
+ char len_buf[4];
+ int len_filled;
+ int remaining;
+};
+
+static void check_pktline(struct check_pktline_state *state, const char *ptr, size_t size)
+{
+ while (size) {
+ if (!state->remaining) {
+ int digits_remaining = 4 - state->len_filled;
+ if (digits_remaining > size)
+ digits_remaining = size;
+ memcpy(&state->len_buf[state->len_filled], ptr, digits_remaining);
+ state->len_filled += digits_remaining;
+ ptr += digits_remaining;
+ size -= digits_remaining;
+
+ if (state->len_filled == 4) {
+ state->remaining = packet_length(state->len_buf);
+ if (state->remaining < 0) {
+ die(_("remote-curl: bad line length character: %.4s"), state->len_buf);
+ } else if (state->remaining < 4) {
+ state->remaining = 0;
+ } else {
+ state->remaining -= 4;
+ }
+ state->len_filled = 0;
+ }
+ }
+
+ if (state->remaining) {
+ int remaining = state->remaining;
+ if (remaining > size)
+ remaining = size;
+ ptr += remaining;
+ size -= remaining;
+ state->remaining -= remaining;
+ }
+ }
+}
+
struct rpc_in_data {
struct rpc_state *rpc;
struct active_request_slot *slot;
+ int check_pktline;
+ struct check_pktline_state pktline_state;
};
/*
@@ -702,6 +746,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,
return size;
if (size)
data->rpc->any_written = 1;
+ if (data->check_pktline)
+ check_pktline(&data->pktline_state, ptr, size);
write_or_die(data->rpc->in, ptr, size);
return size;
}
@@ -778,7 +824,7 @@ static curl_off_t xcurl_off_t(size_t len)
* 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)
+static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_received)
{
struct active_request_slot *slot;
struct curl_slist *headers = http_copy_default_headers();
@@ -920,6 +966,8 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
rpc_in_data.rpc = rpc;
rpc_in_data.slot = slot;
+ rpc_in_data.check_pktline = stateless_connect;
+ memset(&rpc_in_data.pktline_state, 0, sizeof(rpc_in_data.pktline_state));
curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
@@ -936,6 +984,11 @@ static int post_rpc(struct rpc_state *rpc, int flush_received)
if (!rpc->any_written)
err = -1;
+ if (rpc_in_data.pktline_state.len_filled)
+ err = error(_("%d bytes of length header were received"), rpc_in_data.pktline_state.len_filled);
+ if (rpc_in_data.pktline_state.remaining)
+ err = error(_("%d bytes of body are still expected"), rpc_in_data.pktline_state.remaining);
+
curl_slist_free_all(headers);
free(gzip_body);
return err;
@@ -985,7 +1038,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,
break;
rpc->pos = 0;
rpc->len = n;
- err |= post_rpc(rpc, 0);
+ err |= post_rpc(rpc, 0, 0);
}
close(client.in);
@@ -1342,7 +1395,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, status == PACKET_READ_FLUSH))
+ if (post_rpc(&rpc, 1, status == PACKET_READ_FLUSH))
/* We would have an err here */
break;
/* Reset the buffer for next request */
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 1449ee95e9..d2edfa4c50 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -129,6 +129,8 @@ install_script () {
prepare_httpd() {
mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
+ install_script incomplete-length-upload-pack-v2-http.sh
+ install_script incomplete-body-upload-pack-v2-http.sh
install_script broken-smart-http.sh
install_script error-smart-http.sh
install_script error.sh
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 994e5290d6..afa91e38b0 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -117,6 +117,8 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
</LocationMatch>
+ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/
+ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/
ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
@@ -126,6 +128,12 @@ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1
<Directory ${GIT_EXEC_PATH}>
Options FollowSymlinks
</Directory>
+<Files incomplete-length-upload-pack-v2-http.sh>
+ Options ExecCGI
+</Files>
+<Files incomplete-body-upload-pack-v2-http.sh>
+ Options ExecCGI
+</Files>
<Files broken-smart-http.sh>
Options ExecCGI
</Files>
diff --git a/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh
new file mode 100644
index 0000000000..2f5ed9fcf6
--- /dev/null
+++ b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh
@@ -0,0 +1,3 @@
+printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result"
+echo
+printf "%s%s\n" "0079" "45"
diff --git a/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh
new file mode 100644
index 0000000000..86c6e648c9
--- /dev/null
+++ b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh
@@ -0,0 +1,3 @@
+printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result"
+echo
+printf "%s\n" "00"
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 5039e66dc4..4eb81ba2d4 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -586,6 +586,40 @@ test_expect_success 'clone with http:// using protocol v2' '
! grep "Send header: Transfer-Encoding: chunked" log
'
+test_expect_success 'clone repository with http:// using protocol v2 with incomplete pktline length' '
+ test_when_finished "rm -f log" &&
+
+ git init "$HTTPD_DOCUMENT_ROOT_PATH/incomplete_length" &&
+ test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/incomplete_length" file &&
+
+ test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" git -c protocol.version=2 \
+ clone "$HTTPD_URL/smart/incomplete_length" incomplete_length_child 2>err &&
+
+ # Client requested to use protocol v2
+ grep "Git-Protocol: version=2" log &&
+ # Server responded using protocol v2
+ grep "git< version 2" log &&
+ # Client reported appropriate failure
+ test_i18ngrep "bytes of length header were received" err
+'
+
+test_expect_success 'clone repository with http:// using protocol v2 with incomplete pktline body' '
+ test_when_finished "rm -f log" &&
+
+ git init "$HTTPD_DOCUMENT_ROOT_PATH/incomplete_body" &&
+ test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/incomplete_body" file &&
+
+ test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" git -c protocol.version=2 \
+ clone "$HTTPD_URL/smart/incomplete_body" incomplete_body_child 2>err &&
+
+ # Client requested to use protocol v2
+ grep "Git-Protocol: version=2" log &&
+ # Server responded using protocol v2
+ grep "git< version 2" log &&
+ # Client reported appropriate failure
+ test_i18ngrep "bytes of body are still expected" err
+'
+
test_expect_success 'clone big repository with http:// using protocol v2' '
test_when_finished "rm -f log" &&
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH v3 6/7] pkt-line: define PACKET_READ_RESPONSE_END
2020-05-19 10:53 ` [PATCH v3 " Denton Liu
` (4 preceding siblings ...)
2020-05-19 10:53 ` [PATCH v3 5/7] remote-curl: error on incomplete packet Denton Liu
@ 2020-05-19 10:53 ` Denton Liu
2020-05-19 10:54 ` [PATCH v3 7/7] stateless-connect: send response end packet Denton Liu
` (3 subsequent siblings)
9 siblings, 0 replies; 62+ messages in thread
From: Denton Liu @ 2020-05-19 10:53 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine, Junio C Hamano
In a future commit, we will use PACKET_READ_RESPONSE_END to separate
messages proxied by remote-curl. To prepare for this, add the
PACKET_READ_RESPONSE_END enum value.
In switch statements that need a case added, die() or BUG() when a
PACKET_READ_RESPONSE_END is unexpected. Otherwise, mirror how
PACKET_READ_DELIM is implemented (especially in cases where packets are
being forwarded).
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
connect.c | 2 ++
pkt-line.c | 11 +++++++++++
pkt-line.h | 2 ++
remote-curl.c | 2 ++
serve.c | 2 ++
t/helper/test-pkt-line.c | 4 ++++
6 files changed, 23 insertions(+)
diff --git a/connect.c b/connect.c
index 23013c6344..11c6ec70a0 100644
--- a/connect.c
+++ b/connect.c
@@ -127,6 +127,7 @@ enum protocol_version discover_version(struct packet_reader *reader)
die_initial_contact(0);
case PACKET_READ_FLUSH:
case PACKET_READ_DELIM:
+ case PACKET_READ_RESPONSE_END:
version = protocol_v0;
break;
case PACKET_READ_NORMAL:
@@ -310,6 +311,7 @@ struct ref **get_remote_heads(struct packet_reader *reader,
state = EXPECTING_DONE;
break;
case PACKET_READ_DELIM:
+ case PACKET_READ_RESPONSE_END:
die(_("invalid packet"));
}
diff --git a/pkt-line.c b/pkt-line.c
index 3beab1dc6b..8f9bc68ee2 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -99,6 +99,13 @@ void packet_delim(int fd)
die_errno(_("unable to write delim packet"));
}
+void packet_response_end(int fd)
+{
+ packet_trace("0002", 4, 1);
+ if (write_in_full(fd, "0002", 4) < 0)
+ die_errno(_("unable to write stateless separator packet"));
+}
+
int packet_flush_gently(int fd)
{
packet_trace("0000", 4, 1);
@@ -337,6 +344,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
packet_trace("0001", 4, 0);
*pktlen = 0;
return PACKET_READ_DELIM;
+ } else if (len == 2) {
+ packet_trace("0002", 4, 0);
+ *pktlen = 0;
+ return PACKET_READ_RESPONSE_END;
} else if (len < 4) {
die(_("protocol error: bad line length %d"), len);
}
diff --git a/pkt-line.h b/pkt-line.h
index 0d92c5e17f..6cb92d7a5d 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -22,6 +22,7 @@
*/
void packet_flush(int fd);
void packet_delim(int fd);
+void packet_response_end(int fd);
void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
void packet_buf_flush(struct strbuf *buf);
void packet_buf_delim(struct strbuf *buf);
@@ -94,6 +95,7 @@ enum packet_read_status {
PACKET_READ_NORMAL,
PACKET_READ_FLUSH,
PACKET_READ_DELIM,
+ PACKET_READ_RESPONSE_END,
};
enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
size_t *src_len, char *buffer,
diff --git a/remote-curl.c b/remote-curl.c
index e020140092..d02cb547e9 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -601,6 +601,8 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,
case PACKET_READ_FLUSH:
memcpy(buf - 4, "0000", 4);
break;
+ case PACKET_READ_RESPONSE_END:
+ die(_("remote server sent stateless separator"));
}
}
diff --git a/serve.c b/serve.c
index 317256c1a4..c046926ba1 100644
--- a/serve.c
+++ b/serve.c
@@ -217,6 +217,8 @@ static int process_request(void)
state = PROCESS_REQUEST_DONE;
break;
+ case PACKET_READ_RESPONSE_END:
+ BUG("unexpected stateless separator packet");
}
}
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 12ca698e17..69152958e5 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -46,6 +46,9 @@ static void unpack(void)
case PACKET_READ_DELIM:
printf("0001\n");
break;
+ case PACKET_READ_RESPONSE_END:
+ printf("0002\n");
+ break;
}
}
}
@@ -75,6 +78,7 @@ static void unpack_sideband(void)
case PACKET_READ_FLUSH:
return;
case PACKET_READ_DELIM:
+ case PACKET_READ_RESPONSE_END:
break;
}
}
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH v3 7/7] stateless-connect: send response end packet
2020-05-19 10:53 ` [PATCH v3 " Denton Liu
` (5 preceding siblings ...)
2020-05-19 10:53 ` [PATCH v3 6/7] pkt-line: define PACKET_READ_RESPONSE_END Denton Liu
@ 2020-05-19 10:54 ` Denton Liu
2020-05-19 18:40 ` [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
` (2 subsequent siblings)
9 siblings, 0 replies; 62+ messages in thread
From: Denton Liu @ 2020-05-19 10:54 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine, Junio C Hamano, Force Charlie
Currently, remote-curl acts as a proxy and blindly forwards packets
between an HTTP server and fetch-pack. In the case of a stateless RPC
connection where the connection is terminated before the transaction is
complete, remote-curl will blindly forward the packets before waiting on
more input from fetch-pack. Meanwhile, fetch-pack will read the
transaction and continue reading, expecting more input to continue the
transaction. This results in a deadlock between the two processes.
This can be seen in the following command which does not terminate:
$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012
Cloning into 'git'...
whereas the v1 version does terminate as expected:
$ git -c protocol.version=1 clone https://github.com/git/git.git --shallow-since=20151012
Cloning into 'git'...
fatal: the remote end hung up unexpectedly
Instead of blindly forwarding packets, make remote-curl insert a
response end packet after proxying the responses from the remote server
when using stateless_connect(). On the RPC client side, ensure that each
response ends as described.
A separate control packet is chosen because we need to be able to
differentiate between what the remote server sends and remote-curl's
control packets. By ensuring in the remote-curl code that a server
cannot send response end packets, we prevent a malicious server from
being able to perform a denial of service attack in which they spoof a
response end packet and cause the described deadlock to happen.
Reported-by: Force Charlie <charlieio@outlook.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/gitremote-helpers.txt | 4 +++-
Documentation/technical/protocol-v2.txt | 2 ++
builtin/fetch-pack.c | 2 +-
connect.c | 16 +++++++++++++++-
connect.h | 4 ++++
fetch-pack.c | 13 +++++++++++++
remote-curl.c | 5 +++++
remote.h | 3 ++-
t/t5702-protocol-v2.sh | 13 +++++++++++++
transport.c | 3 ++-
10 files changed, 60 insertions(+), 5 deletions(-)
diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index f48a031dc3..93baeeb029 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -405,7 +405,9 @@ Supported if the helper has the "connect" capability.
trying to fall back). After line feed terminating the positive
(empty) response, the output of the service starts. Messages
(both request and response) must consist of zero or more
- PKT-LINEs, terminating in a flush packet. The client must not
+ PKT-LINEs, terminating in a flush packet. Response messages will
+ then have a response end packet after the flush packet to
+ indicate the end of a response. The client must not
expect the server to store any state in between request-response
pairs. After the connection ends, the remote helper exits.
+
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 7e3766cafb..3996d70891 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -33,6 +33,8 @@ In protocol v2 these special packets will have the following semantics:
* '0000' Flush Packet (flush-pkt) - indicates the end of a message
* '0001' Delimiter Packet (delim-pkt) - separates sections of a message
+ * '0002' Message Packet (response-end-pkt) - indicates the end of a response
+ for stateless connections
Initial Client Request
----------------------
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 4771100072..94b0c89b82 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -224,7 +224,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
version = discover_version(&reader);
switch (version) {
case protocol_v2:
- get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
+ get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL, args.stateless_rpc);
break;
case protocol_v1:
case protocol_v0:
diff --git a/connect.c b/connect.c
index 11c6ec70a0..0df45a1108 100644
--- a/connect.c
+++ b/connect.c
@@ -406,10 +406,21 @@ static int process_ref_v2(const char *line, struct ref ***list)
return ret;
}
+void check_stateless_delimiter(int stateless_rpc,
+ struct packet_reader *reader,
+ const char *error)
+{
+ if (!stateless_rpc)
+ return; /* not in stateless mode, no delimiter expected */
+ if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)
+ die("%s", error);
+}
+
struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
struct ref **list, int for_push,
const struct argv_array *ref_prefixes,
- const struct string_list *server_options)
+ const struct string_list *server_options,
+ int stateless_rpc)
{
int i;
*list = NULL;
@@ -446,6 +457,9 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
if (reader->status != PACKET_READ_FLUSH)
die(_("expected flush after ref listing"));
+ check_stateless_delimiter(stateless_rpc, reader,
+ _("expected response end packet after ref listing"));
+
return list;
}
diff --git a/connect.h b/connect.h
index 5f2382e018..235bc66254 100644
--- a/connect.h
+++ b/connect.h
@@ -22,4 +22,8 @@ int server_supports_v2(const char *c, int die_on_error);
int server_supports_feature(const char *c, const char *feature,
int die_on_error);
+void check_stateless_delimiter(int stateless_rpc,
+ struct packet_reader *reader,
+ const char *error);
+
#endif
diff --git a/fetch-pack.c b/fetch-pack.c
index f73a2ce6cb..f096442d4d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1450,6 +1450,13 @@ enum fetch_state {
FETCH_DONE,
};
+static void do_check_stateless_delimiter(const struct fetch_pack_args *args,
+ struct packet_reader *reader)
+{
+ check_stateless_delimiter(args->stateless_rpc, reader,
+ _("git fetch-pack: expected response end packet"));
+}
+
static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
int fd[2],
const struct ref *orig_ref,
@@ -1534,6 +1541,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
/* Process ACKs/NAKs */
switch (process_acks(negotiator, &reader, &common)) {
case READY:
+ /*
+ * Don't check for response delimiter; get_pack() will
+ * read the rest of this response.
+ */
state = FETCH_GET_PACK;
break;
case COMMON_FOUND:
@@ -1541,6 +1552,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
seen_ack = 1;
/* fallthrough */
case NO_COMMON_FOUND:
+ do_check_stateless_delimiter(args, &reader);
state = FETCH_SEND_REQUEST;
break;
}
@@ -1560,6 +1572,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
process_section_header(&reader, "packfile", 0);
if (get_pack(args, fd, pack_lockfile, sought, nr_sought))
die(_("git fetch-pack: fetch failed."));
+ do_check_stateless_delimiter(args, &reader);
state = FETCH_DONE;
break;
diff --git a/remote-curl.c b/remote-curl.c
index d02cb547e9..75532a8bae 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -703,6 +703,8 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si
state->remaining = packet_length(state->len_buf);
if (state->remaining < 0) {
die(_("remote-curl: bad line length character: %.4s"), state->len_buf);
+ } else if (state->remaining == 2) {
+ die(_("remote-curl: unexpected response end packet"));
} else if (state->remaining < 4) {
state->remaining = 0;
} else {
@@ -991,6 +993,9 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
if (rpc_in_data.pktline_state.remaining)
err = error(_("%d bytes of body are still expected"), rpc_in_data.pktline_state.remaining);
+ if (stateless_connect)
+ packet_response_end(rpc->in);
+
curl_slist_free_all(headers);
free(gzip_body);
return err;
diff --git a/remote.h b/remote.h
index 11d8719b58..5cc26c1b3b 100644
--- a/remote.h
+++ b/remote.h
@@ -179,7 +179,8 @@ struct ref **get_remote_heads(struct packet_reader *reader,
struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
struct ref **list, int for_push,
const struct argv_array *ref_prefixes,
- const struct string_list *server_options);
+ const struct string_list *server_options,
+ int stateless_rpc);
int resolve_remote_symref(struct ref *ref, struct ref *list);
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 4eb81ba2d4..8da65e60de 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -620,6 +620,19 @@ test_expect_success 'clone repository with http:// using protocol v2 with incomp
test_i18ngrep "bytes of body are still expected" err
'
+test_expect_success 'clone with http:// using protocol v2 and invalid parameters' '
+ test_when_finished "rm -f log" &&
+
+ test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" \
+ git -c protocol.version=2 \
+ clone --shallow-since=20151012 "$HTTPD_URL/smart/http_parent" http_child_invalid &&
+
+ # Client requested to use protocol v2
+ grep "Git-Protocol: version=2" log &&
+ # Server responded using protocol v2
+ grep "git< version 2" log
+'
+
test_expect_success 'clone big repository with http:// using protocol v2' '
test_when_finished "rm -f log" &&
diff --git a/transport.c b/transport.c
index 431a93caef..7d50c502ad 100644
--- a/transport.c
+++ b/transport.c
@@ -297,7 +297,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
if (must_list_refs)
get_remote_refs(data->fd[1], &reader, &refs, for_push,
ref_prefixes,
- transport->server_options);
+ transport->server_options,
+ transport->stateless_rpc);
break;
case protocol_v1:
case protocol_v0:
--
2.26.2.706.g87896c9627
^ permalink raw reply related [flat|nested] 62+ messages in thread
* Re: [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects
2020-05-19 10:53 ` [PATCH v3 " Denton Liu
` (6 preceding siblings ...)
2020-05-19 10:54 ` [PATCH v3 7/7] stateless-connect: send response end packet Denton Liu
@ 2020-05-19 18:40 ` Jeff King
2020-05-19 21:14 ` Denton Liu
2020-05-19 20:51 ` [PATCH v3 8/7] fixup! pkt-line: extern packet_length() Denton Liu
2020-05-22 13:33 ` [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet Denton Liu
9 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-19 18:40 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Eric Sunshine, Junio C Hamano
On Tue, May 19, 2020 at 06:53:53AM -0400, Denton Liu wrote:
> Changes since v2:
>
> * Use if-else tower in "transport: extract common fetch_pack() call"
>
> * Rename to `lenbuf_hex` and remove null-terminator sentence in
> "pkt-line: extern packet_length()"
>
> * Fix "a a" typo in "remote-curl: error on incomplete packet"
>
> * Don't send flush packet after response end packet
>
> * Move stateless delimiter checks closer to where message processing
> happens in do_fetch_pack_v2()
This looks good to me, modulo the minor comment fixup from Eric. I did
have one final question, though. Our discussion focused a lot on
checking the 0002 packets in the success case. But we didn't discuss how
fetch-pack would deal with an unexpected 0002 packet (i.e., the case
that the server response is truncated, but then remote-curl tacks on its
0002).
It _seems_ to work, because that's the case your invalid-shallow test is
covering. I'm just not sure if it works consistently, or what error we
might produce in some cases (e.g., saying "woah, what's the weird 0002
packet" instead of "the server response ended unexpectedly" or
something).
I suspect any remaining issues there are cosmetic, and it might be OK to
just discover them organically through use. But if you happen to have
done some poking around there, it would be nice to hear it.
Thanks for working on this. When we had the initial discussion, I was
really worried the solution was going to be quite nasty, but I think it
turned out to be reasonably nice.
-Peff
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects
2020-05-19 18:40 ` [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
@ 2020-05-19 21:14 ` Denton Liu
0 siblings, 0 replies; 62+ messages in thread
From: Denton Liu @ 2020-05-19 21:14 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Eric Sunshine, Junio C Hamano
Hi Peff,
On Tue, May 19, 2020 at 02:40:04PM -0400, Jeff King wrote:
> It _seems_ to work, because that's the case your invalid-shallow test is
> covering. I'm just not sure if it works consistently, or what error we
> might produce in some cases (e.g., saying "woah, what's the weird 0002
> packet" instead of "the server response ended unexpectedly" or
> something).
>
> I suspect any remaining issues there are cosmetic, and it might be OK to
> just discover them organically through use. But if you happen to have
> done some poking around there, it would be nice to hear it.
From what I can tell, every time a packet is read using the reader in
do_fetch_pack_v2(), we're careful about checking the packet type so we
should be safe there. Also, when piping stuff over to index-pack and
unpack-objects, it looks like the resulting call to recv_sideband()
treats any control packets as flush packets so it should handle the 0002
fine.
I could have missed checking some spots, though. But as far as I can
tell, if it can't handle the 0002 properly, it was already buggy to
begin with. I agree that we can let any remaining issues be shaken out
through use.
> Thanks for working on this. When we had the initial discussion, I was
> really worried the solution was going to be quite nasty, but I think it
> turned out to be reasonably nice.
Thanks,
Denton
> -Peff
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v3 8/7] fixup! pkt-line: extern packet_length()
2020-05-19 10:53 ` [PATCH v3 " Denton Liu
` (7 preceding siblings ...)
2020-05-19 18:40 ` [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
@ 2020-05-19 20:51 ` Denton Liu
2020-05-22 13:33 ` [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet Denton Liu
9 siblings, 0 replies; 62+ messages in thread
From: Denton Liu @ 2020-05-19 20:51 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine, Junio C Hamano
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
pkt-line.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pkt-line.h b/pkt-line.h
index 6cb92d7a5d..5b373fe4cd 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -79,7 +79,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char
* Convert a four hex digit packet line length header into its numeric
* representation.
*
- * If linelen contains non-hex characters, return -1. Otherwise, return the
+ * If lenbuf_hex contains non-hex characters, return -1. Otherwise, return the
* numeric value of the length header.
*/
int packet_length(const char lenbuf_hex[4]);
--
2.27.0.rc0.114.g141fe7d276
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet
2020-05-19 10:53 ` [PATCH v3 " Denton Liu
` (8 preceding siblings ...)
2020-05-19 20:51 ` [PATCH v3 8/7] fixup! pkt-line: extern packet_length() Denton Liu
@ 2020-05-22 13:33 ` Denton Liu
2020-05-22 15:54 ` Jeff King
9 siblings, 1 reply; 62+ messages in thread
From: Denton Liu @ 2020-05-22 13:33 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Eric Sunshine, Junio C Hamano
In the CGI scripts which emulate a connection being prematurely
terminated, it doesn't make sense for there to be a trailing newline
after the simulated connection cut. Remove these newlines.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
t/lib-httpd/incomplete-body-upload-pack-v2-http.sh | 2 +-
t/lib-httpd/incomplete-length-upload-pack-v2-http.sh | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh
index 2f5ed9fcf6..90e73ef8d5 100644
--- a/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh
+++ b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh
@@ -1,3 +1,3 @@
printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result"
echo
-printf "%s%s\n" "0079" "45"
+printf "%s%s" "0079" "45"
diff --git a/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh
index 86c6e648c9..dce552e348 100644
--- a/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh
+++ b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh
@@ -1,3 +1,3 @@
printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result"
echo
-printf "%s\n" "00"
+printf "%s" "00"
--
2.27.0.rc1.69.g3229fa6e15
^ permalink raw reply related [flat|nested] 62+ messages in thread
* Re: [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet
2020-05-22 13:33 ` [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet Denton Liu
@ 2020-05-22 15:54 ` Jeff King
2020-05-22 16:05 ` Denton Liu
0 siblings, 1 reply; 62+ messages in thread
From: Jeff King @ 2020-05-22 15:54 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Eric Sunshine, Junio C Hamano
On Fri, May 22, 2020 at 09:33:47AM -0400, Denton Liu wrote:
> In the CGI scripts which emulate a connection being prematurely
> terminated, it doesn't make sense for there to be a trailing newline
> after the simulated connection cut. Remove these newlines.
Ah, good catch. I think in the first one it doesn't matter:
> -printf "%s%s\n" "0079" "45"
> +printf "%s%s" "0079" "45"
because we have a too-short packet, so the fact that it is 3 bytes and
not 2 does not change that.
I agree it's clearer without the newline, though. I wonder if:
printf "0079" "fewer than 0x79 bytes"
would make it even more self-documenting. :)
> -printf "%s\n" "00"
> +printf "%s" "00"
This one is a behavior improvement: we were probably hitting "oops,
newline isn't a valid line-length character" before, and now we're
really hitting the truncated packet.
I don't know if it's worth adding an extra test with a bogus
line-length. I'm OK with with it either way.
-Peff
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet
2020-05-22 15:54 ` Jeff King
@ 2020-05-22 16:05 ` Denton Liu
2020-05-22 16:31 ` Jeff King
0 siblings, 1 reply; 62+ messages in thread
From: Denton Liu @ 2020-05-22 16:05 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Eric Sunshine, Junio C Hamano
Hi Peff,
On Fri, May 22, 2020 at 11:54:10AM -0400, Jeff King wrote:
> > -printf "%s\n" "00"
> > +printf "%s" "00"
>
> This one is a behavior improvement: we were probably hitting "oops,
> newline isn't a valid line-length character" before, and now we're
> really hitting the truncated packet.
The test currently actually greps for the "incomplete length" error
message and it passes so the behaviour remains the same. We just got
lucky that we send "00" instead of "000" beacuse "000\n" would've
otherwise given us a full length header.
> I don't know if it's worth adding an extra test with a bogus
> line-length. I'm OK with with it either way.
I think I'll leave this unless anyone really wants this to be tested.
Thanks,
Denton
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet
2020-05-22 16:05 ` Denton Liu
@ 2020-05-22 16:31 ` Jeff King
0 siblings, 0 replies; 62+ messages in thread
From: Jeff King @ 2020-05-22 16:31 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Eric Sunshine, Junio C Hamano
On Fri, May 22, 2020 at 12:05:52PM -0400, Denton Liu wrote:
> Hi Peff,
>
> On Fri, May 22, 2020 at 11:54:10AM -0400, Jeff King wrote:
> > > -printf "%s\n" "00"
> > > +printf "%s" "00"
> >
> > This one is a behavior improvement: we were probably hitting "oops,
> > newline isn't a valid line-length character" before, and now we're
> > really hitting the truncated packet.
>
> The test currently actually greps for the "incomplete length" error
> message and it passes so the behaviour remains the same. We just got
> lucky that we send "00" instead of "000" beacuse "000\n" would've
> otherwise given us a full length header.
Ah, OK. I was surprised it didn't hit your new code in check_pktline()
that complains about non-hex chars. But the reason is that we don't
check digit-by-digit as we read them. We wait until we have 4 chars, and
then we feed the whole thing to packet_length(). But since we only
receive 3, the state machine doesn't move to that step. So that makes
sense.
> > I don't know if it's worth adding an extra test with a bogus
> > line-length. I'm OK with with it either way.
>
> I think I'll leave this unless anyone really wants this to be tested.
Sounds good.
-Peff
^ permalink raw reply [flat|nested] 62+ messages in thread