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

[PATCH 8/9] upload-pack: use PARSE_OBJECT_SKIP_HASH_CHECK in more places

From
Jeff King <peff@peff.net>
Date
Feb 28, 2024, 22:39 UTC
Message-ID
<20240228223903.GH1158131@coredump.intra.peff.net>
In-Reply-To
<20240228223700.GA1157826@coredump.intra.peff.net>

In commit 0bc2557951 (upload-pack: skip parse-object re-hashing of "want" objects, 2022-09-06), we optimized the parse_object() calls for v2 "want" lines from the client so that they avoided parsing blobs, and so that they used the commit-graph rather than parsing commit objects from scratch.

We should extend that to two other spots:
  1. We parse "have" objects in the got_oid() function. These won't
     generally be non-commits (unlike "want" lines from a partial
     clone). But we still benefit from the use of the commit-graph.
  2. For v0, the "want" lines are parsed in receive_needs(). These are
     also less likely to be non-commits because by default they have to
     be ref tips. There are config options you might set to allow
     non-tip objects, but you'd mostly do so to support partial clones,
     and clients recent enough to support partial clone will generally
     speak v2 anyway.

So I don't expect this change to improve performance much for day-to-day operations. But both are possible denial-of-service vectors, where an attacker can waste our time by sending over a large number of objects to parse (of course we may waste even more time serving a pack to them, but we try as much as possible to optimize that in pack-objects; we should do what we can here in upload-pack, too).

With this patch, running p5600 with GIT_TEST_PROTOCOL_VERSION=0 shows similar results to what we saw in 0bc2557951 (which ran with the v2 protocol by default). Here are the numbers for linux.git:

  Test                          HEAD^                 HEAD
  -----------------------------------------------------------------------------
  5600.3: checkout of result    50.91(87.95+2.93)     41.75(79.00+3.18) -18.0%

Or for a more extreme (and malicious) case, we can claim to "have" every blob in git.git over the v0 protocol:

  $ {
      echo "0032want $(git rev-parse HEAD)"
      printf 0000
      git cat-file --batch-all-objects --batch-check='%(objectname) %(objecttype)' |
      perl -alne 'print "0032have $F[0]" if $F[1] eq "blob"'
    } >input
  $ time ./git.old upload-pack . <input >/dev/null
  real	0m52.951s
  user	0m51.633s
  sys	0m1.304s
  $ time ./git.new upload-pack . <input >/dev/null
  real	0m0.261s
  user	0m0.156s
  sys	0m0.105s

(Note that these don't actually compute a pack because of the hacky protocol usage, so those numbers are representing the raw blob-parsing effort done by upload-pack).

Signed-off-by: Jeff King <peff@peff.net>
---
 upload-pack.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 3970bb9b30..b721155442 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -469,7 +469,8 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid)
 {
 	int we_knew_they_have = 0;
-	struct object *o = parse_object(the_repository, oid);
+	struct object *o = parse_object_with_flags(the_repository, oid,
+						   PARSE_OBJECT_SKIP_HASH_CHECK);
 
 	if (!o)
 		die("oops (%s)", oid_to_hex(oid));
@@ -1148,7 +1149,8 @@ static void receive_needs(struct upload_pack_data *data,
 			free(client_sid);
 		}
 
-		o = parse_object(the_repository, &oid_buf);
+		o = parse_object_with_flags(the_repository, &oid_buf,
+					    PARSE_OBJECT_SKIP_HASH_CHECK);
 		if (!o) {
 			packet_writer_error(&data->writer,
 					    "upload-pack: not our ref %s",
-- 
2.44.0.rc2.424.gbdbf4d014b
Previous: Jeff KingNext: Jeff King
Message 12 of 17 in “bound upload-pack memory allocations”
  1. 0/9 bound upload-pack memory allocationsJeff King, Feb 28, 2024
  2. 1/9 upload-pack: drop separate v2 "haves" arrayJeff King, Feb 28, 2024
  3. 2/9 upload-pack: switch deepen-not list to an oid_arrayJeff King, Feb 28, 2024
  4. 3/9 upload-pack: use oidset for deepen_not listJeff King, Feb 28, 2024
  5. 4/9 upload-pack: use a strmap for want-ref linesJeff King, Feb 28, 2024
  6. 5/9 upload-pack: accept only a single packfile-uri lineJeff King, Feb 28, 2024
  7. 6/9 upload-pack: disallow object-info capability by defaultJeff King, Feb 28, 2024
  8. Patrick SteinhardtMar 4, 2024
  9. Jeff KingMar 4, 2024
  10. Taylor BlauApr 29, 2024
  11. 7/9 upload-pack: always turn off save_commit_bufferJeff King, Feb 28, 2024
  12. 8/9 upload-pack: use PARSE_OBJECT_SKIP_HASH_CHECK in more placesJeff King, Feb 28, 2024
  13. 9/9 upload-pack: free tree buffers after parsingJeff King, Feb 28, 2024
  14. Patrick SteinhardtMar 4, 2024
  15. Jeff KingMar 4, 2024
  16. Patrick SteinhardtMar 4, 2024
  17. Junio C HamanoFeb 28, 2024

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.