git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 7/7] v0 protocol: use size_t for capability length/offset

From
Jeff King <peff@peff.net>
Date
Apr 14, 2023, 21:25 UTC
Message-ID
<20230414212520.GG639756@coredump.intra.peff.net>
In-Reply-To
<20230414212404.GA639653@coredump.intra.peff.net>

When parsing server capabilities, we use "int" to store lengths and offsets. At first glance this seems like a spot where our parser may be confused by integer overflow if somebody sent us a malicious response.

In practice these strings are all bounded by the 64k limit of a pkt-line, so using "int" is OK. However, it makes the code simpler to audit if they just use size_t everywhere. Note that because we take these parameters as pointers, this also forces many callers to update their declared types.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/receive-pack.c |  2 +-
 connect.c              | 22 +++++++++++-----------
 connect.h              |  4 ++--
 fetch-pack.c           |  4 ++--
 send-pack.c            |  2 +-
 transport.c            |  2 +-
 upload-pack.c          |  2 +-
 7 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9109552533..3b495ecc84 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2093,7 +2093,7 @@ static struct command *read_head_info(struct packet_reader *reader,
 			const char *feature_list = reader->line + linelen + 1;
 			const char *hash = NULL;
 			const char *client_sid;
-			int len = 0;
+			size_t len = 0;
 			if (parse_feature_request(feature_list, "report-status"))
 				report_status = 1;
 			if (parse_feature_request(feature_list, "report-status-v2"))
diff --git a/connect.c b/connect.c
index b7ba5f5727..c54adc652f 100644
--- a/connect.c
+++ b/connect.c
@@ -22,7 +22,7 @@
 
 static char *server_capabilities_v1;
 static struct strvec server_capabilities_v2 = STRVEC_INIT;
-static const char *next_server_feature_value(const char *feature, int *len, int *offset);
+static const char *next_server_feature_value(const char *feature, size_t *len, size_t *offset);
 
 static int check_ref(const char *name, unsigned int flags)
 {
@@ -205,10 +205,10 @@ static void parse_one_symref_info(struct string_list *symref, const char *val, i
 static void annotate_refs_with_symref_info(struct ref *ref)
 {
 	struct string_list symref = STRING_LIST_INIT_DUP;
-	int offset = 0;
+	size_t offset = 0;
 
 	while (1) {
-		int len;
+		size_t len;
 		const char *val;
 
 		val = next_server_feature_value("symref", &len, &offset);
@@ -231,7 +231,7 @@ static void annotate_refs_with_symref_info(struct ref *ref)
 static void process_capabilities(struct packet_reader *reader, int *linelen)
 {
 	const char *feat_val;
-	int feat_len;
+	size_t feat_len;
 	const char *line = reader->line;
 	int nul_location = strlen(line);
 	if (nul_location == *linelen)
@@ -596,10 +596,10 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
 	return list;
 }
 
-const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset)
+const char *parse_feature_value(const char *feature_list, const char *feature, size_t *lenp, size_t *offset)
 {
 	const char *orig_start = feature_list;
-	int len;
+	size_t len;
 
 	if (!feature_list)
 		return NULL;
@@ -623,7 +623,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i
 			}
 			/* feature with a value (e.g., "agent=git/1.2.3") */
 			else if (*value == '=') {
-				int end;
+				size_t end;
 
 				value++;
 				end = strcspn(value, " \t\n");
@@ -645,8 +645,8 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i
 
 int server_supports_hash(const char *desired, int *feature_supported)
 {
-	int offset = 0;
-	int len;
+	size_t offset = 0;
+	size_t len;
 	const char *hash;
 
 	hash = next_server_feature_value("object-format", &len, &offset);
@@ -670,12 +670,12 @@ int parse_feature_request(const char *feature_list, const char *feature)
 	return !!parse_feature_value(feature_list, feature, NULL, NULL);
 }
 
-static const char *next_server_feature_value(const char *feature, int *len, int *offset)
+static const char *next_server_feature_value(const char *feature, size_t *len, size_t *offset)
 {
 	return parse_feature_value(server_capabilities_v1, feature, len, offset);
 }
 
-const char *server_feature_value(const char *feature, int *len)
+const char *server_feature_value(const char *feature, size_t *len)
 {
 	return parse_feature_value(server_capabilities_v1, feature, len, NULL);
 }
diff --git a/connect.h b/connect.h
index f41a0b4c1f..1645126c17 100644
--- a/connect.h
+++ b/connect.h
@@ -12,14 +12,14 @@ int finish_connect(struct child_process *conn);
 int git_connection_is_socket(struct child_process *conn);
 int server_supports(const char *feature);
 int parse_feature_request(const char *features, const char *feature);
-const char *server_feature_value(const char *feature, int *len_ret);
+const char *server_feature_value(const char *feature, size_t *len_ret);
 int url_is_local_not_ssh(const char *url);
 
 struct packet_reader;
 enum protocol_version discover_version(struct packet_reader *reader);
 
 int server_supports_hash(const char *desired, int *feature_supported);
-const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset);
+const char *parse_feature_value(const char *feature_list, const char *feature, size_t *lenp, size_t *offset);
 int server_supports_v2(const char *c);
 void ensure_server_supports_v2(const char *c);
 int server_feature_v2(const char *c, const char **v);
diff --git a/fetch-pack.c b/fetch-pack.c
index 368f2ed25a..97a44ed582 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1099,7 +1099,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 	struct ref *ref = copy_ref_list(orig_ref);
 	struct object_id oid;
 	const char *agent_feature;
-	int agent_len;
+	size_t agent_len;
 	struct fetch_negotiator negotiator_alloc;
 	struct fetch_negotiator *negotiator;
 
@@ -1117,7 +1117,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		agent_supported = 1;
 		if (agent_len)
 			print_verbose(args, _("Server version is %.*s"),
-				      agent_len, agent_feature);
+				      (int)agent_len, agent_feature);
 	}
 
 	if (!server_supports("session-id"))
diff --git a/send-pack.c b/send-pack.c
index f81bbb28d7..97344b629e 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -538,7 +538,7 @@ int send_pack(struct send_pack_args *args,
 		die(_("the receiving end does not support this repository's hash algorithm"));
 
 	if (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {
-		int len;
+		size_t len;
 		push_cert_nonce = server_feature_value("push-cert", &len);
 		if (push_cert_nonce) {
 			reject_invalid_nonce(push_cert_nonce, len);
diff --git a/transport.c b/transport.c
index 89a220425e..6223dc3de2 100644
--- a/transport.c
+++ b/transport.c
@@ -317,7 +317,7 @@ static struct ref *handshake(struct transport *transport, int for_push,
 	struct git_transport_data *data = transport->data;
 	struct ref *refs = NULL;
 	struct packet_reader reader;
-	int sid_len;
+	size_t sid_len;
 	const char *server_sid;
 
 	connect_setup(transport, for_push);
diff --git a/upload-pack.c b/upload-pack.c
index e23f16dfdd..565e46058f 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1067,7 +1067,7 @@ static void receive_needs(struct upload_pack_data *data,
 		const char *features;
 		struct object_id oid_buf;
 		const char *arg;
-		int feature_len;
+		size_t feature_len;
 
 		reset_timeout(data->timeout);
 		if (packet_reader_read(reader) != PACKET_READ_NORMAL)

-- 
2.40.0.515.gdfb9e78b42
Previous: Jeff KingNext: Junio C Hamano
Message 25 of 30 in “Infinite loop + memory leak in annotate_refs_with_symref_info”
  1. Jonas HaagApr 11, 2023
  2. Taylor BlauApr 11, 2023
  3. Taylor BlauApr 11, 2023
  4. brian m. carlsonApr 12, 2023
  5. Jeff KingApr 11, 2023
  6. Jeff KingApr 11, 2023
  7. Taylor BlauApr 11, 2023
  8. Jeff KingApr 11, 2023
  9. Junio C HamanoApr 11, 2023
  10. 0/7 v0 multiple-symref infinite loop fix and test cleanupJeff King, Apr 12, 2023
  11. 1/7 v0 protocol: fix infinite loop when parsing multi-valued capabilitiesJeff King, Apr 12, 2023
  12. Jeff KingApr 12, 2023
  13. 1/7 v0 protocol: fix infinite loop when parsing multi-valued capabilitiesJeff King, Apr 12, 2023
  14. Jeff KingApr 12, 2023
  15. 2/7 t5512: stop referring to "v1" protocolJeff King, Apr 12, 2023
  16. 3/7 t5512: stop using jgit for capabilities^{} testJeff King, Apr 12, 2023
  17. Jeff KingApr 12, 2023
  18. 0/7 v0 multiple-symref infinite loop fix and test cleanupJeff King, Apr 14, 2023
  19. 1/7 v0 protocol: fix infinite loop when parsing multi-valued capabilitiesJeff King, Apr 14, 2023
  20. 2/7 t5512: stop referring to "v1" protocolJeff King, Apr 14, 2023
  21. 3/7 v0 protocol: fix sha1/sha256 confusion for capabilities^{}Jeff King, Apr 14, 2023
  22. 5/7 t5512: allow any protocol version for filtered symref testJeff King, Apr 14, 2023
  23. 4/7 t5512: add v2 support for "ls-remote --symref" testJeff King, Apr 14, 2023
  24. 6/7 t5512: test "ls-remote --heads --symref" filtering with v0 and v2Jeff King, Apr 14, 2023
  25. 7/7 v0 protocol: use size_t for capability length/offsetJeff King, Apr 14, 2023
  26. Junio C HamanoApr 17, 2023
  27. 4/7 t5512: add v2 support for "ls-remote --symref" testJeff King, Apr 12, 2023
  28. 5/7 t5512: allow any protocol version for filtered symref testJeff King, Apr 12, 2023
  29. 6/7 t5512: test "ls-remote --heads --symref" filtering with v0 and v2Jeff King, Apr 12, 2023
  30. 7/7 v0 protocol: use size_t for capability length/offsetJeff King, Apr 12, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.