summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorLibravatar Junio C Hamano <gitster@pobox.com>2021-09-28 13:06:53 -0700
committerLibravatar Junio C Hamano <gitster@pobox.com>2021-09-28 13:06:53 -0700
commitbb1677fc29cf64b5ca5c3835261fe360ac2780ad (patch)
tree33b173bca408a590ef10afe8285e0d99ac6e9f7c
parentThe eighth batch (diff)
parentls-refs: reject unknown arguments (diff)
downloadtgif-bb1677fc29cf64b5ca5c3835261fe360ac2780ad.tar.xz
Merge branch 'jk/reduce-malloc-in-v2-servers'
Code cleanup to limit memory consumption and tighten protocol message parsing. * jk/reduce-malloc-in-v2-servers: ls-refs: reject unknown arguments serve: reject commands used as capabilities serve: reject bogus v2 "command=ls-refs=foo" docs/protocol-v2: clarify some ls-refs ref-prefix details ls-refs: ignore very long ref-prefix counts serve: drop "keys" strvec serve: provide "receive" function for session-id capability serve: provide "receive" function for object-format capability serve: add "receive" method for v2 capabilities table serve: return capability "value" from get_capability() serve: rename is_command() to parse_command()
-rw-r--r--Documentation/technical/protocol-v2.txt6
-rw-r--r--ls-refs.c22
-rw-r--r--serve.c120
-rwxr-xr-xt/t5701-git-serve.sh75
4 files changed, 164 insertions, 59 deletions
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 59b86fcf97..21e8258ccf 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -199,7 +199,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.
diff --git a/ls-refs.c b/ls-refs.c
index be09568910..73eb7da528 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -41,6 +41,12 @@ static void ensure_config_read(void)
}
/*
+ * 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.
*/
@@ -158,15 +164,27 @@ 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;
+ else
+ die(_("unexpected line: '%s'"), arg);
}
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/serve.c b/serve.c
index 1817edc7f5..b3fe9b5126 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 &&
@@ -45,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
@@ -70,6 +90,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[] = {
@@ -94,10 +124,12 @@ static struct protocol_capability capabilities[] = {
{
.name = "object-format",
.advertise = object_format_advertise,
+ .receive = object_format_receive,
},
{
.name = "session-id",
.advertise = session_id_advertise,
+ .receive = session_id_receive,
},
{
.name = "object-info",
@@ -139,7 +171,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,31 +181,46 @@ 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)
+static int receive_client_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);
+ if (!c || c->command || !c->advertise(the_repository, NULL))
+ return 0;
+
+ if (c->receive)
+ c->receive(the_repository, value);
+ return 1;
}
-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;
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'",
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;
@@ -183,42 +230,6 @@ static int is_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;
-}
-
-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,
@@ -228,9 +239,8 @@ 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;
- const char *client_sid;
packet_reader_init(&reader, 0, NULL, 0,
PACKET_READ_CHOMP_NEWLINE |
@@ -250,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 (is_command(reader.line, &command) ||
- is_valid_capability(reader.line))
- strvec_push(&keys, reader.line);
+ if (parse_command(reader.line, &command) ||
+ receive_client_capability(reader.line))
+ seen_capability_or_command = 1;
else
die("unknown capability '%s'", reader.line);
@@ -265,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;
/*
@@ -292,14 +301,13 @@ static int process_request(void)
if (!command)
die("no command requested");
- check_algorithm(the_repository, &keys);
-
- if (has_capability(&keys, "session-id", &client_sid))
- trace2_data_string("transfer", NULL, "client-sid", client_sid);
+ 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);
command->command(the_repository, &reader);
- strvec_clear(&keys);
return 0;
}
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index 930721f053..aa1827d841 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -72,6 +72,37 @@ 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
+ 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
@@ -116,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
@@ -158,6 +202,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