| Message ID | YUE1ym1dALRQLztq@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | limit memory allocations for v2 servers | expand |
On Tue, Sep 14 2021, Jeff King wrote: > 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. > > This isn't really hurting anything, but the request does violate the > spec. Let's tighten it up to prevent any surprising behavior. Doesn't need a re-roll, but just for my own sanity: By violating the spec you mean it doesn't coform to the "key" in the "Capability Advertisement" section of protocol-v2.txt, one might skim this and think values with "=" in them are OK, but a command=WHATEVER has a WHATEVER as a "key", not a "value", that's for other parts of the dialog. But you could also have meant that it violates the spec because there's no such command as "ls-refs=whatever", just like there isn't "foobar", but I don't think that's what you mean... > Signed-off-by: Jeff King <peff@peff.net> > --- > serve.c | 2 +- > t/t5701-git-serve.sh | 10 ++++++++++ > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/serve.c b/serve.c > index 5ea6c915cb..63ee1be7ff 100644 > --- a/serve.c > +++ b/serve.c > @@ -220,7 +220,7 @@ static int parse_command(const char *key, struct protocol_capability **command) > if (*command) > die("command '%s' requested after already requesting command '%s'", > out, (*command)->name); > - if (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command) > + if (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command || value) > die("invalid command '%s'", out); > > *command = cmd; > diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh > index 3bc96ebcde..ab15078bc0 100755 > --- a/t/t5701-git-serve.sh > +++ b/t/t5701-git-serve.sh > @@ -72,6 +72,16 @@ test_expect_success 'request invalid command' ' > test_i18ngrep "invalid command" err > ' > > +test_expect_success 'requested command is command=value' ' > + test-tool pkt-line pack >in <<-\EOF && > + command=ls-refs=whatever > + object-format=$(test_oid algo) > + 0000 > + EOF > + test_must_fail test-tool serve-v2 --stateless-rpc 2>err <in && > + grep invalid.command.*ls-refs=whatever err > +' > + > test_expect_success 'wrong object-format' ' > test-tool pkt-line pack >in <<-EOF && > command=fetch
On Tue, Sep 14, 2021 at 7:52 PM Jeff King <peff@peff.net> wrote: > 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. > > This isn't really hurting anything, but the request does violate the > spec. Let's tighten it up to prevent any surprising behavior. > > Signed-off-by: Jeff King <peff@peff.net> > --- > diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh > @@ -72,6 +72,16 @@ test_expect_success 'request invalid command' ' > +test_expect_success 'requested command is command=value' ' > + test-tool pkt-line pack >in <<-\EOF && > + command=ls-refs=whatever > + object-format=$(test_oid algo) > + 0000 > + EOF This here-doc uses <<-\EOF yet (presumably) expects $(test_oid algo) to be expanded. I'm guessing that you meant <<-EOF but never noticed... > + test_must_fail test-tool serve-v2 --stateless-rpc 2>err <in && ... because of this test_must_fail(). > + grep invalid.command.*ls-refs=whatever err > +'
On Wed, Sep 15, 2021 at 02:27:35AM +0200, Ævar Arnfjörð Bjarmason wrote: > On Tue, Sep 14 2021, Jeff King wrote: > > > 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. > > > > This isn't really hurting anything, but the request does violate the > > spec. Let's tighten it up to prevent any surprising behavior. > > Doesn't need a re-roll, but just for my own sanity: > > By violating the spec you mean it doesn't coform to the "key" in the > "Capability Advertisement" section of protocol-v2.txt, one might skim > this and think values with "=" in them are OK, but a command=WHATEVER > has a WHATEVER as a "key", not a "value", that's for other parts of the > dialog. > > But you could also have meant that it violates the spec because there's > no such command as "ls-refs=whatever", just like there isn't "foobar", > but I don't think that's what you mean... Both, I think. :) The line "command=foo=bar" is obvious nonsense by the spec, no matter which way you interpret it: - "foo" is a command name, but adding a value does not make sense when it is used in this context, so it's invalid - "foo=bar" is not a command name Our parser does pick out "foo", so in terms of the code it is more like the former. But since it is invalid either way, I'm not sure the distinction matters. -Peff
On Wed, Sep 15, 2021 at 01:09:19AM -0400, Eric Sunshine wrote: > > diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh > > @@ -72,6 +72,16 @@ test_expect_success 'request invalid command' ' > > +test_expect_success 'requested command is command=value' ' > > + test-tool pkt-line pack >in <<-\EOF && > > + command=ls-refs=whatever > > + object-format=$(test_oid algo) > > + 0000 > > + EOF > > This here-doc uses <<-\EOF yet (presumably) expects $(test_oid algo) > to be expanded. I'm guessing that you meant <<-EOF but never > noticed... Ah, thanks. It's muscle memory to inhibit expansion in my here-docs. :) > > + test_must_fail test-tool serve-v2 --stateless-rpc 2>err <in && > > ... because of this test_must_fail(). > > > + grep invalid.command.*ls-refs=whatever err > > +' Actually, it's a little more complicated. The "grep" here would notice if we failed for the wrong reason. But we never actually look at the object-format line at all, because we barf as soon as we see the bogus command line. So the object-format line is superfluous in this test. I added it mostly for consistency with the other tests (including the existing unknown-command test, which is in the same boat). But it also makes the test slightly more robust, in that the command is more likely to succeed (and thus fail the test) if the code accidentally did not notice the wrong command line. But obviously it's worth fixing. Thanks for noticing. -Peff
Jeff King <peff@peff.net> writes: > 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. Maybe I am slow but I had to read the above a few times and finally look at the implementation of parse_command() to realize that what the last sentence describes is: When parse_command() is fed "command=ls-refs=foo", it strips "command=", feeds "ls-refs=foo" to get_capability(), and because we do not ensure value is NULL, we silently ignore "=foo" that is bogus. And it makes sense. It would probably have helped if I peeked the updated test ;-) > This isn't really hurting anything, but the request does violate the > spec. Let's tighten it up to prevent any surprising behavior. Good.
On Wed, Sep 15, 2021 at 10:33:32AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > 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. > > Maybe I am slow but I had to read the above a few times and finally > look at the implementation of parse_command() to realize that what > the last sentence describes is: > > When parse_command() is fed "command=ls-refs=foo", it strips > "command=", feeds "ls-refs=foo" to get_capability(), and because > we do not ensure value is NULL, we silently ignore "=foo" that > is bogus. > > And it makes sense. It would probably have helped if I peeked the > updated test ;-) Since I'm re-rolling anyway, I'll expand it a bit (and also cover Ævar's "what exactly does violate mean here" question). -Peff
diff --git a/serve.c b/serve.c index 5ea6c915cb..63ee1be7ff 100644 --- a/serve.c +++ b/serve.c @@ -220,7 +220,7 @@ static int parse_command(const char *key, struct protocol_capability **command) if (*command) die("command '%s' requested after already requesting command '%s'", out, (*command)->name); - if (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command) + if (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command || value) die("invalid command '%s'", out); *command = cmd; diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh index 3bc96ebcde..ab15078bc0 100755 --- a/t/t5701-git-serve.sh +++ b/t/t5701-git-serve.sh @@ -72,6 +72,16 @@ test_expect_success 'request invalid command' ' test_i18ngrep "invalid command" err ' +test_expect_success 'requested command is command=value' ' + test-tool pkt-line pack >in <<-\EOF && + command=ls-refs=whatever + object-format=$(test_oid algo) + 0000 + EOF + test_must_fail test-tool serve-v2 --stateless-rpc 2>err <in && + grep invalid.command.*ls-refs=whatever err +' + test_expect_success 'wrong object-format' ' test-tool pkt-line pack >in <<-EOF && command=fetch
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. This isn't really hurting anything, but the request does violate the spec. Let's tighten it up to prevent any surprising behavior. Signed-off-by: Jeff King <peff@peff.net> --- serve.c | 2 +- t/t5701-git-serve.sh | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-)