* [PATCH v2 01/11] serve: rename is_command() to parse_command()
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
@ 2021-09-14 23:51 ` Jeff King
2021-09-14 23:51 ` [PATCH v2 02/11] serve: return capability "value" from get_capability() Jeff King
` (11 subsequent siblings)
12 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-14 23:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
The is_command() function not only tells us whether the pktline is a
valid command string, but it also parses out the command (and complains
if we see a duplicate). Let's rename it to make those extra functions
a bit more obvious.
Signed-off-by: Jeff King <peff@peff.net>
---
serve.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/serve.c b/serve.c
index 1817edc7f5..fd88b95343 100644
--- a/serve.c
+++ b/serve.c
@@ -163,7 +163,7 @@ static int is_valid_capability(const char *key)
return c && c->advertise(the_repository, NULL);
}
-static int is_command(const char *key, struct protocol_capability **command)
+static int parse_command(const char *key, struct protocol_capability **command)
{
const char *out;
@@ -251,7 +251,7 @@ static int process_request(void)
BUG("Should have already died when seeing EOF");
case PACKET_READ_NORMAL:
/* collect request; a sequence of keys and values */
- if (is_command(reader.line, &command) ||
+ if (parse_command(reader.line, &command) ||
is_valid_capability(reader.line))
strvec_push(&keys, reader.line);
else
--
2.33.0.917.gae6ecbedc7
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v2 02/11] serve: return capability "value" from get_capability()
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
2021-09-14 23:51 ` [PATCH v2 01/11] serve: rename is_command() to parse_command() Jeff King
@ 2021-09-14 23:51 ` Jeff King
2021-09-14 23:51 ` [PATCH v2 03/11] serve: add "receive" method for v2 capabilities table Jeff King
` (10 subsequent siblings)
12 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-14 23:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
When the client sends v2 capabilities, they may be simple, like:
foo
or have a value like:
foo=bar
(all of the current capabilities actually expect a value, but the
protocol allows for boolean ones).
We use get_capability() to make sure the client's pktline matches a
capability. In doing so, we parse enough to see the "=" and the value
(if any), but we immediately forget it. Nobody cares for now, because they end
up parsing the values out later using has_capability(). But in
preparation for changing that, let's pass back a pointer so the callers
know what we found.
Note that unlike has_capability(), we'll return NULL for a "simple"
capability. Distinguishing these will be useful for some future patches.
Signed-off-by: Jeff King <peff@peff.net>
---
serve.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/serve.c b/serve.c
index fd88b95343..78a4e83554 100644
--- a/serve.c
+++ b/serve.c
@@ -139,7 +139,7 @@ void protocol_v2_advertise_capabilities(void)
strbuf_release(&value);
}
-static struct protocol_capability *get_capability(const char *key)
+static struct protocol_capability *get_capability(const char *key, const char **value)
{
int i;
@@ -149,16 +149,25 @@ static struct protocol_capability *get_capability(const char *key)
for (i = 0; i < ARRAY_SIZE(capabilities); i++) {
struct protocol_capability *c = &capabilities[i];
const char *out;
- if (skip_prefix(key, c->name, &out) && (!*out || *out == '='))
+ if (!skip_prefix(key, c->name, &out))
+ continue;
+ if (!*out) {
+ *value = NULL;
return c;
+ }
+ if (*out++ == '=') {
+ *value = out;
+ return c;
+ }
}
return NULL;
}
static int is_valid_capability(const char *key)
{
- const struct protocol_capability *c = get_capability(key);
+ const char *value;
+ const struct protocol_capability *c = get_capability(key, &value);
return c && c->advertise(the_repository, NULL);
}
@@ -168,7 +177,8 @@ static int parse_command(const char *key, struct protocol_capability **command)
const char *out;
if (skip_prefix(key, "command=", &out)) {
- struct protocol_capability *cmd = get_capability(out);
+ const char *value;
+ struct protocol_capability *cmd = get_capability(out, &value);
if (*command)
die("command '%s' requested after already requesting command '%s'",
--
2.33.0.917.gae6ecbedc7
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v2 03/11] serve: add "receive" method for v2 capabilities table
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
2021-09-14 23:51 ` [PATCH v2 01/11] serve: rename is_command() to parse_command() Jeff King
2021-09-14 23:51 ` [PATCH v2 02/11] serve: return capability "value" from get_capability() Jeff King
@ 2021-09-14 23:51 ` Jeff King
2021-09-15 0:31 ` Ævar Arnfjörð Bjarmason
2021-09-15 16:41 ` Junio C Hamano
2021-09-14 23:51 ` [PATCH v2 04/11] serve: provide "receive" function for object-format capability Jeff King
` (9 subsequent siblings)
12 siblings, 2 replies; 77+ messages in thread
From: Jeff King @ 2021-09-14 23:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
We have a capabilities table that tells us what we should tell the
client we are capable of, and what to do when a client gives us a
particular command (e.g., "command=ls-refs"). But it doesn't tell us
what to do when the client sends us back a capability (e.g.,
"object-format=sha256"). We just collect them all in a strvec and hope
somebody can use them later.
Instead, let's provide a function pointer in the table to act on these.
This will eventually help us avoid collecting the strings, which will be
more efficient and less prone to mischief.
Using the new method is optional, which helps in two ways:
- we can move existing capabilities over to this new system gradually
in individual commits
- some capabilities we don't actually do anything with anyway. For
example, the client is free to say "agent=git/1.2.3" to us, but we
do not act on the information in any way.
Signed-off-by: Jeff King <peff@peff.net>
---
serve.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/serve.c b/serve.c
index 78a4e83554..a161189984 100644
--- a/serve.c
+++ b/serve.c
@@ -70,6 +70,16 @@ struct protocol_capability {
* This field should be NULL for capabilities which are not commands.
*/
int (*command)(struct repository *r, struct packet_reader *request);
+
+ /*
+ * Function called when a client requests the capability as a
+ * non-command. This may be NULL if the capability does nothing.
+ *
+ * For a capability of the form "foo=bar", the value string points to
+ * the content after the "=" (i.e., "bar"). For simple capabilities
+ * (just "foo"), it is NULL.
+ */
+ void (*receive)(struct repository *r, const char *value);
};
static struct protocol_capability capabilities[] = {
@@ -164,12 +174,17 @@ static struct protocol_capability *get_capability(const char *key, const char **
return NULL;
}
-static int is_valid_capability(const char *key)
+static int receive_client_capability(const char *key)
{
const char *value;
const struct protocol_capability *c = get_capability(key, &value);
- return c && c->advertise(the_repository, NULL);
+ if (!c || !c->advertise(the_repository, NULL))
+ return 0;
+
+ if (c->receive)
+ c->receive(the_repository, value);
+ return 1;
}
static int parse_command(const char *key, struct protocol_capability **command)
@@ -262,7 +277,7 @@ static int process_request(void)
case PACKET_READ_NORMAL:
/* collect request; a sequence of keys and values */
if (parse_command(reader.line, &command) ||
- is_valid_capability(reader.line))
+ receive_client_capability(reader.line))
strvec_push(&keys, reader.line);
else
die("unknown capability '%s'", reader.line);
--
2.33.0.917.gae6ecbedc7
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2 03/11] serve: add "receive" method for v2 capabilities table
2021-09-14 23:51 ` [PATCH v2 03/11] serve: add "receive" method for v2 capabilities table Jeff King
@ 2021-09-15 0:31 ` Ævar Arnfjörð Bjarmason
2021-09-15 16:35 ` Jeff King
2021-09-15 16:41 ` Junio C Hamano
1 sibling, 1 reply; 77+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-15 0:31 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Taylor Blau, Martin Ågren
On Tue, Sep 14 2021, Jeff King wrote:
> -static int is_valid_capability(const char *key)
> +static int receive_client_capability(const char *key)
> {
> const char *value;
> const struct protocol_capability *c = get_capability(key, &value);
>
> - return c && c->advertise(the_repository, NULL);
> + if (!c || !c->advertise(the_repository, NULL))
> + return 0;
> +
> + if (c->receive)
> + c->receive(the_repository, value);
> + return 1;
> }
>
I haven't actually run this yet (and need to zZzZ soon), but AFAICT at
the end of this series you leave the existing advertise semantics of
advertise() be (which is fine). I have this unsubmitted patch locally
which you may or may not want to work into this.
I considered splitting up the advertise() method as well, i.e. we could
have a new "is_advertised" boolean callback, and then a
"capability_string" or whatever. "server-option" and "object-info" never
add anything, so they could leave that as NULL.
But it's probably not worth it, just food for thought...
-- >8 -- serve: document that "advertise" is called in two modes
If we're being called with a non-NULL argument we're sending out the
advertisement line, but if it's NULL we're actually in the middle of a
request.
So you can use the check for NULL to emit your own "die" on "return
0", like "wtf, I said I don't support command xyz, why are you calling
it?", as opposed to the default "invalid command '%s'".
Maybe nobody cares, and we can't assume that we're going from an
advertisement to a command for the same request anyway (can
we?). I.e. are we serving multiple clients?
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
serve.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/serve.c b/serve.c
index aa8209f147e..b187ce26911 100644
--- a/serve.c
+++ b/serve.c
@@ -55,6 +55,12 @@ struct protocol_capability {
* Optionally a value can be specified by adding it to 'value'.
* If a value is added to 'value', the server will advertise this
* capability as "<name>=<value>" instead of "<name>".
+ *
+ * When called with a NULL value we're past the advertisement
+ * itself, and are checking during a request whether we
+ * support this command. This can be checked and used used to
+ * e.g. emit a die("we support this, but don't have it
+ * enabled!") error.
*/
int (*advertise)(struct repository *r, struct strbuf *value);
--
2.33.0.1013.ge8323766266
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2 03/11] serve: add "receive" method for v2 capabilities table
2021-09-15 0:31 ` Ævar Arnfjörð Bjarmason
@ 2021-09-15 16:35 ` Jeff King
0 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 16:35 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Taylor Blau, Martin Ågren
On Wed, Sep 15, 2021 at 02:31:28AM +0200, Ævar Arnfjörð Bjarmason wrote:
> I haven't actually run this yet (and need to zZzZ soon), but AFAICT at
> the end of this series you leave the existing advertise semantics of
> advertise() be (which is fine). I have this unsubmitted patch locally
> which you may or may not want to work into this.
>
> I considered splitting up the advertise() method as well, i.e. we could
> have a new "is_advertised" boolean callback, and then a
> "capability_string" or whatever. "server-option" and "object-info" never
> add anything, so they could leave that as NULL.
>
> But it's probably not worth it, just food for thought...
Yes, I noticed the advertise() function is really doing double-duty
here. Nothing changes with respect to it in my series, but I agree it's
a bit confusing.
The functions are simple enough that splitting up the two functions of
advertise() might make sense. Likewise, your documentation seems
reasonable. I'd prefer to punt on it for now, though, as my series isn't
otherwise touching this function.
I'm sure there are textual conflicts with what I'm doing here, since
it's nearby, but I'd prefer to just build more cleanups on top later.
-Peff
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH v2 03/11] serve: add "receive" method for v2 capabilities table
2021-09-14 23:51 ` [PATCH v2 03/11] serve: add "receive" method for v2 capabilities table Jeff King
2021-09-15 0:31 ` Ævar Arnfjörð Bjarmason
@ 2021-09-15 16:41 ` Junio C Hamano
2021-09-15 16:57 ` Jeff King
1 sibling, 1 reply; 77+ messages in thread
From: Junio C Hamano @ 2021-09-15 16:41 UTC (permalink / raw)
To: Jeff King
Cc: git, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
Jeff King <peff@peff.net> writes:
> + /*
> + * Function called when a client requests the capability as a
> + * non-command. This may be NULL if the capability does nothing.
> + *
> + * For a capability of the form "foo=bar", the value string points to
> + * the content after the "=" (i.e., "bar"). For simple capabilities
> + * (just "foo"), it is NULL.
> + */
> + void (*receive)(struct repository *r, const char *value);
What does "as a non-command" mean? To put it another way, when a
client requests the capability as a command, what does the receive
method do differently?
> @@ -164,12 +174,17 @@ static struct protocol_capability *get_capability(const char *key, const char **
> return NULL;
> }
>
> -static int is_valid_capability(const char *key)
> +static int receive_client_capability(const char *key)
> {
> const char *value;
> const struct protocol_capability *c = get_capability(key, &value);
>
> - return c && c->advertise(the_repository, NULL);
> + if (!c || !c->advertise(the_repository, NULL))
> + return 0;
> +
> + if (c->receive)
> + c->receive(the_repository, value);
> + return 1;
> }
>
> static int parse_command(const char *key, struct protocol_capability **command)
> @@ -262,7 +277,7 @@ static int process_request(void)
> case PACKET_READ_NORMAL:
> /* collect request; a sequence of keys and values */
The comment tentatively gets slightly stale here, but that will be
corrected at the end, so it would be fine ;-)
> if (parse_command(reader.line, &command) ||
> - is_valid_capability(reader.line))
> + receive_client_capability(reader.line))
> strvec_push(&keys, reader.line);
> else
> die("unknown capability '%s'", reader.line);
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2 03/11] serve: add "receive" method for v2 capabilities table
2021-09-15 16:41 ` Junio C Hamano
@ 2021-09-15 16:57 ` Jeff King
0 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 16:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
On Wed, Sep 15, 2021 at 09:41:28AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > + /*
> > + * Function called when a client requests the capability as a
> > + * non-command. This may be NULL if the capability does nothing.
> > + *
> > + * For a capability of the form "foo=bar", the value string points to
> > + * the content after the "=" (i.e., "bar"). For simple capabilities
> > + * (just "foo"), it is NULL.
> > + */
> > + void (*receive)(struct repository *r, const char *value);
>
> What does "as a non-command" mean? To put it another way, when a
> client requests the capability as a command, what does the receive
> method do differently?
For each entry in this capability table, clients can say:
command=foo
or just:
foo
The latter is a non-command. The "receive" function is not called at all
if it is a "command". I think this is a bit more clear when read
together with the existing function just above (which you can't see in
the diff context):
/*
* Function called when a client requests the capability as a command.
* Will be provided a struct packet_reader 'request' which it should
* use to read the command specific part of the request. Every command
* MUST read until a flush packet is seen before sending a response.
*
* This field should be NULL for capabilities which are not commands.
*/
int (*command)(struct repository *r, struct packet_reader *request);
I guess these could define "as a command", but I think it's pretty clear
in the context.
> > static int parse_command(const char *key, struct protocol_capability **command)
> > @@ -262,7 +277,7 @@ static int process_request(void)
> > case PACKET_READ_NORMAL:
> > /* collect request; a sequence of keys and values */
>
> The comment tentatively gets slightly stale here, but that will be
> corrected at the end, so it would be fine ;-)
Hmm. I think it is not stale here, as we are still collecting the "keys"
strvec. But it _does_ get stale by the end, when we stop doing so.
I'm preparing a re-roll for the test issues Eric noted, so I'll drop
that comment in the appropriate patch.
-Peff
^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH v2 04/11] serve: provide "receive" function for object-format capability
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
` (2 preceding siblings ...)
2021-09-14 23:51 ` [PATCH v2 03/11] serve: add "receive" method for v2 capabilities table Jeff King
@ 2021-09-14 23:51 ` Jeff King
2021-09-15 16:54 ` Junio C Hamano
2021-09-14 23:51 ` [PATCH v2 05/11] serve: provide "receive" function for session-id capability Jeff King
` (8 subsequent siblings)
12 siblings, 1 reply; 77+ messages in thread
From: Jeff King @ 2021-09-14 23:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
We get any "object-format" specified by the client by searching for it
in the collected list of capabilities the client sent. We can instead
just handle it as soon as they send it. This is slightly more efficient,
and gets us one step closer to dropping that collected list.
Note that we do still have to do our final hash check after receiving
all capabilities (because they might not have sent an object-format line
at all, and we still have to check that the default matches our
repository algorithm). Since the check_algorithm() function would now be
down to a single if() statement, I've just inlined it in its only
caller.
There should be no change of behavior here, except for two
broken-protocol cases:
- if the client sends multiple conflicting object-format capabilities
(which they should not), we'll now choose the last one rather than
the first. We could also detect and complain about the duplicates
quite easily now, which we could not before, but I didn't do so
here.
- if the client sends a bogus "object-format" with no equals sign,
we'll now say so, rather than "unknown object format: ''"
Signed-off-by: Jeff King <peff@peff.net>
---
serve.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/serve.c b/serve.c
index a161189984..f6ea2953eb 100644
--- a/serve.c
+++ b/serve.c
@@ -10,6 +10,7 @@
#include "upload-pack.h"
static int advertise_sid = -1;
+static int client_hash_algo = GIT_HASH_SHA1;
static int always_advertise(struct repository *r,
struct strbuf *value)
@@ -33,6 +34,17 @@ static int object_format_advertise(struct repository *r,
return 1;
}
+static void object_format_receive(struct repository *r,
+ const char *algo_name)
+{
+ if (!algo_name)
+ die("object-format capability requires an argument");
+
+ client_hash_algo = hash_algo_by_name(algo_name);
+ if (client_hash_algo == GIT_HASH_UNKNOWN)
+ die("unknown object format '%s'", algo_name);
+}
+
static int session_id_advertise(struct repository *r, struct strbuf *value)
{
if (advertise_sid == -1 &&
@@ -104,6 +116,7 @@ static struct protocol_capability capabilities[] = {
{
.name = "object-format",
.advertise = object_format_advertise,
+ .receive = object_format_receive,
},
{
.name = "session-id",
@@ -228,22 +241,6 @@ static int has_capability(const struct strvec *keys, const char *capability,
return 0;
}
-static void check_algorithm(struct repository *r, struct strvec *keys)
-{
- int client = GIT_HASH_SHA1, server = hash_algo_by_ptr(r->hash_algo);
- const char *algo_name;
-
- if (has_capability(keys, "object-format", &algo_name)) {
- client = hash_algo_by_name(algo_name);
- if (client == GIT_HASH_UNKNOWN)
- die("unknown object format '%s'", algo_name);
- }
-
- if (client != server)
- die("mismatched object format: server %s; client %s\n",
- r->hash_algo->name, hash_algos[client].name);
-}
-
enum request_state {
PROCESS_REQUEST_KEYS,
PROCESS_REQUEST_DONE,
@@ -317,7 +314,10 @@ static int process_request(void)
if (!command)
die("no command requested");
- check_algorithm(the_repository, &keys);
+ if (client_hash_algo != hash_algo_by_ptr(the_repository->hash_algo))
+ die("mismatched object format: server %s; client %s\n",
+ the_repository->hash_algo->name,
+ hash_algos[client_hash_algo].name);
if (has_capability(&keys, "session-id", &client_sid))
trace2_data_string("transfer", NULL, "client-sid", client_sid);
--
2.33.0.917.gae6ecbedc7
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2 04/11] serve: provide "receive" function for object-format capability
2021-09-14 23:51 ` [PATCH v2 04/11] serve: provide "receive" function for object-format capability Jeff King
@ 2021-09-15 16:54 ` Junio C Hamano
0 siblings, 0 replies; 77+ messages in thread
From: Junio C Hamano @ 2021-09-15 16:54 UTC (permalink / raw)
To: Jeff King
Cc: git, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
Jeff King <peff@peff.net> writes:
> There should be no change of behavior here, except for two
> broken-protocol cases:
>
> - if the client sends multiple conflicting object-format capabilities
> (which they should not), we'll now choose the last one rather than
> the first. We could also detect and complain about the duplicates
> quite easily now, which we could not before, but I didn't do so
> here.
I'd imagine that it would have been easy to retain the "use the
first one" behaviour if we wanted to; I do not think this behaviour
change really matters in practice, so it's OK.
> - if the client sends a bogus "object-format" with no equals sign,
> we'll now say so, rather than "unknown object format: ''"
OK.
> @@ -228,22 +241,6 @@ static int has_capability(const struct strvec *keys, const char *capability,
> return 0;
> }
>
> -static void check_algorithm(struct repository *r, struct strvec *keys)
> -{
> - int client = GIT_HASH_SHA1, server = hash_algo_by_ptr(r->hash_algo);
> - const char *algo_name;
> -
> - if (has_capability(keys, "object-format", &algo_name)) {
> - client = hash_algo_by_name(algo_name);
> - if (client == GIT_HASH_UNKNOWN)
> - die("unknown object format '%s'", algo_name);
> - }
> -
> - if (client != server)
> - die("mismatched object format: server %s; client %s\n",
> - r->hash_algo->name, hash_algos[client].name);
> -}
> -
> enum request_state {
> PROCESS_REQUEST_KEYS,
> PROCESS_REQUEST_DONE,
> @@ -317,7 +314,10 @@ static int process_request(void)
> if (!command)
> die("no command requested");
>
> - check_algorithm(the_repository, &keys);
> + if (client_hash_algo != hash_algo_by_ptr(the_repository->hash_algo))
> + die("mismatched object format: server %s; client %s\n",
> + the_repository->hash_algo->name,
> + hash_algos[client_hash_algo].name);
OK.
^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH v2 05/11] serve: provide "receive" function for session-id capability
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
` (3 preceding siblings ...)
2021-09-14 23:51 ` [PATCH v2 04/11] serve: provide "receive" function for object-format capability Jeff King
@ 2021-09-14 23:51 ` Jeff King
2021-09-15 16:56 ` Junio C Hamano
2021-09-14 23:51 ` [PATCH v2 06/11] serve: drop "keys" strvec Jeff King
` (7 subsequent siblings)
12 siblings, 1 reply; 77+ messages in thread
From: Jeff King @ 2021-09-14 23:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
Rather than pulling the session-id string from the list of collected
capabilities, we can handle it as soon as we receive it. This gets us
closer to dropping the collected list entirely.
The behavior should be the same, with one exception. Previously if the
client sent us multiple session-id lines, we'd report only the first.
Now we'll pass each one along to trace2. This shouldn't matter in
practice, since clients shouldn't do that (and if they do, it's probably
sensible to log them all).
As this removes the last caller of the static has_capability(), we can
remove it, as well (and in fact we must to avoid -Wunused-function
complaining).
Signed-off-by: Jeff King <peff@peff.net>
---
serve.c | 33 +++++++++------------------------
1 file changed, 9 insertions(+), 24 deletions(-)
diff --git a/serve.c b/serve.c
index f6ea2953eb..6bbf54cbbe 100644
--- a/serve.c
+++ b/serve.c
@@ -57,6 +57,14 @@ static int session_id_advertise(struct repository *r, struct strbuf *value)
return 1;
}
+static void session_id_receive(struct repository *r,
+ const char *client_sid)
+{
+ if (!client_sid)
+ client_sid = "";
+ trace2_data_string("transfer", NULL, "client-sid", client_sid);
+}
+
struct protocol_capability {
/*
* The name of the capability. The server uses this name when
@@ -121,6 +129,7 @@ static struct protocol_capability capabilities[] = {
{
.name = "session-id",
.advertise = session_id_advertise,
+ .receive = session_id_receive,
},
{
.name = "object-info",
@@ -221,26 +230,6 @@ static int parse_command(const char *key, struct protocol_capability **command)
return 0;
}
-static int has_capability(const struct strvec *keys, const char *capability,
- const char **value)
-{
- int i;
- for (i = 0; i < keys->nr; i++) {
- const char *out;
- if (skip_prefix(keys->v[i], capability, &out) &&
- (!*out || *out == '=')) {
- if (value) {
- if (*out == '=')
- out++;
- *value = out;
- }
- return 1;
- }
- }
-
- return 0;
-}
-
enum request_state {
PROCESS_REQUEST_KEYS,
PROCESS_REQUEST_DONE,
@@ -252,7 +241,6 @@ static int process_request(void)
struct packet_reader reader;
struct strvec keys = STRVEC_INIT;
struct protocol_capability *command = NULL;
- const char *client_sid;
packet_reader_init(&reader, 0, NULL, 0,
PACKET_READ_CHOMP_NEWLINE |
@@ -319,9 +307,6 @@ static int process_request(void)
the_repository->hash_algo->name,
hash_algos[client_hash_algo].name);
- if (has_capability(&keys, "session-id", &client_sid))
- trace2_data_string("transfer", NULL, "client-sid", client_sid);
-
command->command(the_repository, &reader);
strvec_clear(&keys);
--
2.33.0.917.gae6ecbedc7
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2 05/11] serve: provide "receive" function for session-id capability
2021-09-14 23:51 ` [PATCH v2 05/11] serve: provide "receive" function for session-id capability Jeff King
@ 2021-09-15 16:56 ` Junio C Hamano
0 siblings, 0 replies; 77+ messages in thread
From: Junio C Hamano @ 2021-09-15 16:56 UTC (permalink / raw)
To: Jeff King
Cc: git, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
Jeff King <peff@peff.net> writes:
> Rather than pulling the session-id string from the list of collected
> capabilities, we can handle it as soon as we receive it. This gets us
> closer to dropping the collected list entirely.
>
> The behavior should be the same, with one exception. Previously if the
> client sent us multiple session-id lines, we'd report only the first.
> Now we'll pass each one along to trace2. This shouldn't matter in
> practice, since clients shouldn't do that (and if they do, it's probably
> sensible to log them all).
>
> As this removes the last caller of the static has_capability(), we can
> remove it, as well (and in fact we must to avoid -Wunused-function
> complaining).
Nice.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> serve.c | 33 +++++++++------------------------
> 1 file changed, 9 insertions(+), 24 deletions(-)
>
> diff --git a/serve.c b/serve.c
> index f6ea2953eb..6bbf54cbbe 100644
> --- a/serve.c
> +++ b/serve.c
> @@ -57,6 +57,14 @@ static int session_id_advertise(struct repository *r, struct strbuf *value)
> return 1;
> }
>
> +static void session_id_receive(struct repository *r,
> + const char *client_sid)
> +{
> + if (!client_sid)
> + client_sid = "";
> + trace2_data_string("transfer", NULL, "client-sid", client_sid);
> +}
> +
> struct protocol_capability {
> /*
> * The name of the capability. The server uses this name when
> @@ -121,6 +129,7 @@ static struct protocol_capability capabilities[] = {
> {
> .name = "session-id",
> .advertise = session_id_advertise,
> + .receive = session_id_receive,
> },
> {
> .name = "object-info",
> @@ -221,26 +230,6 @@ static int parse_command(const char *key, struct protocol_capability **command)
> return 0;
> }
>
> -static int has_capability(const struct strvec *keys, const char *capability,
> - const char **value)
> -{
> - int i;
> - for (i = 0; i < keys->nr; i++) {
> - const char *out;
> - if (skip_prefix(keys->v[i], capability, &out) &&
> - (!*out || *out == '=')) {
> - if (value) {
> - if (*out == '=')
> - out++;
> - *value = out;
> - }
> - return 1;
> - }
> - }
> -
> - return 0;
> -}
> -
> enum request_state {
> PROCESS_REQUEST_KEYS,
> PROCESS_REQUEST_DONE,
> @@ -252,7 +241,6 @@ static int process_request(void)
> struct packet_reader reader;
> struct strvec keys = STRVEC_INIT;
> struct protocol_capability *command = NULL;
> - const char *client_sid;
>
> packet_reader_init(&reader, 0, NULL, 0,
> PACKET_READ_CHOMP_NEWLINE |
> @@ -319,9 +307,6 @@ static int process_request(void)
> the_repository->hash_algo->name,
> hash_algos[client_hash_algo].name);
>
> - if (has_capability(&keys, "session-id", &client_sid))
> - trace2_data_string("transfer", NULL, "client-sid", client_sid);
> -
> command->command(the_repository, &reader);
>
> strvec_clear(&keys);
^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH v2 06/11] serve: drop "keys" strvec
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
` (4 preceding siblings ...)
2021-09-14 23:51 ` [PATCH v2 05/11] serve: provide "receive" function for session-id capability Jeff King
@ 2021-09-14 23:51 ` Jeff King
2021-09-15 17:01 ` Junio C Hamano
2021-09-14 23:51 ` [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts Jeff King
` (6 subsequent siblings)
12 siblings, 1 reply; 77+ messages in thread
From: Jeff King @ 2021-09-14 23:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
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 | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/serve.c b/serve.c
index 6bbf54cbbe..5ea6c915cb 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,7 +263,7 @@ 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);
@@ -275,7 +275,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 +309,6 @@ static int process_request(void)
command->command(the_repository, &reader);
- strvec_clear(&keys);
return 0;
}
--
2.33.0.917.gae6ecbedc7
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2 06/11] serve: drop "keys" strvec
2021-09-14 23:51 ` [PATCH v2 06/11] serve: drop "keys" strvec Jeff King
@ 2021-09-15 17:01 ` Junio C Hamano
0 siblings, 0 replies; 77+ messages in thread
From: Junio C Hamano @ 2021-09-15 17:01 UTC (permalink / raw)
To: Jeff King
Cc: git, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
Jeff King <peff@peff.net> writes:
> 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 | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/serve.c b/serve.c
> index 6bbf54cbbe..5ea6c915cb 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,7 +263,7 @@ 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;
OK, we no longer "collect" request in the keys strvec, but I guess
what receive_client_capability() does still counts as "collecting",
so the "tentatively stale" comment is not wrong after all at the end
(we have tentatively been collecting in two different places and one
of them is dropped here).
> @@ -275,7 +275,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 +309,6 @@ static int process_request(void)
>
> command->command(the_repository, &reader);
>
> - strvec_clear(&keys);
> return 0;
> }
Nice.
^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
` (5 preceding siblings ...)
2021-09-14 23:51 ` [PATCH v2 06/11] serve: drop "keys" strvec Jeff King
@ 2021-09-14 23:51 ` Jeff King
2021-09-15 4:16 ` Taylor Blau
2021-09-15 5:00 ` Eric Sunshine
2021-09-14 23:52 ` [PATCH v2 08/11] docs/protocol-v2: clarify some ls-refs ref-prefix details Jeff King
` (5 subsequent siblings)
12 siblings, 2 replies; 77+ messages in thread
From: Jeff King @ 2021-09-14 23:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
Because each "ref-prefix" capability from the client comes in its own
pkt-line, there's no limit to the number of them that a misbehaving
client may send. We read them all into a strvec, which means the client
can waste arbitrary amounts of our memory by just sending us "ref-prefix
foo" over and over.
One possible solution is to just drop the connection when the limit is
reached. If we set it high enough, then only misbehaving or malicious
clients would hit it. But "high enough" is vague, and it's unfriendly if
we guess wrong and a legitimate client hits this.
But we can do better. Since supporting the ref-prefix capability is
optional anyway, the client has to further cull the response based on
their own patterns. So we can simply ignore the patterns once we cross a
certain threshold. Note that we have to ignore _all_ patterns, not just
the ones past our limit (since otherwise we'd send too little data).
The limit here is fairly arbitrary, and probably much higher than anyone
would need in practice. It might be worth limiting it further, if only
because we check it linearly (so with "m" local refs and "n" patterns,
we do "m * n" string comparisons). But if we care about optimizing this,
an even better solution may be a more advanced data structure anyway.
I didn't bother making the limit configurable, since it's so high and
since Git should behave correctly in either case. It wouldn't be too
hard to do, but it makes both the code and documentation more complex.
Signed-off-by: Jeff King <peff@peff.net>
---
ls-refs.c | 20 ++++++++++++++++++--
t/t5701-git-serve.sh | 31 +++++++++++++++++++++++++++++++
2 files changed, 49 insertions(+), 2 deletions(-)
diff --git a/ls-refs.c b/ls-refs.c
index a1a0250607..18c4f41e87 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -40,6 +40,12 @@ static void ensure_config_read(void)
config_read = 1;
}
+/*
+ * If we see this many or more "ref-prefix" lines from the client, we consider
+ * it "too many" and will avoid using the prefix feature entirely.
+ */
+#define TOO_MANY_PREFIXES 65536
+
/*
* Check if one of the prefixes is a prefix of the ref.
* If no prefixes were provided, all refs match.
@@ -156,15 +162,25 @@ int ls_refs(struct repository *r, struct packet_reader *request)
data.peel = 1;
else if (!strcmp("symrefs", arg))
data.symrefs = 1;
- else if (skip_prefix(arg, "ref-prefix ", &out))
- strvec_push(&data.prefixes, out);
+ else if (skip_prefix(arg, "ref-prefix ", &out)) {
+ if (data.prefixes.nr < TOO_MANY_PREFIXES)
+ strvec_push(&data.prefixes, out);
+ }
else if (!strcmp("unborn", arg))
data.unborn = allow_unborn;
}
if (request->status != PACKET_READ_FLUSH)
die(_("expected flush after ls-refs arguments"));
+ /*
+ * If we saw too many prefixes, we must avoid using them at all; as
+ * soon as we have any prefix, they are meant to form a comprehensive
+ * list.
+ */
+ if (data.prefixes.nr >= TOO_MANY_PREFIXES)
+ strvec_clear(&data.prefixes);
+
send_possibly_unborn_head(&data);
if (!data.prefixes.nr)
strvec_push(&data.prefixes, "");
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index 930721f053..3bc96ebcde 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -158,6 +158,37 @@ test_expect_success 'refs/heads prefix' '
test_cmp expect actual
'
+test_expect_success 'ignore very large set of prefixes' '
+ # generate a large number of ref-prefixes that we expect
+ # to match nothing; the value here exceeds TOO_MANY_PREFIXES
+ # from ls-refs.c.
+ {
+ echo command=ls-refs &&
+ echo object-format=$(test_oid algo)
+ echo 0001 &&
+ perl -le "print \"ref-prefix refs/heads/\$_\" for (1..65536)" &&
+ echo 0000
+ } |
+ test-tool pkt-line pack >in &&
+
+ # and then confirm that we see unmatched prefixes anyway (i.e.,
+ # that the prefix was not applied).
+ cat >expect <<-EOF &&
+ $(git rev-parse HEAD) HEAD
+ $(git rev-parse refs/heads/dev) refs/heads/dev
+ $(git rev-parse refs/heads/main) refs/heads/main
+ $(git rev-parse refs/heads/release) refs/heads/release
+ $(git rev-parse refs/tags/annotated-tag) refs/tags/annotated-tag
+ $(git rev-parse refs/tags/one) refs/tags/one
+ $(git rev-parse refs/tags/two) refs/tags/two
+ 0000
+ EOF
+
+ test-tool serve-v2 --stateless-rpc <in >out &&
+ test-tool pkt-line unpack <out >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'peel parameter' '
test-tool pkt-line pack >in <<-EOF &&
command=ls-refs
--
2.33.0.917.gae6ecbedc7
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts
2021-09-14 23:51 ` [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts Jeff King
@ 2021-09-15 4:16 ` Taylor Blau
2021-09-15 16:39 ` Jeff King
2021-09-15 5:00 ` Eric Sunshine
1 sibling, 1 reply; 77+ messages in thread
From: Taylor Blau @ 2021-09-15 4:16 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
On Tue, Sep 14, 2021 at 07:51:35PM -0400, Jeff King wrote:
> @@ -156,15 +162,25 @@ int ls_refs(struct repository *r, struct packet_reader *request)
> data.peel = 1;
> else if (!strcmp("symrefs", arg))
> data.symrefs = 1;
> - else if (skip_prefix(arg, "ref-prefix ", &out))
> - strvec_push(&data.prefixes, out);
> + else if (skip_prefix(arg, "ref-prefix ", &out)) {
> + if (data.prefixes.nr < TOO_MANY_PREFIXES)
> + strvec_push(&data.prefixes, out);
> + }
> else if (!strcmp("unborn", arg))
> data.unborn = allow_unborn;
> }
>
> if (request->status != PACKET_READ_FLUSH)
> die(_("expected flush after ls-refs arguments"));
>
> + /*
> + * If we saw too many prefixes, we must avoid using them at all; as
> + * soon as we have any prefix, they are meant to form a comprehensive
> + * list.
> + */
> + if (data.prefixes.nr >= TOO_MANY_PREFIXES)
> + strvec_clear(&data.prefixes);
> +
Great, I find this version even better than what I suggested where the
case where the list already has too many prefixes `continue`s through
the loop before calling strvec_add().
Just noting aloud, the `>` part of this conditional will never happen
(since data.prefixes.nr will be at most equal to TOO_MANY_PREFIXES, but
never larger than it).
Obviously not wrong, but I figured it'd be worth mentioning in case it
caught the attention of other reviewers.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts
2021-09-15 4:16 ` Taylor Blau
@ 2021-09-15 16:39 ` Jeff King
0 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 16:39 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Junio C Hamano, Martin Ågren,
Ævar Arnfjörð Bjarmason
On Wed, Sep 15, 2021 at 12:16:04AM -0400, Taylor Blau wrote:
> > + /*
> > + * If we saw too many prefixes, we must avoid using them at all; as
> > + * soon as we have any prefix, they are meant to form a comprehensive
> > + * list.
> > + */
> > + if (data.prefixes.nr >= TOO_MANY_PREFIXES)
> > + strvec_clear(&data.prefixes);
> > +
>
> Great, I find this version even better than what I suggested where the
> case where the list already has too many prefixes `continue`s through
> the loop before calling strvec_add().
>
> Just noting aloud, the `>` part of this conditional will never happen
> (since data.prefixes.nr will be at most equal to TOO_MANY_PREFIXES, but
> never larger than it).
>
> Obviously not wrong, but I figured it'd be worth mentioning in case it
> caught the attention of other reviewers.
Yeah, your analysis is right. Long ago I read some advice (I think from
K&R, either the C book or a related article) that suggested always using
bounding checks like this rather than equality, because they do the
right thing even if the variable ends up with a surprising value.
I can certainly see an argument against it (like that if you're so
worried about it, maybe you ought to detect it and figure out who is
setting the variable to be unexpectedly high). But it seems like a
reasonable defensive measure to me (against an off-by-one mis-analysis,
or against the earlier code changing in the future).
-Peff
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts
2021-09-14 23:51 ` [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts Jeff King
2021-09-15 4:16 ` Taylor Blau
@ 2021-09-15 5:00 ` Eric Sunshine
2021-09-15 16:40 ` Jeff King
1 sibling, 1 reply; 77+ messages in thread
From: Eric Sunshine @ 2021-09-15 5:00 UTC (permalink / raw)
To: Jeff King
Cc: Git List, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
On Tue, Sep 14, 2021 at 7:51 PM Jeff King <peff@peff.net> wrote:
> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
> @@ -158,6 +158,37 @@ test_expect_success 'refs/heads prefix' '
> +test_expect_success 'ignore very large set of prefixes' '
> + # generate a large number of ref-prefixes that we expect
> + # to match nothing; the value here exceeds TOO_MANY_PREFIXES
> + # from ls-refs.c.
> + {
> + echo command=ls-refs &&
> + echo object-format=$(test_oid algo)
> + echo 0001 &&
> + perl -le "print \"ref-prefix refs/heads/\$_\" for (1..65536)" &&
> + echo 0000
> + } |
> + test-tool pkt-line pack >in &&
Broken &&-chain in {...}.
(Granted, it doesn't matter in this case since the exit code gets lost
down the pipe, but better to be consistent about it.)
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts
2021-09-15 5:00 ` Eric Sunshine
@ 2021-09-15 16:40 ` Jeff King
0 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 16:40 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
On Wed, Sep 15, 2021 at 01:00:01AM -0400, Eric Sunshine wrote:
> On Tue, Sep 14, 2021 at 7:51 PM Jeff King <peff@peff.net> wrote:
> > diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
> > @@ -158,6 +158,37 @@ test_expect_success 'refs/heads prefix' '
> > +test_expect_success 'ignore very large set of prefixes' '
> > + # generate a large number of ref-prefixes that we expect
> > + # to match nothing; the value here exceeds TOO_MANY_PREFIXES
> > + # from ls-refs.c.
> > + {
> > + echo command=ls-refs &&
> > + echo object-format=$(test_oid algo)
> > + echo 0001 &&
> > + perl -le "print \"ref-prefix refs/heads/\$_\" for (1..65536)" &&
> > + echo 0000
> > + } |
> > + test-tool pkt-line pack >in &&
>
> Broken &&-chain in {...}.
Thanks, will fix.
> (Granted, it doesn't matter in this case since the exit code gets lost
> down the pipe, but better to be consistent about it.)
Yep. I think what happened is that I started to convert this to a
here-doc to match the nearby tests, but then realized that expanding the
multi-line bit was weird (even more so when it was a shell loop with a
pipe, before I switched it to perl).
-Peff
^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH v2 08/11] docs/protocol-v2: clarify some ls-refs ref-prefix details
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
` (6 preceding siblings ...)
2021-09-14 23:51 ` [PATCH v2 07/11] ls-refs: ignore very long ref-prefix counts Jeff King
@ 2021-09-14 23:52 ` Jeff King
2021-09-14 23:52 ` [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo" Jeff King
` (4 subsequent siblings)
12 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-14 23:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
We've never documented the fact that a client can provide multiple
ref-prefix capabilities. Let's describe the behavior.
We also never discussed the "best effort" nature of the prefixes. The
client side of git.git has always treated them this way, filtering the
result with local patterns. And indeed any client must do this, because
the prefix patterns are not sufficient to express the usual refspecs
(and so for "foo" we ask for "refs/heads/foo", "refs/tags/foo", and so
on).
So this may be considered a change in the spec with respect to client
expectations / requirements, but it's mostly codifying existing
behavior.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/technical/protocol-v2.txt | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 213538f1d0..9347b2ad13 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -193,7 +193,11 @@ ls-refs takes in the following arguments:
Show peeled tags.
ref-prefix <prefix>
When specified, only references having a prefix matching one of
- the provided prefixes are displayed.
+ the provided prefixes are displayed. Multiple instances may be
+ given, in which case references matching any prefix will be
+ shown. Note that this is purely for optimization; a server MAY
+ show refs not matching the prefix if it chooses, and clients
+ should filter the result themselves.
If the 'unborn' feature is advertised the following argument can be
included in the client's request.
--
2.33.0.917.gae6ecbedc7
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo"
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
` (7 preceding siblings ...)
2021-09-14 23:52 ` [PATCH v2 08/11] docs/protocol-v2: clarify some ls-refs ref-prefix details Jeff King
@ 2021-09-14 23:52 ` Jeff King
2021-09-15 0:27 ` Ævar Arnfjörð Bjarmason
` (2 more replies)
2021-09-14 23:52 ` [PATCH v2 10/11] serve: reject commands used as capabilities Jeff King
` (3 subsequent siblings)
12 siblings, 3 replies; 77+ messages in thread
From: Jeff King @ 2021-09-14 23:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
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(-)
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
--
2.33.0.917.gae6ecbedc7
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo"
2021-09-14 23:52 ` [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo" Jeff King
@ 2021-09-15 0:27 ` Ævar Arnfjörð Bjarmason
2021-09-15 16:28 ` Jeff King
2021-09-15 5:09 ` Eric Sunshine
2021-09-15 17:33 ` Junio C Hamano
2 siblings, 1 reply; 77+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-15 0:27 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Taylor Blau, Martin Ågren
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
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo"
2021-09-15 0:27 ` Ævar Arnfjörð Bjarmason
@ 2021-09-15 16:28 ` Jeff King
0 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 16:28 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Taylor Blau, Martin Ågren
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo"
2021-09-14 23:52 ` [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo" Jeff King
2021-09-15 0:27 ` Ævar Arnfjörð Bjarmason
@ 2021-09-15 5:09 ` Eric Sunshine
2021-09-15 16:32 ` Jeff King
2021-09-15 17:33 ` Junio C Hamano
2 siblings, 1 reply; 77+ messages in thread
From: Eric Sunshine @ 2021-09-15 5:09 UTC (permalink / raw)
To: Jeff King
Cc: Git List, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
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
> +'
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo"
2021-09-15 5:09 ` Eric Sunshine
@ 2021-09-15 16:32 ` Jeff King
0 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 16:32 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo"
2021-09-14 23:52 ` [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo" Jeff King
2021-09-15 0:27 ` Ævar Arnfjörð Bjarmason
2021-09-15 5:09 ` Eric Sunshine
@ 2021-09-15 17:33 ` Junio C Hamano
2021-09-15 17:39 ` Jeff King
2 siblings, 1 reply; 77+ messages in thread
From: Junio C Hamano @ 2021-09-15 17:33 UTC (permalink / raw)
To: Jeff King
Cc: git, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
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.
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo"
2021-09-15 17:33 ` Junio C Hamano
@ 2021-09-15 17:39 ` Jeff King
0 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 17:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH v2 10/11] serve: reject commands used as capabilities
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
` (8 preceding siblings ...)
2021-09-14 23:52 ` [PATCH v2 09/11] serve: reject bogus v2 "command=ls-refs=foo" Jeff King
@ 2021-09-14 23:52 ` Jeff King
2021-09-14 23:54 ` [PATCH v2 11/11] ls-refs: reject unknown arguments Jeff King
` (2 subsequent siblings)
12 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-14 23:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
Our table of v2 "capabilities" contains everything we might tell the
client we support. But there are differences in how we expect the client
to respond. Some of the entries are true capabilities (i.e., we expect
the client to say "yes, I support this"), and some are ones we expect
them to send as commands (with "command=ls-refs" or similar).
When we receive a capability used as a command, we complain about that.
But when we receive a command used as a capability (e.g., just "ls-refs"
in a pkt-line by itself), we silently ignore it.
This isn't really hurting anything (clients shouldn't send it, and we'll
ignore it), but we can tighten up the protocol to match what we expect
to happen.
There are two new tests here. The first one checks a capability used as
a command, which already passes. The second tests a command as a
capability, which this patch fixes.
Signed-off-by: Jeff King <peff@peff.net>
---
serve.c | 2 +-
t/t5701-git-serve.sh | 19 +++++++++++++++++++
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/serve.c b/serve.c
index 63ee1be7ff..0636b79f92 100644
--- a/serve.c
+++ b/serve.c
@@ -201,7 +201,7 @@ static int receive_client_capability(const char *key)
const char *value;
const struct protocol_capability *c = get_capability(key, &value);
- if (!c || !c->advertise(the_repository, NULL))
+ if (!c || c->command || !c->advertise(the_repository, NULL))
return 0;
if (c->receive)
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index ab15078bc0..b027ba9b06 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -72,6 +72,25 @@ test_expect_success 'request invalid command' '
test_i18ngrep "invalid command" err
'
+test_expect_success 'request capability as command' '
+ test-tool pkt-line pack >in <<-\EOF &&
+ command=agent
+ 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 &&
+ command=ls-refs
+ fetch
+ 0000
+ EOF
+ test_must_fail test-tool serve-v2 --stateless-rpc 2>err <in &&
+ grep unknown.capability err
+'
+
test_expect_success 'requested command is command=value' '
test-tool pkt-line pack >in <<-\EOF &&
command=ls-refs=whatever
--
2.33.0.917.gae6ecbedc7
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v2 11/11] ls-refs: reject unknown arguments
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
` (9 preceding siblings ...)
2021-09-14 23:52 ` [PATCH v2 10/11] serve: reject commands used as capabilities Jeff King
@ 2021-09-14 23:54 ` Jeff King
2021-09-15 0:09 ` Ævar Arnfjörð Bjarmason
2021-09-15 4:17 ` [PATCH v2 0/11] limit memory allocations for v2 servers Taylor Blau
2021-09-15 18:33 ` Jeff King
12 siblings, 1 reply; 77+ messages in thread
From: Jeff King @ 2021-09-14 23:54 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
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
--
2.33.0.917.gae6ecbedc7
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2 11/11] ls-refs: reject unknown arguments
2021-09-14 23:54 ` [PATCH v2 11/11] ls-refs: reject unknown arguments Jeff King
@ 2021-09-15 0:09 ` Ævar Arnfjörð Bjarmason
2021-09-15 16:25 ` Jeff King
0 siblings, 1 reply; 77+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-15 0:09 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Taylor Blau, Martin Ågren
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 &&
--
2.33.0.1013.ge8323766266
^ permalink raw reply related [flat|nested] 77+ messages in thread* Re: [PATCH v2 11/11] ls-refs: reject unknown arguments
2021-09-15 0:09 ` Ævar Arnfjörð Bjarmason
@ 2021-09-15 16:25 ` Jeff King
0 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 16:25 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Taylor Blau, Martin Ågren
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
^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH v2 0/11] limit memory allocations for v2 servers
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
` (10 preceding siblings ...)
2021-09-14 23:54 ` [PATCH v2 11/11] ls-refs: reject unknown arguments Jeff King
@ 2021-09-15 4:17 ` Taylor Blau
2021-09-15 18:33 ` Jeff King
12 siblings, 0 replies; 77+ messages in thread
From: Taylor Blau @ 2021-09-15 4:17 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
On Tue, Sep 14, 2021 at 07:51:06PM -0400, Jeff King wrote:
> Here's a re-roll of my series to limit the memory a v2 server is willing
> to allocate on behalf of a client. See v1:
>
> https://lore.kernel.org/git/YUC%2F6n1hhUbMJiLw@coredump.intra.peff.net/
>
> for an overview. The existing patches are mostly small fixups pointed
> out by reviewers (thanks!), but I did take Martin's TOO_MANY_PREFIXES
> suggestion in patch 7 (without the change to the name of the constant it
> seemed too clever to me, but with it it seems just the right amount of
> clever).
Thanks for this. The two new patches look good to me, and I took a light
skim over the ones which were modified since v1. Everything I saw seemed
very reasonable to me.
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Thanks,
Taylor
^ permalink raw reply [flat|nested] 77+ messages in thread* Re: [PATCH v2 0/11] limit memory allocations for v2 servers
2021-09-14 23:51 ` [PATCH v2 0/11] limit " Jeff King
` (11 preceding siblings ...)
2021-09-15 4:17 ` [PATCH v2 0/11] limit memory allocations for v2 servers Taylor Blau
@ 2021-09-15 18:33 ` Jeff King
2021-09-15 18:34 ` [PATCH v3 " Jeff King
12 siblings, 1 reply; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:33 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 77+ messages in thread* [PATCH v3 0/11] limit memory allocations for v2 servers
2021-09-15 18:33 ` Jeff King
@ 2021-09-15 18:34 ` Jeff King
2021-09-15 18:35 ` [PATCH v3 01/11] serve: rename is_command() to parse_command() Jeff King
` (10 more replies)
0 siblings, 11 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:34 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
On Wed, Sep 15, 2021 at 02:33:53PM -0400, Jeff King wrote:
> 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.
Er, that should have been "v3" in the subject, as this one now is.
-Peff
^ permalink raw reply [flat|nested] 77+ messages in thread* [PATCH v3 01/11] serve: rename is_command() to parse_command()
2021-09-15 18:34 ` [PATCH v3 " Jeff King
@ 2021-09-15 18:35 ` Jeff King
2021-09-15 18:35 ` [PATCH v3 02/11] serve: return capability "value" from get_capability() Jeff King
` (9 subsequent siblings)
10 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:35 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
The is_command() function not only tells us whether the pktline is a
valid command string, but it also parses out the command (and complains
if we see a duplicate). Let's rename it to make those extra functions
a bit more obvious.
Signed-off-by: Jeff King <peff@peff.net>
---
serve.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/serve.c b/serve.c
index 1817edc7f5..fd88b95343 100644
--- a/serve.c
+++ b/serve.c
@@ -163,7 +163,7 @@ static int is_valid_capability(const char *key)
return c && c->advertise(the_repository, NULL);
}
-static int is_command(const char *key, struct protocol_capability **command)
+static int parse_command(const char *key, struct protocol_capability **command)
{
const char *out;
@@ -251,7 +251,7 @@ static int process_request(void)
BUG("Should have already died when seeing EOF");
case PACKET_READ_NORMAL:
/* collect request; a sequence of keys and values */
- if (is_command(reader.line, &command) ||
+ if (parse_command(reader.line, &command) ||
is_valid_capability(reader.line))
strvec_push(&keys, reader.line);
else
--
2.33.0.917.g33ebf6a5f6
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v3 02/11] serve: return capability "value" from get_capability()
2021-09-15 18:34 ` [PATCH v3 " Jeff King
2021-09-15 18:35 ` [PATCH v3 01/11] serve: rename is_command() to parse_command() Jeff King
@ 2021-09-15 18:35 ` Jeff King
2021-09-15 18:35 ` [PATCH v3 03/11] serve: add "receive" method for v2 capabilities table Jeff King
` (8 subsequent siblings)
10 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:35 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
When the client sends v2 capabilities, they may be simple, like:
foo
or have a value like:
foo=bar
(all of the current capabilities actually expect a value, but the
protocol allows for boolean ones).
We use get_capability() to make sure the client's pktline matches a
capability. In doing so, we parse enough to see the "=" and the value
(if any), but we immediately forget it. Nobody cares for now, because they end
up parsing the values out later using has_capability(). But in
preparation for changing that, let's pass back a pointer so the callers
know what we found.
Note that unlike has_capability(), we'll return NULL for a "simple"
capability. Distinguishing these will be useful for some future patches.
Signed-off-by: Jeff King <peff@peff.net>
---
serve.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/serve.c b/serve.c
index fd88b95343..78a4e83554 100644
--- a/serve.c
+++ b/serve.c
@@ -139,7 +139,7 @@ void protocol_v2_advertise_capabilities(void)
strbuf_release(&value);
}
-static struct protocol_capability *get_capability(const char *key)
+static struct protocol_capability *get_capability(const char *key, const char **value)
{
int i;
@@ -149,16 +149,25 @@ static struct protocol_capability *get_capability(const char *key)
for (i = 0; i < ARRAY_SIZE(capabilities); i++) {
struct protocol_capability *c = &capabilities[i];
const char *out;
- if (skip_prefix(key, c->name, &out) && (!*out || *out == '='))
+ if (!skip_prefix(key, c->name, &out))
+ continue;
+ if (!*out) {
+ *value = NULL;
return c;
+ }
+ if (*out++ == '=') {
+ *value = out;
+ return c;
+ }
}
return NULL;
}
static int is_valid_capability(const char *key)
{
- const struct protocol_capability *c = get_capability(key);
+ const char *value;
+ const struct protocol_capability *c = get_capability(key, &value);
return c && c->advertise(the_repository, NULL);
}
@@ -168,7 +177,8 @@ static int parse_command(const char *key, struct protocol_capability **command)
const char *out;
if (skip_prefix(key, "command=", &out)) {
- struct protocol_capability *cmd = get_capability(out);
+ const char *value;
+ struct protocol_capability *cmd = get_capability(out, &value);
if (*command)
die("command '%s' requested after already requesting command '%s'",
--
2.33.0.917.g33ebf6a5f6
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v3 03/11] serve: add "receive" method for v2 capabilities table
2021-09-15 18:34 ` [PATCH v3 " Jeff King
2021-09-15 18:35 ` [PATCH v3 01/11] serve: rename is_command() to parse_command() Jeff King
2021-09-15 18:35 ` [PATCH v3 02/11] serve: return capability "value" from get_capability() Jeff King
@ 2021-09-15 18:35 ` Jeff King
2021-09-15 18:35 ` [PATCH v3 04/11] serve: provide "receive" function for object-format capability Jeff King
` (7 subsequent siblings)
10 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:35 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
We have a capabilities table that tells us what we should tell the
client we are capable of, and what to do when a client gives us a
particular command (e.g., "command=ls-refs"). But it doesn't tell us
what to do when the client sends us back a capability (e.g.,
"object-format=sha256"). We just collect them all in a strvec and hope
somebody can use them later.
Instead, let's provide a function pointer in the table to act on these.
This will eventually help us avoid collecting the strings, which will be
more efficient and less prone to mischief.
Using the new method is optional, which helps in two ways:
- we can move existing capabilities over to this new system gradually
in individual commits
- some capabilities we don't actually do anything with anyway. For
example, the client is free to say "agent=git/1.2.3" to us, but we
do not act on the information in any way.
Signed-off-by: Jeff King <peff@peff.net>
---
serve.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/serve.c b/serve.c
index 78a4e83554..a161189984 100644
--- a/serve.c
+++ b/serve.c
@@ -70,6 +70,16 @@ struct protocol_capability {
* This field should be NULL for capabilities which are not commands.
*/
int (*command)(struct repository *r, struct packet_reader *request);
+
+ /*
+ * Function called when a client requests the capability as a
+ * non-command. This may be NULL if the capability does nothing.
+ *
+ * For a capability of the form "foo=bar", the value string points to
+ * the content after the "=" (i.e., "bar"). For simple capabilities
+ * (just "foo"), it is NULL.
+ */
+ void (*receive)(struct repository *r, const char *value);
};
static struct protocol_capability capabilities[] = {
@@ -164,12 +174,17 @@ static struct protocol_capability *get_capability(const char *key, const char **
return NULL;
}
-static int is_valid_capability(const char *key)
+static int receive_client_capability(const char *key)
{
const char *value;
const struct protocol_capability *c = get_capability(key, &value);
- return c && c->advertise(the_repository, NULL);
+ if (!c || !c->advertise(the_repository, NULL))
+ return 0;
+
+ if (c->receive)
+ c->receive(the_repository, value);
+ return 1;
}
static int parse_command(const char *key, struct protocol_capability **command)
@@ -262,7 +277,7 @@ static int process_request(void)
case PACKET_READ_NORMAL:
/* collect request; a sequence of keys and values */
if (parse_command(reader.line, &command) ||
- is_valid_capability(reader.line))
+ receive_client_capability(reader.line))
strvec_push(&keys, reader.line);
else
die("unknown capability '%s'", reader.line);
--
2.33.0.917.g33ebf6a5f6
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v3 04/11] serve: provide "receive" function for object-format capability
2021-09-15 18:34 ` [PATCH v3 " Jeff King
` (2 preceding siblings ...)
2021-09-15 18:35 ` [PATCH v3 03/11] serve: add "receive" method for v2 capabilities table Jeff King
@ 2021-09-15 18:35 ` Jeff King
2021-09-15 18:35 ` [PATCH v3 05/11] serve: provide "receive" function for session-id capability Jeff King
` (6 subsequent siblings)
10 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:35 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
We get any "object-format" specified by the client by searching for it
in the collected list of capabilities the client sent. We can instead
just handle it as soon as they send it. This is slightly more efficient,
and gets us one step closer to dropping that collected list.
Note that we do still have to do our final hash check after receiving
all capabilities (because they might not have sent an object-format line
at all, and we still have to check that the default matches our
repository algorithm). Since the check_algorithm() function would now be
down to a single if() statement, I've just inlined it in its only
caller.
There should be no change of behavior here, except for two
broken-protocol cases:
- if the client sends multiple conflicting object-format capabilities
(which they should not), we'll now choose the last one rather than
the first. We could also detect and complain about the duplicates
quite easily now, which we could not before, but I didn't do so
here.
- if the client sends a bogus "object-format" with no equals sign,
we'll now say so, rather than "unknown object format: ''"
Signed-off-by: Jeff King <peff@peff.net>
---
serve.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/serve.c b/serve.c
index a161189984..f6ea2953eb 100644
--- a/serve.c
+++ b/serve.c
@@ -10,6 +10,7 @@
#include "upload-pack.h"
static int advertise_sid = -1;
+static int client_hash_algo = GIT_HASH_SHA1;
static int always_advertise(struct repository *r,
struct strbuf *value)
@@ -33,6 +34,17 @@ static int object_format_advertise(struct repository *r,
return 1;
}
+static void object_format_receive(struct repository *r,
+ const char *algo_name)
+{
+ if (!algo_name)
+ die("object-format capability requires an argument");
+
+ client_hash_algo = hash_algo_by_name(algo_name);
+ if (client_hash_algo == GIT_HASH_UNKNOWN)
+ die("unknown object format '%s'", algo_name);
+}
+
static int session_id_advertise(struct repository *r, struct strbuf *value)
{
if (advertise_sid == -1 &&
@@ -104,6 +116,7 @@ static struct protocol_capability capabilities[] = {
{
.name = "object-format",
.advertise = object_format_advertise,
+ .receive = object_format_receive,
},
{
.name = "session-id",
@@ -228,22 +241,6 @@ static int has_capability(const struct strvec *keys, const char *capability,
return 0;
}
-static void check_algorithm(struct repository *r, struct strvec *keys)
-{
- int client = GIT_HASH_SHA1, server = hash_algo_by_ptr(r->hash_algo);
- const char *algo_name;
-
- if (has_capability(keys, "object-format", &algo_name)) {
- client = hash_algo_by_name(algo_name);
- if (client == GIT_HASH_UNKNOWN)
- die("unknown object format '%s'", algo_name);
- }
-
- if (client != server)
- die("mismatched object format: server %s; client %s\n",
- r->hash_algo->name, hash_algos[client].name);
-}
-
enum request_state {
PROCESS_REQUEST_KEYS,
PROCESS_REQUEST_DONE,
@@ -317,7 +314,10 @@ static int process_request(void)
if (!command)
die("no command requested");
- check_algorithm(the_repository, &keys);
+ if (client_hash_algo != hash_algo_by_ptr(the_repository->hash_algo))
+ die("mismatched object format: server %s; client %s\n",
+ the_repository->hash_algo->name,
+ hash_algos[client_hash_algo].name);
if (has_capability(&keys, "session-id", &client_sid))
trace2_data_string("transfer", NULL, "client-sid", client_sid);
--
2.33.0.917.g33ebf6a5f6
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v3 05/11] serve: provide "receive" function for session-id capability
2021-09-15 18:34 ` [PATCH v3 " Jeff King
` (3 preceding siblings ...)
2021-09-15 18:35 ` [PATCH v3 04/11] serve: provide "receive" function for object-format capability Jeff King
@ 2021-09-15 18:35 ` Jeff King
2021-09-15 18:35 ` [PATCH v3 06/11] serve: drop "keys" strvec Jeff King
` (5 subsequent siblings)
10 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:35 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
Rather than pulling the session-id string from the list of collected
capabilities, we can handle it as soon as we receive it. This gets us
closer to dropping the collected list entirely.
The behavior should be the same, with one exception. Previously if the
client sent us multiple session-id lines, we'd report only the first.
Now we'll pass each one along to trace2. This shouldn't matter in
practice, since clients shouldn't do that (and if they do, it's probably
sensible to log them all).
As this removes the last caller of the static has_capability(), we can
remove it, as well (and in fact we must to avoid -Wunused-function
complaining).
Signed-off-by: Jeff King <peff@peff.net>
---
serve.c | 33 +++++++++------------------------
1 file changed, 9 insertions(+), 24 deletions(-)
diff --git a/serve.c b/serve.c
index f6ea2953eb..6bbf54cbbe 100644
--- a/serve.c
+++ b/serve.c
@@ -57,6 +57,14 @@ static int session_id_advertise(struct repository *r, struct strbuf *value)
return 1;
}
+static void session_id_receive(struct repository *r,
+ const char *client_sid)
+{
+ if (!client_sid)
+ client_sid = "";
+ trace2_data_string("transfer", NULL, "client-sid", client_sid);
+}
+
struct protocol_capability {
/*
* The name of the capability. The server uses this name when
@@ -121,6 +129,7 @@ static struct protocol_capability capabilities[] = {
{
.name = "session-id",
.advertise = session_id_advertise,
+ .receive = session_id_receive,
},
{
.name = "object-info",
@@ -221,26 +230,6 @@ static int parse_command(const char *key, struct protocol_capability **command)
return 0;
}
-static int has_capability(const struct strvec *keys, const char *capability,
- const char **value)
-{
- int i;
- for (i = 0; i < keys->nr; i++) {
- const char *out;
- if (skip_prefix(keys->v[i], capability, &out) &&
- (!*out || *out == '=')) {
- if (value) {
- if (*out == '=')
- out++;
- *value = out;
- }
- return 1;
- }
- }
-
- return 0;
-}
-
enum request_state {
PROCESS_REQUEST_KEYS,
PROCESS_REQUEST_DONE,
@@ -252,7 +241,6 @@ static int process_request(void)
struct packet_reader reader;
struct strvec keys = STRVEC_INIT;
struct protocol_capability *command = NULL;
- const char *client_sid;
packet_reader_init(&reader, 0, NULL, 0,
PACKET_READ_CHOMP_NEWLINE |
@@ -319,9 +307,6 @@ static int process_request(void)
the_repository->hash_algo->name,
hash_algos[client_hash_algo].name);
- if (has_capability(&keys, "session-id", &client_sid))
- trace2_data_string("transfer", NULL, "client-sid", client_sid);
-
command->command(the_repository, &reader);
strvec_clear(&keys);
--
2.33.0.917.g33ebf6a5f6
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v3 06/11] serve: drop "keys" strvec
2021-09-15 18:34 ` [PATCH v3 " Jeff King
` (4 preceding siblings ...)
2021-09-15 18:35 ` [PATCH v3 05/11] serve: provide "receive" function for session-id capability Jeff King
@ 2021-09-15 18:35 ` Jeff King
2021-09-15 18:35 ` [PATCH v3 07/11] ls-refs: ignore very long ref-prefix counts Jeff King
` (4 subsequent siblings)
10 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:35 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
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, 3 insertions(+), 5 deletions(-)
diff --git a/serve.c b/serve.c
index 6bbf54cbbe..1a7c8a118f 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,
@@ -260,10 +260,9 @@ static int process_request(void)
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);
+ seen_capability_or_command = 1;
else
die("unknown capability '%s'", reader.line);
@@ -275,7 +274,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 +308,6 @@ static int process_request(void)
command->command(the_repository, &reader);
- strvec_clear(&keys);
return 0;
}
--
2.33.0.917.g33ebf6a5f6
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v3 07/11] ls-refs: ignore very long ref-prefix counts
2021-09-15 18:34 ` [PATCH v3 " Jeff King
` (5 preceding siblings ...)
2021-09-15 18:35 ` [PATCH v3 06/11] serve: drop "keys" strvec Jeff King
@ 2021-09-15 18:35 ` Jeff King
2021-09-15 18:35 ` [PATCH v3 08/11] docs/protocol-v2: clarify some ls-refs ref-prefix details Jeff King
` (3 subsequent siblings)
10 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:35 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
Because each "ref-prefix" capability from the client comes in its own
pkt-line, there's no limit to the number of them that a misbehaving
client may send. We read them all into a strvec, which means the client
can waste arbitrary amounts of our memory by just sending us "ref-prefix
foo" over and over.
One possible solution is to just drop the connection when the limit is
reached. If we set it high enough, then only misbehaving or malicious
clients would hit it. But "high enough" is vague, and it's unfriendly if
we guess wrong and a legitimate client hits this.
But we can do better. Since supporting the ref-prefix capability is
optional anyway, the client has to further cull the response based on
their own patterns. So we can simply ignore the patterns once we cross a
certain threshold. Note that we have to ignore _all_ patterns, not just
the ones past our limit (since otherwise we'd send too little data).
The limit here is fairly arbitrary, and probably much higher than anyone
would need in practice. It might be worth limiting it further, if only
because we check it linearly (so with "m" local refs and "n" patterns,
we do "m * n" string comparisons). But if we care about optimizing this,
an even better solution may be a more advanced data structure anyway.
I didn't bother making the limit configurable, since it's so high and
since Git should behave correctly in either case. It wouldn't be too
hard to do, but it makes both the code and documentation more complex.
Signed-off-by: Jeff King <peff@peff.net>
---
ls-refs.c | 20 ++++++++++++++++++--
t/t5701-git-serve.sh | 31 +++++++++++++++++++++++++++++++
2 files changed, 49 insertions(+), 2 deletions(-)
diff --git a/ls-refs.c b/ls-refs.c
index a1a0250607..18c4f41e87 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -40,6 +40,12 @@ static void ensure_config_read(void)
config_read = 1;
}
+/*
+ * If we see this many or more "ref-prefix" lines from the client, we consider
+ * it "too many" and will avoid using the prefix feature entirely.
+ */
+#define TOO_MANY_PREFIXES 65536
+
/*
* Check if one of the prefixes is a prefix of the ref.
* If no prefixes were provided, all refs match.
@@ -156,15 +162,25 @@ int ls_refs(struct repository *r, struct packet_reader *request)
data.peel = 1;
else if (!strcmp("symrefs", arg))
data.symrefs = 1;
- else if (skip_prefix(arg, "ref-prefix ", &out))
- strvec_push(&data.prefixes, out);
+ else if (skip_prefix(arg, "ref-prefix ", &out)) {
+ if (data.prefixes.nr < TOO_MANY_PREFIXES)
+ strvec_push(&data.prefixes, out);
+ }
else if (!strcmp("unborn", arg))
data.unborn = allow_unborn;
}
if (request->status != PACKET_READ_FLUSH)
die(_("expected flush after ls-refs arguments"));
+ /*
+ * If we saw too many prefixes, we must avoid using them at all; as
+ * soon as we have any prefix, they are meant to form a comprehensive
+ * list.
+ */
+ if (data.prefixes.nr >= TOO_MANY_PREFIXES)
+ strvec_clear(&data.prefixes);
+
send_possibly_unborn_head(&data);
if (!data.prefixes.nr)
strvec_push(&data.prefixes, "");
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index 930721f053..520672f842 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -158,6 +158,37 @@ test_expect_success 'refs/heads prefix' '
test_cmp expect actual
'
+test_expect_success 'ignore very large set of prefixes' '
+ # generate a large number of ref-prefixes that we expect
+ # to match nothing; the value here exceeds TOO_MANY_PREFIXES
+ # from ls-refs.c.
+ {
+ echo command=ls-refs &&
+ echo object-format=$(test_oid algo) &&
+ echo 0001 &&
+ perl -le "print \"ref-prefix refs/heads/\$_\" for (1..65536)" &&
+ echo 0000
+ } |
+ test-tool pkt-line pack >in &&
+
+ # and then confirm that we see unmatched prefixes anyway (i.e.,
+ # that the prefix was not applied).
+ cat >expect <<-EOF &&
+ $(git rev-parse HEAD) HEAD
+ $(git rev-parse refs/heads/dev) refs/heads/dev
+ $(git rev-parse refs/heads/main) refs/heads/main
+ $(git rev-parse refs/heads/release) refs/heads/release
+ $(git rev-parse refs/tags/annotated-tag) refs/tags/annotated-tag
+ $(git rev-parse refs/tags/one) refs/tags/one
+ $(git rev-parse refs/tags/two) refs/tags/two
+ 0000
+ EOF
+
+ test-tool serve-v2 --stateless-rpc <in >out &&
+ test-tool pkt-line unpack <out >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'peel parameter' '
test-tool pkt-line pack >in <<-EOF &&
command=ls-refs
--
2.33.0.917.g33ebf6a5f6
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v3 08/11] docs/protocol-v2: clarify some ls-refs ref-prefix details
2021-09-15 18:34 ` [PATCH v3 " Jeff King
` (6 preceding siblings ...)
2021-09-15 18:35 ` [PATCH v3 07/11] ls-refs: ignore very long ref-prefix counts Jeff King
@ 2021-09-15 18:35 ` Jeff King
2021-09-15 18:36 ` [PATCH v3 09/11] serve: reject bogus v2 "command=ls-refs=foo" Jeff King
` (2 subsequent siblings)
10 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:35 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
We've never documented the fact that a client can provide multiple
ref-prefix capabilities. Let's describe the behavior.
We also never discussed the "best effort" nature of the prefixes. The
client side of git.git has always treated them this way, filtering the
result with local patterns. And indeed any client must do this, because
the prefix patterns are not sufficient to express the usual refspecs
(and so for "foo" we ask for "refs/heads/foo", "refs/tags/foo", and so
on).
So this may be considered a change in the spec with respect to client
expectations / requirements, but it's mostly codifying existing
behavior.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/technical/protocol-v2.txt | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 213538f1d0..9347b2ad13 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -193,7 +193,11 @@ ls-refs takes in the following arguments:
Show peeled tags.
ref-prefix <prefix>
When specified, only references having a prefix matching one of
- the provided prefixes are displayed.
+ the provided prefixes are displayed. Multiple instances may be
+ given, in which case references matching any prefix will be
+ shown. Note that this is purely for optimization; a server MAY
+ show refs not matching the prefix if it chooses, and clients
+ should filter the result themselves.
If the 'unborn' feature is advertised the following argument can be
included in the client's request.
--
2.33.0.917.g33ebf6a5f6
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v3 09/11] serve: reject bogus v2 "command=ls-refs=foo"
2021-09-15 18:34 ` [PATCH v3 " Jeff King
` (7 preceding siblings ...)
2021-09-15 18:35 ` [PATCH v3 08/11] docs/protocol-v2: clarify some ls-refs ref-prefix details Jeff King
@ 2021-09-15 18:36 ` Jeff King
2021-09-15 18:36 ` [PATCH v3 10/11] serve: reject commands used as capabilities Jeff King
2021-09-15 18:36 ` [PATCH v3 11/11] ls-refs: reject unknown arguments Jeff King
10 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:36 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
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 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 need to 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>
---
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 1a7c8a118f..db5ecfed2d 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 520672f842..2e51886def 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
--
2.33.0.917.g33ebf6a5f6
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v3 10/11] serve: reject commands used as capabilities
2021-09-15 18:34 ` [PATCH v3 " Jeff King
` (8 preceding siblings ...)
2021-09-15 18:36 ` [PATCH v3 09/11] serve: reject bogus v2 "command=ls-refs=foo" Jeff King
@ 2021-09-15 18:36 ` Jeff King
2021-09-15 18:36 ` [PATCH v3 11/11] ls-refs: reject unknown arguments Jeff King
10 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:36 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
Our table of v2 "capabilities" contains everything we might tell the
client we support. But there are differences in how we expect the client
to respond. Some of the entries are true capabilities (i.e., we expect
the client to say "yes, I support this"), and some are ones we expect
them to send as commands (with "command=ls-refs" or similar).
When we receive a capability used as a command, we complain about that.
But when we receive a command used as a capability (e.g., just "ls-refs"
in a pkt-line by itself), we silently ignore it.
This isn't really hurting anything (clients shouldn't send it, and we'll
ignore it), but we can tighten up the protocol to match what we expect
to happen.
There are two new tests here. The first one checks a capability used as
a command, which already passes. The second tests a command as a
capability, which this patch fixes.
Signed-off-by: Jeff King <peff@peff.net>
---
serve.c | 2 +-
t/t5701-git-serve.sh | 21 +++++++++++++++++++++
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/serve.c b/serve.c
index db5ecfed2d..b3fe9b5126 100644
--- a/serve.c
+++ b/serve.c
@@ -201,7 +201,7 @@ static int receive_client_capability(const char *key)
const char *value;
const struct protocol_capability *c = get_capability(key, &value);
- if (!c || !c->advertise(the_repository, NULL))
+ if (!c || c->command || !c->advertise(the_repository, NULL))
return 0;
if (c->receive)
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index 2e51886def..3928424e1b 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -72,6 +72,27 @@ test_expect_success 'request invalid command' '
test_i18ngrep "invalid command" err
'
+test_expect_success 'request capability as command' '
+ 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 &&
+ command=ls-refs
+ object-format=$(test_oid algo)
+ fetch
+ 0000
+ EOF
+ test_must_fail test-tool serve-v2 --stateless-rpc 2>err <in &&
+ grep unknown.capability err
+'
+
test_expect_success 'requested command is command=value' '
test-tool pkt-line pack >in <<-EOF &&
command=ls-refs=whatever
--
2.33.0.917.g33ebf6a5f6
^ permalink raw reply related [flat|nested] 77+ messages in thread* [PATCH v3 11/11] ls-refs: reject unknown arguments
2021-09-15 18:34 ` [PATCH v3 " Jeff King
` (9 preceding siblings ...)
2021-09-15 18:36 ` [PATCH v3 10/11] serve: reject commands used as capabilities Jeff King
@ 2021-09-15 18:36 ` Jeff King
10 siblings, 0 replies; 77+ messages in thread
From: Jeff King @ 2021-09-15 18:36 UTC (permalink / raw)
To: git
Cc: Eric Sunshine, Junio C Hamano, Taylor Blau, Martin Ågren,
Ævar Arnfjörð Bjarmason
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 3928424e1b..aa1827d841 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -147,6 +147,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
--
2.33.0.917.g33ebf6a5f6
^ permalink raw reply related [flat|nested] 77+ messages in thread