| Message ID | YUE2J74PP8TGthOZ@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: > The v2 ls-refs command may receive extra arguments from the client, one > per pkt-line. The spec is pretty clear that the arguments must come from > a specified set, but we silently ignore any unknown entries. For a > well-behaved client this doesn't matter, but it makes testing and > debugging more confusing. Let's tighten this up to match the spec. > > In theory this liberal behavior _could_ be useful for extending the > protocol. But: > > - every other part of the protocol requires that the server first > indicate that it supports the argument; this includes the fetch and > object-info commands, plus the "unborn" capability added to ls-refs > itself > > - it's not a very good extension mechanism anyway; without the server > advertising support, clients would have no idea if the argument was > silently ignored, or accepted and simply had no effect > > So we're not really losing anything by tightening this. > > Signed-off-by: Jeff King <peff@peff.net> > --- > ls-refs.c | 2 ++ > t/t5701-git-serve.sh | 13 +++++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/ls-refs.c b/ls-refs.c > index 18c4f41e87..460ac9b229 100644 > --- a/ls-refs.c > +++ b/ls-refs.c > @@ -168,6 +168,8 @@ int ls_refs(struct repository *r, struct packet_reader *request) > } > else if (!strcmp("unborn", arg)) > data.unborn = allow_unborn; > + else > + die(_("unexpected line: '%s'"), arg); > } > > if (request->status != PACKET_READ_FLUSH) > diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh > index b027ba9b06..e4d60bc605 100755 > --- a/t/t5701-git-serve.sh > +++ b/t/t5701-git-serve.sh > @@ -145,6 +145,19 @@ test_expect_success 'basics of ls-refs' ' > test_cmp expect actual > ' > > +test_expect_success 'ls-refs complains about unknown options' ' > + test-tool pkt-line pack >in <<-EOF && > + command=ls-refs > + object-format=$(test_oid algo) > + 0001 > + no-such-arg > + 0000 > + EOF > + > + test_must_fail test-tool serve-v2 --stateless-rpc 2>err <in && > + grep unexpected.line.*no-such-arg err > +' > + > test_expect_success 'basic ref-prefixes' ' > test-tool pkt-line pack >in <<-EOF && > command=ls-refs Looks good. For what it's worth some of this series is stuff I've been meaning to submit after my current in-flight series, so I guess we'll be playing some rebase ping-pong in this area. In my version of this I'd led with a change to do changes like these for all the protocol verbs: diff --git a/connect.c b/connect.c index 70b13389ba5..5e991a92279 100644 --- a/connect.c +++ b/connect.c @@ -514,6 +514,8 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader, packet_write_fmt(fd_out, "symrefs\n"); if (server_supports_feature("ls-refs", "unborn", 0)) packet_write_fmt(fd_out, "unborn\n"); + if (git_env_bool("GIT_TEST_PROTOCOL_BAD_LS_REFS", 0)) + packet_write_fmt(fd_out, "test-bad-client\n"); for (i = 0; ref_prefixes && i < ref_prefixes->nr; i++) { packet_write_fmt(fd_out, "ref-prefix %s\n", ref_prefixes->v[i]); So the below patch isn't anywhere near applying, but you can see the test coverage (those tests are new) & what changes if we instrument a client to actually send this bad data. That packet_client_error() function is new, part of what I'm doing there is converting all of this error emitting from die() to properly sending ERR packets. I think a bug in your versio is that you're using _() here, if your server program happens to be started in Chinese you probably still want to emit errors to clients in LANG=C. That's why I'm using N_() in my version, it knows to emit the die() localized, but the ERR packet in LANG=C, no you'll get the i18n message in the server terminal, but the client gets LANG=C. Of course that actually working is subject to various races that I may or may not be able to untangle... -- >8 -- protocol v2: correct & adjust "ls-refs" ERR behavior Change the protocol v2 "ls-refs" to error out on unknown arguments, before this we'd silently ignore unknown arguments. This brings us in line with the behavior we've had in "fetch" (i.e. upload-pack.c) since 3145ea957d2 (upload-pack: introduce fetch server command, 2018-03-15) (see the "/* ignore unknown lines maybe? */" comment in "process_args()". The looser behavior in "ls-refs" seems to have been unintentionally added in 72d0ea0056b (ls-refs: introduce ls-refs server command, 2018-03-15). I've also changed the wording of the "expected flush" message in "ls-refs" to be consistent with "fetch" and "object-info". In the changes leading up to this we had to grep out the racy "fatal: the remote end hung up unexpectedly" messages. Now we need to guard the full test_cmp with a test_expect_failure. I could also write a looser test, but this makes subsequent changes smaller, and is more accurate. Most of the time the test_expect_failure() succeeds, but in some cases the two messages will be out-of-order, or e.g. only the STDERR one will make it, and not the ERR message, or the other way around. I.e. now that we emit both an ERR line and die() on the remote, we might see ERR before die(), or die() before ERR. This race does not happen in the t5701-git-serve.sh and other "test-tool pkt-line" tests, which only use one process. Ideally we should only emit ERR and not also something on STDERR when we die() on the server. Subsequent commits will address that, but for now let's keep that behavior. I.e. we'll now emit two mostly duplicate errors in some cases from the client, or one where we emitted none (or one that made no sense) before. The current client implementation doesn't expect the server to be this overly helpful in giving us errors both via pkt-line and STDERR, so we usually get two lines, and in some cases just one (but never zero). Subsequent commits will address this. I.e. we can assume that the client can format the ERR lines, and that we can just exit(128) or disconnect on the server, not die(). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- ls-refs.c | 6 +++++- t/t5703-protocol-v2-file.sh | 24 +++++++++++++++++------- t/t5704-protocol-v2-git.sh | 20 +++++++++++++------- t/t5705-protocol-v2-http.sh | 25 +++++++++++++++++++------ t/t5710-protocol-violations.sh | 2 +- 5 files changed, 55 insertions(+), 22 deletions(-) diff --git a/ls-refs.c b/ls-refs.c index 1e50e785665..f9bd577dcd2 100644 --- a/ls-refs.c +++ b/ls-refs.c @@ -150,11 +150,15 @@ int ls_refs(struct repository *r, struct packet_reader *request) strvec_push(&data.prefixes, out); else if (!strcmp("unborn", arg)) data.unborn = allow_unborn; + else + packet_client_error(&data.writer, + N_("ls-refs: unexpected argument: '%s'"), + request->line); } if (request->status != PACKET_READ_FLUSH) packet_client_error(&data.writer, - N_("expected flush after ls-refs arguments")); + N_("ls-refs: expected flush after arguments")); send_possibly_unborn_head(&data); if (!data.prefixes.nr) diff --git a/t/t5703-protocol-v2-file.sh b/t/t5703-protocol-v2-file.sh index e572767ee00..79457eaf101 100755 --- a/t/t5703-protocol-v2-file.sh +++ b/t/t5703-protocol-v2-file.sh @@ -32,18 +32,28 @@ test_expect_success 'list refs with file:// using protocol v2' ' test_expect_success 'ls-remote handling a bad client using file:// protocol v2' ' test_when_finished "rm -f log" && - cat >expect <<-EOF && - $(git -C file_parent rev-parse refs/heads/main)$(printf "\t")refs/heads/main + cat >log.expect <<-\EOF && + packet: upload-pack> ERR ls-refs: unexpected argument: '"'"'test-bad-client'"'"' + packet: git< ERR ls-refs: unexpected argument: '"'"'test-bad-client'"'"' EOF - env \ + cat >err.expect <<-\EOF && + fatal: ls-refs: unexpected argument: '"'"'test-bad-client'"'"' + fatal: remote error: ls-refs: unexpected argument: '"'"'test-bad-client'"'"' + EOF + test_must_fail env \ GIT_TRACE_PACKET="$(pwd)/log" \ GIT_TEST_PROTOCOL_BAD_LS_REFS=true \ git -c protocol.version=2 \ - ls-remote "file://$(pwd)/file_parent" main >actual 2>err && + ls-remote "file://$(pwd)/file_parent" main >out 2>err.actual && - test_must_be_empty err && - test_cmp expect actual && - grep "git> test-bad-client$" log + grep "unexpected argument.*test-bad-client" err.actual && + test_must_be_empty out && + grep ERR log >log.actual && + test_cmp log.expect log.actual +' + +test_expect_failure 'ls-remote ERR and die() is racy under file:// protocol v2' ' + test_cmp err.expect err.actual ' test_expect_success 'ref advertisement is filtered with ls-remote using protocol v2' ' diff --git a/t/t5704-protocol-v2-git.sh b/t/t5704-protocol-v2-git.sh index c9293bbaad2..123c3f56ef8 100755 --- a/t/t5704-protocol-v2-git.sh +++ b/t/t5704-protocol-v2-git.sh @@ -48,19 +48,25 @@ test_expect_success 'ref advertisement is filtered with ls-remote using protocol test_cmp expect actual ' -test_expect_success 'ls-remote handling a bad client using git:// protocol v2' ' +test_expect_success 'ls-remote handling a bad client using protocol v2' ' test_when_finished "rm -f log" && - git ls-remote "$daemon_parent" >expect && - env \ + cat >err.expect <<-EOF && + fatal: remote error: ls-refs: unexpected argument: '"'"'test-bad-client'"'"' + EOF + test_must_fail env \ GIT_TRACE_PACKET="$(pwd)/log" \ GIT_TEST_PROTOCOL_BAD_LS_REFS=true \ git -c protocol.version=2 \ - ls-remote "$GIT_DAEMON_URL/parent" >actual 2>err && + ls-remote "$GIT_DAEMON_URL/parent" main >out.actual 2>err.actual && - test_must_be_empty err && - test_cmp expect actual && - grep "git> test-bad-client$" log + test_must_be_empty out.actual && + grep "unexpected argument.*test-bad-client" err.actual && + grep "ERR ls-refs: unexpected argument.*test-bad-client" log +' + +test_expect_failure 'ls-remote ERR and die() is racy under file:// protocol v2' ' + test_cmp err.expect err.actual ' test_expect_success 'clone with git:// using protocol v2' ' diff --git a/t/t5705-protocol-v2-http.sh b/t/t5705-protocol-v2-http.sh index 5982c38743e..6f526e0829f 100755 --- a/t/t5705-protocol-v2-http.sh +++ b/t/t5705-protocol-v2-http.sh @@ -192,16 +192,29 @@ test_expect_success 'fetch from namespaced repo respects namespaces' ' test_expect_success 'ls-remote handling a bad client using http:// protocol v2' ' test_when_finished "rm -f log" && - git ls-remote "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" >expect && - env \ + cat >log.expect <<-\EOF && + packet: upload-pack> ERR ls-refs: unexpected argument: '"'"'test-bad-client'"'"' + packet: git< ERR ls-refs: unexpected argument: '"'"'test-bad-client'"'"' + EOF + + cat >err.expect <<-\EOF && + fatal: ls-refs: unexpected argument: '"'"'test-bad-client'"'"' + fatal: remote error: ls-refs: unexpected argument: '"'"'test-bad-client'"'"' + EOF + test_must_fail env \ GIT_TRACE_PACKET="$(pwd)/log" \ GIT_TEST_PROTOCOL_BAD_LS_REFS=true \ git -c protocol.version=2 \ - ls-remote "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" >actual 2>err && + ls-remote "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" >out 2>err.actual && - test_must_be_empty err && - test_cmp expect actual && - grep "git> test-bad-client$" log + grep "unexpected argument.*test-bad-client" err.actual && + test_must_be_empty out && + grep ERR log >log.actual && + test_cmp log.expect log.actual +' + +test_expect_failure 'ls-remote ERR and die() is racy under http:// protocol v2' ' + test_cmp err.expect err.actual ' test_expect_success 'ls-remote with v2 http sends only one POST' ' diff --git a/t/t5710-protocol-violations.sh b/t/t5710-protocol-violations.sh index 44e2c0d3ded..75a457d39ca 100755 --- a/t/t5710-protocol-violations.sh +++ b/t/t5710-protocol-violations.sh @@ -16,7 +16,7 @@ test_expect_success 'extra delim packet in v2 ls-refs args' ' EOF cat >err.expect <<-\EOF && - fatal: expected flush after ls-refs arguments + fatal: ls-refs: expected flush after arguments EOF test_must_fail env GIT_PROTOCOL=version=2 \ git upload-pack . <input 2>err.actual &&
On Wed, Sep 15, 2021 at 02:09:16AM +0200, Ævar Arnfjörð Bjarmason wrote: > So the below patch isn't anywhere near applying, but you can see the > test coverage (those tests are new) & what changes if we instrument a > client to actually send this bad data. > > That packet_client_error() function is new, part of what I'm doing there > is converting all of this error emitting from die() to properly sending > ERR packets. Right, I noticed that none of this is likely to make it to a client (unless they are using file:// or an ssh channel which passes back stderr). I agree that we should probably be passing these back via ERR packets, but note that the client is racy here. If the server reports an error and dies while the client is still writing, they'll see SIGPIPE and exit without showing the user the ERR packet. The solution may be something along these lines: https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/ Alternatively, the server can pump the client data to /dev/null until we hit a flush, and _then_ write the ERR. But that doesn't work for some protocol-level errors (like "oops, I'm having trouble reading your pkt-lines). > I think a bug in your versio is that you're using _() here, if your > server program happens to be started in Chinese you probably still want > to emit errors to clients in LANG=C. I'm not sure it's a bug. If you set LANG=zh on your server, that might be harmful (if you serve a multi-lingual international audience), or it might be helpful (if it's a company server where everybody speaks the language). Likewise, for a file:// or ssh:// operation, your local LANG would kick in. I don't really have a strong opinion either way on whether it's helpful or hindering overall. But I did follow the convention of nearby code in this series, so I think we don't need to worry about it now (it also seemed inconsistent to me; serve.c does not mark for translation, but ls-refs.c does). > Of course that actually working is subject to various races that I may > or may not be able to untangle... Oh good, so you know about them. :) -Peff
diff --git a/ls-refs.c b/ls-refs.c index 18c4f41e87..460ac9b229 100644 --- a/ls-refs.c +++ b/ls-refs.c @@ -168,6 +168,8 @@ int ls_refs(struct repository *r, struct packet_reader *request) } else if (!strcmp("unborn", arg)) data.unborn = allow_unborn; + else + die(_("unexpected line: '%s'"), arg); } if (request->status != PACKET_READ_FLUSH) diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh index b027ba9b06..e4d60bc605 100755 --- a/t/t5701-git-serve.sh +++ b/t/t5701-git-serve.sh @@ -145,6 +145,19 @@ test_expect_success 'basics of ls-refs' ' test_cmp expect actual ' +test_expect_success 'ls-refs complains about unknown options' ' + test-tool pkt-line pack >in <<-EOF && + command=ls-refs + object-format=$(test_oid algo) + 0001 + no-such-arg + 0000 + EOF + + test_must_fail test-tool serve-v2 --stateless-rpc 2>err <in && + grep unexpected.line.*no-such-arg err +' + test_expect_success 'basic ref-prefixes' ' test-tool pkt-line pack >in <<-EOF && command=ls-refs
The v2 ls-refs command may receive extra arguments from the client, one per pkt-line. The spec is pretty clear that the arguments must come from a specified set, but we silently ignore any unknown entries. For a well-behaved client this doesn't matter, but it makes testing and debugging more confusing. Let's tighten this up to match the spec. In theory this liberal behavior _could_ be useful for extending the protocol. But: - every other part of the protocol requires that the server first indicate that it supports the argument; this includes the fetch and object-info commands, plus the "unborn" capability added to ls-refs itself - it's not a very good extension mechanism anyway; without the server advertising support, clients would have no idea if the argument was silently ignored, or accepted and simply had no effect So we're not really losing anything by tightening this. Signed-off-by: Jeff King <peff@peff.net> --- ls-refs.c | 2 ++ t/t5701-git-serve.sh | 13 +++++++++++++ 2 files changed, 15 insertions(+)