diff mbox series

[5/9] upload-pack: accept only a single packfile-uri line

Message ID 20240228223846.GE1158131@coredump.intra.peff.net (mailing list archive)
State Accepted
Commit 179776f9e6c4bd2f423c78fcf8b1e4cb4afc4c45
Headers show
Series bound upload-pack memory allocations | expand

Commit Message

Jeff King Feb. 28, 2024, 10:38 p.m. UTC
When we see a packfile-uri line from the client, we use
string_list_split() to split it on commas and store the result in a
string_list.  A single packfile-uri line is therefore limited to storing
~64kb, the size of a pkt-line.

But we'll happily accept multiple such lines, and each line appends to
the string list, growing without bound.

In theory this could be useful, making:

  0017packfile-uris http
  0018packfile-uris https

equivalent to:

  001dpackfile-uris http,https

But the protocol documentation doesn't indicate that this should work
(and indeed, refers to this in the singular as "the following argument
can be included in the client's request"). And the client-side
implementation in fetch-pack has always sent a single line (JGit appears
to understand the line on the server side but has no client-side
implementation, and libgit2 understands neither).

If we were worried about compatibility, we could instead just put a
limit on the maximum number of values we'd accept. The current client
implementation limits itself to only two values: "http" and "https", so
something like "256" would be more than enough. But accepting only a
single line seems more in line with the protocol documentation, and
matches other parts of the protocol (e.g., we will not accept a second
"filter" line).

We'll also make this more explicit in the protocol documentation; as
above, I think this was always the intent, but there's no harm in making
it clear.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/gitprotocol-v2.txt | 3 ++-
 upload-pack.c                    | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)
diff mbox series

Patch

diff --git a/Documentation/gitprotocol-v2.txt b/Documentation/gitprotocol-v2.txt
index 837ea6171e..414bc625d5 100644
--- a/Documentation/gitprotocol-v2.txt
+++ b/Documentation/gitprotocol-v2.txt
@@ -362,7 +362,8 @@  included in the client's request:
 If the 'packfile-uris' feature is advertised, the following argument
 can be included in the client's request as well as the potential
 addition of the 'packfile-uris' section in the server's response as
-explained below.
+explained below. Note that at most one `packfile-uris` line can be sent
+to the server.
 
     packfile-uris <comma-separated-list-of-protocols>
 	Indicates to the server that the client is willing to receive
diff --git a/upload-pack.c b/upload-pack.c
index 8b47576ec7..2a5c52666e 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1646,6 +1646,9 @@  static void process_args(struct packet_reader *request,
 		}
 
 		if (skip_prefix(arg, "packfile-uris ", &p)) {
+			if (data->uri_protocols.nr)
+				send_err_and_die(data,
+						 "multiple packfile-uris lines forbidden");
 			string_list_split(&data->uri_protocols, p, ',', -1);
 			continue;
 		}