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

From: Jeff King <peff@peff.net>
Date: Sep 14, 2021, 17:16 UTC
Message-ID: <YUDY1UIFBxOjNA37@coredump.intra.peff.net>
In-Reply-To: <YUDU5gE/AazOus+s@nand.local>

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

Show 13 quoted lines

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

Show 13 quoted lines

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