Re: [PATCH v2 0/11] limit memory allocations for v2 servers

OK, hopefully third time's the charm. The changes from v2 are pretty minor:

 - test typos noticed by Eric (none affected the outcome of the tests)

 - I added "object-format=$(test_oid algo)" to the input of a few of the
   new tests even where it doesn't change the outcome, for consistency
   with the existing tests

 - dropped out-dated "collect" comment noticed by Junio

 - explained the command=ls-refs=foo case a little further, including
   specific references in how it violates the spec

Range-diff is below.

  [01/11]: serve: rename is_command() to parse_command()
  [02/11]: serve: return capability "value" from get_capability()
  [03/11]: serve: add "receive" method for v2 capabilities table
  [04/11]: serve: provide "receive" function for object-format capability
  [05/11]: serve: provide "receive" function for session-id capability
  [06/11]: serve: drop "keys" strvec
  [07/11]: ls-refs: ignore very long ref-prefix counts
  [08/11]: docs/protocol-v2: clarify some ls-refs ref-prefix details
  [09/11]: serve: reject bogus v2 "command=ls-refs=foo"
  [10/11]: serve: reject commands used as capabilities
  [11/11]: ls-refs: reject unknown arguments

 Documentation/technical/protocol-v2.txt |   6 +-
 ls-refs.c                               |  22 ++++-
 serve.c                                 | 120 +++++++++++++-----------
 t/t5701-git-serve.sh                    |  75 +++++++++++++++
 4 files changed, 164 insertions(+), 59 deletions(-)

 1:  e642ced1e8 =  1:  71003eb21a serve: rename is_command() to parse_command()
 2:  75d119ae49 =  2:  1e86c31477 serve: return capability "value" from get_capability()
 3:  9fb21cab1d =  3:  12a159d5c8 serve: add "receive" method for v2 capabilities table
 4:  5b661c9ed5 =  4:  f5c29b5cdf serve: provide "receive" function for object-format capability
 5:  34e0ce5c12 =  5:  063cb60d1e serve: provide "receive" function for session-id capability
 6:  8e42fa9aec !  6:  0ed0b946ea serve: drop "keys" strvec
    @@ serve.c: static int process_request(void)
      
      	packet_reader_init(&reader, 0, NULL, 0,
     @@ serve.c: static int process_request(void)
    - 			/* collect request; a sequence of keys and values */
    + 		case PACKET_READ_EOF:
    + 			BUG("Should have already died when seeing EOF");
    + 		case PACKET_READ_NORMAL:
    +-			/* collect request; a sequence of keys and values */
      			if (parse_command(reader.line, &command) ||
      			    receive_client_capability(reader.line))
     -				strvec_push(&keys, reader.line);
 7:  de7ac11ad3 !  7:  a9392d0e68 ls-refs: ignore very long ref-prefix counts
    @@ t/t5701-git-serve.sh: test_expect_success 'refs/heads prefix' '
     +	# from ls-refs.c.
     +	{
     +		echo command=ls-refs &&
    -+		echo object-format=$(test_oid algo)
    ++		echo object-format=$(test_oid algo) &&
     +		echo 0001 &&
     +		perl -le "print \"ref-prefix refs/heads/\$_\" for (1..65536)" &&
     +		echo 0000
 8:  3f78422fd3 =  8:  0a982f676a docs/protocol-v2: clarify some ls-refs ref-prefix details
 9:  6c55a7412d !  9:  553db6f83e serve: reject bogus v2 "command=ls-refs=foo"
    @@ Commit message
         When we see a line from the client like "command=ls-refs", we parse
         everything after the equals sign as a capability, which we check against
         our capabilities table. If we don't recognize the command (e.g.,
    -    "command=foo"), we'll reject it. But we use the same parser that checks
    -    for regular capabilities like "object-format=sha256". And so we'll
    -    accept "ls-refs=foo", even though everything after the equals is bogus,
    -    and simply ignored.
    +    "command=foo"), we'll reject it.
     
    -    This isn't really hurting anything, but the request does violate the
    -    spec. Let's tighten it up to prevent any surprising behavior.
    +    But in parse_command(), we use the same get_capability() parser for
    +    parsing non-command lines. So if we see "command=ls-refs=foo", we will
    +    feed "ls-refs=foo" to get_capability(), which will say "OK, that's
    +    ls-refs, with value 'foo'". But then we simply ignore the value
    +    entirely.
    +
    +    The client is violating the spec here, which says:
    +
    +          command = PKT-LINE("command=" key LF)
    +          key = 1*(ALPHA | DIGIT | "-_")
    +
    +    I.e., the key is not even allowed to have an equals sign in it. Whereas
    +    a real non-command capability does allow a value:
    +
    +          capability = PKT-LINE(key[=value] LF)
    +
    +    So by reusing the same get_capability() parser, we are mixing up the
    +    "key" and "capability" tokens. However, since that parser tells us
    +    whether it saw an "=", we can still use it; we just reject any input
    +    that produces a non-NULL value field.
    +
    +    The current behavior isn't really hurting anything (the client should
    +    never send such a request, and if it does, we just ignore the "value"
    +    part). But since it does violate the spec, let's tighten it up to
    +    prevent any surprising behavior.
     
         Signed-off-by: Jeff King <peff@peff.net>
     
    @@ t/t5701-git-serve.sh: test_expect_success 'request invalid command' '
      '
      
     +test_expect_success 'requested command is command=value' '
    -+	test-tool pkt-line pack >in <<-\EOF &&
    ++	test-tool pkt-line pack >in <<-EOF &&
     +	command=ls-refs=whatever
     +	object-format=$(test_oid algo)
     +	0000
10:  bbb12669b9 ! 10:  93216b9eaa serve: reject commands used as capabilities
    @@ t/t5701-git-serve.sh: test_expect_success 'request invalid command' '
      '
      
     +test_expect_success 'request capability as command' '
    -+	test-tool pkt-line pack >in <<-\EOF &&
    ++	test-tool pkt-line pack >in <<-EOF &&
     +	command=agent
    ++	object-format=$(test_oid algo)
     +	0000
     +	EOF
     +	test_must_fail test-tool serve-v2 --stateless-rpc 2>err <in &&
     +	grep invalid.command.*agent err
     +'
     +
     +test_expect_success 'request command as capability' '
    -+	test-tool pkt-line pack >in <<-\EOF &&
    ++	test-tool pkt-line pack >in <<-EOF &&
     +	command=ls-refs
    ++	object-format=$(test_oid algo)
     +	fetch
     +	0000
     +	EOF
    @@ t/t5701-git-serve.sh: test_expect_success 'request invalid command' '
     +'
     +
      test_expect_success 'requested command is command=value' '
    - 	test-tool pkt-line pack >in <<-\EOF &&
    + 	test-tool pkt-line pack >in <<-EOF &&
      	command=ls-refs=whatever
11:  375a85b9a6 = 11:  30802eeb54 ls-refs: reject unknown arguments