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

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

From
Taylor Blau <me@ttaylorr.com>
Date
Apr 15, 2020, 00:27 UTC
Message-ID
<20200415002758.GC7457@syl.local>
In-Reply-To
<20200330140309.GA2456038@coredump.intra.peff.net>
On Mon, Mar 30, 2020 at 10:03:09AM -0400, Jeff King wrote:
Show 55 quoted lines
> 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.

;). This patch looks good, and makes immediate sense given your explanation.

Show 22 quoted lines
> 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

Thanks, Taylor

Previous: Jeff KingNext: Jeff King
Message 4 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.