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

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.

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 serve.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

On Tue, Sep 14, 2021 at 11:33:18AM -0400, Jeff King wrote:
> 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.

> 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).

>  			/* 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

On Tue, Sep 14, 2021 at 12:59:18PM -0400, Taylor Blau wrote:

> On Tue, Sep 14, 2021 at 11:33:18AM -0400, Jeff King wrote:
> > 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.

Right. By the way, one thing I noticed while working on this (that is
unchanged by my series) is that some capabilities are ignored. That make
sense for "agent", where the primary goal is telling the client about
our version (and letting them speak their version, which might be
helpful for debug logs but doesn't impact the protocol itself). But we
do nothing at all with "server-option".

The client wouldn't send one by default, but we do support "git fetch
--server-option". So I assume that it would be a sort of "I'll tell you
this string, and you might feed it to hooks, etc" function, like
push-options. But we just throw it away (well, before my series we
stuffed it into a strvec that nobody looked at).

So possibly we could stop advertising it, but I wonder if any clients
would get unhappy.

I also wonder if jgit uses it. Grepping in jgit.git shows that it is
recorded but I don't see anybody doing anything useful with it; but
maybe some user of the library does.

> > @@ -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).

Thanks. While this section ended up quite simple, it got rebased a whole
lot of times as I figured out all of quirks that led to the final
protocol-tightening patches. :)

I don't know how I missed the extra line on my final read-through.

-Peff