diff mbox series

[v2,5/7] remote-curl: error on incomplete packet

Message ID 52ce5fdffd6741eeee8d69b804403383da0d879d.1589816719.git.liu.denton@gmail.com (mailing list archive)
State New, archived
Headers show
Series remote-curl: fix deadlocks when remote server disconnects | expand

Commit Message

Denton Liu May 18, 2020, 3:47 p.m. UTC 
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

Comments

Jeff King May 18, 2020, 4:22 p.m. UTC | #1 
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
Denton Liu May 18, 2020, 4:51 p.m. UTC | #2 
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.
diff mbox series

Patch

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" &&