Re: [PATCH v2 2/2] pack-protocol.txt: accept error packets in any context

[Josh Steadmon <steadmon@google.com>]

On 2018.12.29 13:19, Masaya Suzuki wrote:
> In the Git pack protocol definition, an error packet may appear only in
> a certain context. However, servers can face a runtime error (e.g. I/O
> error) at an arbitrary timing. This patch changes the protocol to allow
> an error packet to be sent instead of any packet.
> 
> Without this protocol spec change, when a server cannot process a
> request, there's no way to tell that to a client. Since the server
> cannot produce a valid response, it would be forced to cut a connection
> without telling why. With this protocol spec change, the server can be
> more gentle in this situation. An old client may see these error packets
> as an unexpected packet, but this is not worse than having an unexpected
> EOF.
> 
> Following this protocol spec change, the error packet handling code is
> moved to pkt-line.c. Implementation wise, this implementation uses
> pkt-line to communicate with a subprocess. Since this is not a part of
> Git protocol, it's possible that a packet that is not supposed to be an
> error packet is mistakenly parsed as an error packet. This error packet
> handling is enabled only for the Git pack protocol parsing code
> considering this.
> 
> Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
> ---
>  Documentation/technical/pack-protocol.txt | 20 +++++++++++---------
>  builtin/archive.c                         |  6 +++---
>  builtin/fetch-pack.c                      |  3 ++-
>  builtin/receive-pack.c                    |  4 +++-
>  builtin/send-pack.c                       |  3 ++-
>  connect.c                                 |  3 ---
>  fetch-pack.c                              |  8 ++++----
>  pkt-line.c                                |  4 ++++
>  pkt-line.h                                |  8 ++++++--
>  remote-curl.c                             |  9 ++++++---
>  send-pack.c                               |  4 +++-
>  serve.c                                   |  5 +++--
>  t/t5703-upload-pack-ref-in-want.sh        |  4 ++--
>  transport.c                               |  3 ++-
>  upload-pack.c                             |  4 +++-
>  15 files changed, 54 insertions(+), 34 deletions(-)
> 
> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
> index 6ac774d5f..7a2375a55 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate `PKT-LINE(...)`, unless
>  otherwise noted the usual pkt-line LF rules apply: the sender SHOULD
>  include a LF, but the receiver MUST NOT complain if it is not present.
>  
> +An error packet is a special pkt-line that contains an error string.
> +
> +----
> +  error-line     =  PKT-LINE("ERR" SP explanation-text)
> +----
> +
> +Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY
> +be sent. Once this packet is sent by a client or a server, the data transfer
> +process defined in this protocol is terminated.
> +
>  Transports
>  ----------
>  There are three transports over which the packfile protocol is
> @@ -89,13 +99,6 @@ process on the server side over the Git protocol is this:
>       "0039git-upload-pack /schacon/gitbook.git\0host=example.com\0" |
>       nc -v example.com 9418
>  
> -If the server refuses the request for some reasons, it could abort
> -gracefully with an error message.
> -
> -----
> -  error-line     =  PKT-LINE("ERR" SP explanation-text)
> -----
> -
>  
>  SSH Transport
>  -------------
> @@ -398,12 +401,11 @@ from the client).
>  Then the server will start sending its packfile data.
>  
>  ----
> -  server-response = *ack_multi ack / nak / error-line
> +  server-response = *ack_multi ack / nak
>    ack_multi       = PKT-LINE("ACK" SP obj-id ack_status)
>    ack_status      = "continue" / "common" / "ready"
>    ack             = PKT-LINE("ACK" SP obj-id)
>    nak             = PKT-LINE("NAK")
> -  error-line     =  PKT-LINE("ERR" SP explanation-text)
>  ----
>  
>  A simple clone may look like this (with no 'have' lines):
> diff --git a/builtin/archive.c b/builtin/archive.c
> index 2fe1f05ca..45d11669a 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -53,15 +53,15 @@ static int run_remote_archiver(int argc, const char **argv,
>  		packet_write_fmt(fd[1], "argument %s\n", argv[i]);
>  	packet_flush(fd[1]);
>  
> -	packet_reader_init(&reader, fd[0], NULL, 0, PACKET_READ_CHOMP_NEWLINE);
> +	packet_reader_init(&reader, fd[0], NULL, 0,
> +			   PACKET_READ_CHOMP_NEWLINE |
> +			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
>  		die(_("git archive: expected ACK/NAK, got a flush packet"));
>  	if (strcmp(reader.line, "ACK")) {
>  		if (starts_with(reader.line, "NACK "))
>  			die(_("git archive: NACK %s"), reader.line + 5);
> -		if (starts_with(reader.line, "ERR "))
> -			die(_("remote error: %s"), reader.line + 4);
>  		die(_("git archive: protocol error"));
>  	}
>  
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 63e69a580..85dbc2af8 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -217,7 +217,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>  
>  	packet_reader_init(&reader, fd[0], NULL, 0,
>  			   PACKET_READ_CHOMP_NEWLINE |
> -			   PACKET_READ_GENTLE_ON_EOF);
> +			   PACKET_READ_GENTLE_ON_EOF |
> +			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	switch (discover_version(&reader)) {
>  	case protocol_v2:
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 81cc07370..d58b7750b 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1986,7 +1986,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  	if (advertise_refs)
>  		return 0;
>  
> -	packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);
> +	packet_reader_init(&reader, 0, NULL, 0,
> +			   PACKET_READ_CHOMP_NEWLINE |
> +			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	if ((commands = read_head_info(&reader, &shallow)) != NULL) {
>  		const char *unpack_status = NULL;
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 8e3c7490f..098ebf22d 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -250,7 +250,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>  
>  	packet_reader_init(&reader, fd[0], NULL, 0,
>  			   PACKET_READ_CHOMP_NEWLINE |
> -			   PACKET_READ_GENTLE_ON_EOF);
> +			   PACKET_READ_GENTLE_ON_EOF |
> +			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	switch (discover_version(&reader)) {
>  	case protocol_v2:
> diff --git a/connect.c b/connect.c
> index 24281b608..4813f005a 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -296,7 +296,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,
>  	struct ref **orig_list = list;
>  	int len = 0;
>  	enum get_remote_heads_state state = EXPECTING_FIRST_REF;
> -	const char *arg;
>  
>  	*list = NULL;
>  
> @@ -306,8 +305,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,
>  			die_initial_contact(1);
>  		case PACKET_READ_NORMAL:
>  			len = reader->pktlen;
> -			if (len > 4 && skip_prefix(reader->line, "ERR ", &arg))
> -				die(_("remote error: %s"), arg);
>  			break;
>  		case PACKET_READ_FLUSH:
>  			state = EXPECTING_DONE;
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 86790b9bb..3f9626dbf 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -182,8 +182,6 @@ static enum ack_type get_ack(struct packet_reader *reader,
>  			return ACK;
>  		}
>  	}
> -	if (skip_prefix(reader->line, "ERR ", &arg))
> -		die(_("remote error: %s"), arg);
>  	die(_("git fetch-pack: expected ACK/NAK, got '%s'"), reader->line);
>  }
>  
> @@ -258,7 +256,8 @@ static int find_common(struct fetch_negotiator *negotiator,
>  		die(_("--stateless-rpc requires multi_ack_detailed"));
>  
>  	packet_reader_init(&reader, fd[0], NULL, 0,
> -			   PACKET_READ_CHOMP_NEWLINE);
> +			   PACKET_READ_CHOMP_NEWLINE |
> +			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	if (!args->no_dependents) {
>  		mark_tips(negotiator, args->negotiation_tips);
> @@ -1358,7 +1357,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  	struct fetch_negotiator negotiator;
>  	fetch_negotiator_init(&negotiator, negotiation_algorithm);
>  	packet_reader_init(&reader, fd[0], NULL, 0,
> -			   PACKET_READ_CHOMP_NEWLINE);
> +			   PACKET_READ_CHOMP_NEWLINE |
> +			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	while (state != FETCH_DONE) {
>  		switch (state) {
> diff --git a/pkt-line.c b/pkt-line.c
> index 04d10bbd0..e70ea6d88 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
>  		return PACKET_READ_EOF;
>  	}
>  
> +	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
> +	    starts_with(buffer, "ERR "))
> +		die(_("remote error: %s"), buffer + 4);
> +
>  	if ((options & PACKET_READ_CHOMP_NEWLINE) &&
>  	    len && buffer[len-1] == '\n')
>  		len--;
> diff --git a/pkt-line.h b/pkt-line.h
> index 5b28d4347..d7e1dbc04 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -62,9 +62,13 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
>   *
>   * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if
>   * present) is removed from the buffer before returning.
> + *
> + * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an
> + * ERR packet.
>   */
> -#define PACKET_READ_GENTLE_ON_EOF (1u<<0)
> -#define PACKET_READ_CHOMP_NEWLINE (1u<<1)
> +#define PACKET_READ_GENTLE_ON_EOF     (1u<<0)
> +#define PACKET_READ_CHOMP_NEWLINE     (1u<<1)
> +#define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)
>  int packet_read(int fd, char **src_buffer, size_t *src_len, char
>  		*buffer, unsigned size, int options);
>  
> diff --git a/remote-curl.c b/remote-curl.c
> index bbd9ba0f3..10b50869c 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -204,7 +204,8 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
>  
>  	packet_reader_init(&reader, -1, heads->buf, heads->len,
>  			   PACKET_READ_CHOMP_NEWLINE |
> -			   PACKET_READ_GENTLE_ON_EOF);
> +			   PACKET_READ_GENTLE_ON_EOF |
> +			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	heads->version = discover_version(&reader);
>  	switch (heads->version) {
> @@ -411,7 +412,8 @@ static struct discovery *discover_refs(const char *service, int for_push)
>  	    !strbuf_cmp(&exp, &type)) {
>  		struct packet_reader reader;
>  		packet_reader_init(&reader, -1, last->buf, last->len,
> -				   PACKET_READ_CHOMP_NEWLINE);
> +				   PACKET_READ_CHOMP_NEWLINE |
> +				   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  		/*
>  		 * smart HTTP response; validate that the service
> @@ -1182,7 +1184,8 @@ static void proxy_state_init(struct proxy_state *p, const char *service_name,
>  		p->headers = curl_slist_append(p->headers, buf.buf);
>  
>  	packet_reader_init(&p->reader, p->in, NULL, 0,
> -			   PACKET_READ_GENTLE_ON_EOF);
> +			   PACKET_READ_GENTLE_ON_EOF |
> +			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	strbuf_release(&buf);
>  }
> diff --git a/send-pack.c b/send-pack.c
> index 913645046..7b9829f16 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -558,7 +558,9 @@ int send_pack(struct send_pack_args *args,
>  		in = demux.out;
>  	}
>  
> -	packet_reader_init(&reader, in, NULL, 0, PACKET_READ_CHOMP_NEWLINE);
> +	packet_reader_init(&reader, in, NULL, 0,
> +			   PACKET_READ_CHOMP_NEWLINE |
> +			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	if (need_pack_data && cmds_sent) {
>  		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
> diff --git a/serve.c b/serve.c
> index bda085f09..317256c1a 100644
> --- a/serve.c
> +++ b/serve.c
> @@ -167,7 +167,8 @@ static int process_request(void)
>  
>  	packet_reader_init(&reader, 0, NULL, 0,
>  			   PACKET_READ_CHOMP_NEWLINE |
> -			   PACKET_READ_GENTLE_ON_EOF);
> +			   PACKET_READ_GENTLE_ON_EOF |
> +			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	/*
>  	 * Check to see if the client closed their end before sending another
> @@ -175,7 +176,7 @@ static int process_request(void)
>  	 */
>  	if (packet_reader_peek(&reader) == PACKET_READ_EOF)
>  		return 1;
> -	reader.options = PACKET_READ_CHOMP_NEWLINE;
> +	reader.options &= ~PACKET_READ_GENTLE_ON_EOF;
>  
>  	while (state != PROCESS_REQUEST_DONE) {
>  		switch (packet_reader_peek(&reader)) {
> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> index 3f58f05cb..d2a9d0c12 100755
> --- a/t/t5703-upload-pack-ref-in-want.sh
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -208,7 +208,7 @@ test_expect_success 'server is initially ahead - no ref in want' '
>  	cp -r "$LOCAL_PRISTINE" local &&
>  	inconsistency master 1234567890123456789012345678901234567890 &&
>  	test_must_fail git -C local fetch 2>err &&
> -	grep "ERR upload-pack: not our ref" err
> +	grep "fatal: remote error: upload-pack: not our ref" err
>  '
>  
>  test_expect_success 'server is initially ahead - ref in want' '
> @@ -254,7 +254,7 @@ test_expect_success 'server loses a ref - ref in want' '
>  	echo "s/master/raster/" >"$HTTPD_ROOT_PATH/one-time-sed" &&
>  	test_must_fail git -C local fetch 2>err &&
>  
> -	grep "ERR unknown ref refs/heads/raster" err
> +	grep "fatal: remote error: unknown ref refs/heads/raster" err
>  '
>  
>  stop_httpd
> diff --git a/transport.c b/transport.c
> index 5a74b609f..12db4251c 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -273,7 +273,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
>  
>  	packet_reader_init(&reader, data->fd[0], NULL, 0,
>  			   PACKET_READ_CHOMP_NEWLINE |
> -			   PACKET_READ_GENTLE_ON_EOF);
> +			   PACKET_READ_GENTLE_ON_EOF |
> +			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	data->version = discover_version(&reader);
>  	switch (data->version) {
> diff --git a/upload-pack.c b/upload-pack.c
> index 1638825ee..08b547cf0 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1078,7 +1078,9 @@ void upload_pack(struct upload_pack_options *options)
>  	if (options->advertise_refs)
>  		return;
>  
> -	packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE);
> +	packet_reader_init(&reader, 0, NULL, 0,
> +			   PACKET_READ_CHOMP_NEWLINE |
> +			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	receive_needs(&reader, &want_obj);
>  	if (want_obj.nr) {
> -- 
> 2.20.1.415.g653613c723-goog
> 

This all looks good to me.

Reviewed-by: Josh Steadmon <steadmon@google.com>