Re: [PATCH 7/9] ls-refs: ignore very long ref-prefix counts

On Tue, 14 Sept 2021 at 17:38, Jeff King [off-list ref] wrote:

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).

This all makes sense to me. At some point, we should be able to go "I
don't know what you're trying to do, but let me just ignore all this
craziness and instead try to give you a useful result sooner rather than
later".

I do wonder if we should document that the client can't trust us to
actually do all this culling. In general, I find that it's a matter of
hygiene for the client to do its own checks, but with this change they
actually *need* to do them. (Unless they know our limit and that they're
on the right side of it, but that kind of magic is even less hygienic.)

Is it easier to reason about with something like this
(whitespace-damaged) on top?

diff --git a/ls-refs.c b/ls-refs.c
index 839fb0caa9..b3101ff361 100644
--- a/ls-refs.c
+++ b/ls-refs.c

@@ -147,7 +147,6 @@ static int ls_refs_config(const char *var, const

char *value, void *data)
 int ls_refs(struct repository *r, struct packet_reader *request)
 {
        struct ls_refs_data data;
-       int too_many_prefixes = 0;

        memset(&data, 0, sizeof(data));
        strvec_init(&data.prefixes);

@@ -164,14 +163,8 @@ int ls_refs(struct repository *r, struct

packet_reader *request)
                else if (!strcmp("symrefs", arg))
                        data.symrefs = 1;
                else if (skip_prefix(arg, "ref-prefix ", &out)) {
-                       if (too_many_prefixes) {
-                               /* ignore any further ones */
-                       } else if (data.prefixes.nr >= MAX_ALLOWED_PREFIXES) {
-                               strvec_clear(&data.prefixes);
-                               too_many_prefixes = 1;
-                       } else {
+                       if (data.prefixes.nr <= MAX_ALLOWED_PREFIXES)
                                strvec_push(&data.prefixes, out);
-                       }
                }
                else if (!strcmp("unborn", arg))
                        data.unborn = allow_unborn;

@@ -180,6 +173,9 @@ int ls_refs(struct repository *r, struct

packet_reader *request)
        if (request->status != PACKET_READ_FLUSH)
                die(_("expected flush after ls-refs arguments"));

+       if (data.prefixes.nr > MAX_ALLOWED_PREFIXES)
+               strvec_clear(&data.prefixes);
+
        send_possibly_unborn_head(&data);
        if (!data.prefixes.nr)
                strvec_push(&data.prefixes, "");

Maybe even name the macro TOO_MANY_PREFIXES (and bump it by one)
to make the logic instead be

        if (data.prefixes.nr < TOO_MANY_PREFIXES)
                strvec_push(&data.prefixes, out);
 ...
        if (data.prefixes.nr >= TOO_MANY_PREFIXES)
                strvec_clear(&data.prefixes);

Just a thought. I'm reaching to try to find a way to improve this
series. ;-) It was a nice read.


Martin