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

[PATCH 1/7] oid_array: use size_t for count and allocation

From
Jeff King <peff@peff.net>
Date
Mar 30, 2020, 14:03 UTC
Message-ID
<20200330140309.GA2456038@coredump.intra.peff.net>
In-Reply-To
<20200330140247.GA476088@coredump.intra.peff.net>

The oid_array object uses an "int" to store the number of items and the allocated size. It's rather unlikely for somebody to have more than 2^31 objects in a repository (the sha1's alone would be 40GB!), but if they do, we'd overflow our alloc variable.

You can reproduce this case with something like:
  git init repo
  cd repo
  # make a pack with 2^24 objects
  perl -e '
    my $nr = 2**24;
    for (my $i = 0; $i < $nr; $i++) {
	    print "blob\n";
	    print "data 4\n";
	    print pack("N", $i);
    }
  ' | git fast-import
  # now make 256 copies of it; most of these objects will be duplicates,
  # but oid_array doesn't de-dup until all values are read and it can
  # sort the result.
  cd .git/objects/pack/
  pack=$(echo *.pack)
  idx=$(echo *.idx)
  for i in $(seq 0 255); do
    # no need to waste disk space
    ln "$pack" "pack-extra-$i.pack"
    ln "$idx" "pack-extra-$i.idx"
  done
  # and now force an oid_array to store all of it
  git cat-file --batch-all-objects --batch-check
which results in:
  fatal: size_t overflow: 32 * 18446744071562067968

So the good news is that st_mult() sees the problem (the large number is because our int wraps negative, and then that gets cast to a size_t), doing the job it was meant to: bailing in crazy situations rather than causing an undersized buffer.

But we should avoid hitting this case at all, and instead limit ourselves based on what malloc() is willing to give us. We can easily do that by switching to size_t.

The cat-file process above made it to ~120GB virtual set size before the integer overflow (our internal hash storage is 32-bytes now in preparation for sha256, so we'd expect ~128GB total needed, plus potentially more to copy from one realloc'd block to another)). After this patch (and about 130GB of RAM+swap), it does eventually read in the whole set. No test for obvious reasons.

Signed-off-by: Jeff King <peff@peff.net>
---
 sha1-array.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sha1-array.h b/sha1-array.h
index dc1bca9c9a..c5e4b9324f 100644
--- a/sha1-array.h
+++ b/sha1-array.h
@@ -49,8 +49,8 @@
  */
 struct oid_array {
 	struct object_id *oid;
-	int nr;
-	int alloc;
+	size_t nr;
+	size_t alloc;
 	int sorted;
 };
 
-- 
2.26.0.597.g7e08ed78ff
Previous: Jeff KingNext: Jeff King
Message 2 of 12 in “oid_array cleanups”
  1. 0/7 oid_array cleanupsJeff King, Mar 30, 2020
  2. 1/7 oid_array: use size_t for count and allocationJeff King, Mar 30, 2020
  3. Jeff KingMar 30, 2020
  4. Taylor BlauApr 15, 2020
  5. 2/7 oid_array: use size_t for iterationJeff King, Mar 30, 2020
  6. 3/7 oid_array: rename source file from sha1-arrayJeff King, Mar 30, 2020
  7. Taylor BlauApr 15, 2020
  8. 4/7 test-tool: rename sha1-array to oid-arrayJeff King, Mar 30, 2020
  9. 5/7 bisect: stop referring to sha1_arrayJeff King, Mar 30, 2020
  10. 6/7 ref-filter: stop referring to "sha1 array"Jeff King, Mar 30, 2020
  11. 7/7 oidset: stop referring to sha1-arrayJeff King, Mar 30, 2020
  12. Taylor BlauApr 15, 2020

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.