Re: [PATCH 6/9] serve: drop "keys" strvec

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 14, 2021, 16:59 UTC
Message-ID: <YUDU5gE/AazOus+s@nand.local>
In-Reply-To: <YUDAvjteb/JAtyNz@coredump.intra.peff.net>

On Tue, Sep 14, 2021 at 11:33:18AM -0400, Jeff King wrote:

Show 8 quoted lines

> We collect the set of capabilities the client sends us in a strvec.
> While this is usually small, there's no limit to the number of
> capabilities the client can send us (e.g., they could just send us
> "agent" pkt-lines over and over, and we'd keep adding them to the list).
>
> Since all code has been converted away from using this list, let's get
> rid of it. This avoids a potential attack where clients waste our
> memory.

...because now we only bother to tell capabilities about information the client sent as it happened, instead of accumulating an unbounded set of strings together into a single strvec.

> Note that we do have to replace it with a flag, because some of the
> flush-packet logic checks whether we've seen any valid commands or keys.

Makes sense.

Show 28 quoted lines

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  serve.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/serve.c b/serve.c
> index 6bbf54cbbe..baa0a17502 100644
> --- a/serve.c
> +++ b/serve.c
> @@ -239,7 +239,7 @@ static int process_request(void)
>  {
>  	enum request_state state = PROCESS_REQUEST_KEYS;
>  	struct packet_reader reader;
> -	struct strvec keys = STRVEC_INIT;
> +	int seen_capability_or_command = 0;
>  	struct protocol_capability *command = NULL;
>
>  	packet_reader_init(&reader, 0, NULL, 0,
> @@ -263,10 +263,11 @@ static int process_request(void)
>  			/* collect request; a sequence of keys and values */
>  			if (parse_command(reader.line, &command) ||
>  			    receive_client_capability(reader.line))
> -				strvec_push(&keys, reader.line);
> +				seen_capability_or_command = 1;
>  			else
>  				die("unknown capability '%s'", reader.line);
>
> +

Nit; unnecessary whitespace change (but obviously not worth a re-roll on its own).

Show 22 quoted lines

>  			/* Consume the peeked line */
>  			packet_reader_read(&reader);
>  			break;
> @@ -275,7 +276,7 @@ static int process_request(void)
>  			 * If no command and no keys were given then the client
>  			 * wanted to terminate the connection.
>  			 */
> -			if (!keys.nr)
> +			if (!seen_capability_or_command)
>  				return 1;
>
>  			/*
> @@ -309,7 +310,6 @@ static int process_request(void)
>
>  	command->command(the_repository, &reader);
>
> -	strvec_clear(&keys);
>  	return 0;
>  }
>
> --
> 2.33.0.887.g5b1f44e68d

The rest of this change looks obviously good to me.

Thanks, Taylor