Inflate large loose blobs by ethomson · Pull Request #4443 · libgit2/libgit2

ethomson · 2017-12-12T15:35:23Z

As reported at libgit2/libgit2sharp#1515, we are unable to inflate large loose blobs (that are larger than 4GB). (In that issue, we create a large loose blob but fail to read it immediately after that.) The culprit, of course, is 32 bitness: in a number of places we either improperly truncate 64 bit values via casting, or fail to read/write in a loop for APIs that take blocks of data, attempting to give them the entire blob at a time.

Fix this by:

Move the loose object backend to use the git_zstream API instead of using zlib directly. The git_zstream API simplifies the common inflate or deflate experience and helps eliminate common problems like neglecting to loop to process the entire file.
Update the win32 and CommonCrypto hash backends to loop over the entire file

Also add a test for reading/writing loose objects, guarded by an environment variable to avoid running by default.

ethomson · 2017-12-15T11:28:25Z

lolol the build machines OOM trying to run this test as we inflate a very large blob into memory. 🙄

I'll add a streaming reader test, just to make sure that we can read 5GB of blobs into /dev/null, and I'll make sure that these segfaults on Linux are actually due to running out of memory before the OOM killer can get to us (and not us ignoring an error code or something).

pks-t

I just had a cursory read right now, in particular I only skimmed the migration to git_zstream. I'll have another look as soon as you've changed the tests.

pks-t · 2017-12-15T12:26:09Z


 	for (i = 0; i < ARRAY_SIZE(git_objects_table); i++)
-		if (git_object_typeisloose(i) &&
+		if (*git_objects_table[i].str &&


Ah, yeah, that line confused me a lot in the last commit. So probably this line change should be moved to the previous commit?

Yes, agreed. I think I fixup'd the wrong commit here.

pks-t · 2017-12-15T12:34:45Z

-	obj_hdr *out, size_t *out_len, unsigned char *data, size_t data_len)
+	obj_hdr *out, size_t *out_len, unsigned char *_data, size_t data_len)
 {
+	const char *data = (char *)_data;


Looks like this chunk belongs into "odb: support large loose objects"? You should also make the data parameter const.

pks-t · 2017-12-15T12:36:55Z

 		goto on_error;

+	if ((uint64_t)size > SIZE_MAX) {
+		giterr_set(GITERR_OBJECT, "object is larger than available memory");


This is technically not correct, as this is not about available memory (which we don't know about right now). Rather, this should be somethink like "object size exceeds address space"?

Well, I think that's not technically correct either. size_t doesn't reflect the address space, it reflects the size of the largest single object size. But I think that this is practically correct for the set of computers that are actually in use, and that this is a much more actionable error message.

Yeah, guess one should be a bit more precise when nagging about technical incorrectness :P

pks-t · 2017-12-15T12:38:59Z

-GIT_INLINE(int) git_hash_update(git_hash_ctx *ctx, const void *data, size_t len)
+GIT_INLINE(int) git_hash_update(git_hash_ctx *ctx, const void *_data, size_t len)
 {
+	const unsigned char *data = _data;


By the way, why did you add all those casts to the functions? Just curious

xcode was whinging about a signed/unsigned mismatch on CC_SHA1_Update so I decided to cast them to their proper types as defined by the implementations.

ethomson · 2017-12-19T00:35:06Z

I'll add a streaming reader test, just to make sure that we can read 5GB of blobs into /dev/null, and I'll make sure that these segfaults on Linux are actually due to running out of memory before the OOM killer can get to us (and not us ignoring an error code or something).

Oh, I remember way back when I wrote this comment, full of naivete.

Yes, I would simply add a streaming reader test... if we had a streaming reader. Despite having a streaming reader API in the ODB layer, it turns out that we don't actually have any backends that implement it.

So I've now changed the streaming reader API a bit, to provide the type and length of the object when initializing the stream. This would be a breaking API change - except, of course, that nobody is actually using the streaming reader API since none of the backends support it. So I feel pretty good breaking this API that nobody could be using.

Then I added streaming reader support to the loose object ODB backend. I cleaned up a bit while I was in there to add some more tests for things like read_header which had no explicit tests (and in fact failed on some of the loose ODB test corpus), teach read_header how to cope with "packlike loose objects" (which were a weird loose object format that was attempted for a bit and will never actually be seen in the wild) and drop the odb_loose internal zstream abstraction layer that ultimately became git_zstream.

I didn't really expect this PR to become so large. Alas.

ethomson · 2017-12-19T01:01:12Z

I didn't really expect this PR to become so large. Alas.

Actually, I'm going to limit this PR to the large loose blob handling. I will follow up with a PR that includes the refactoring to include readstream support for loose odb backends.

zlib will return `Z_BUF_ERROR` whenever there is more input to inflate or deflate than there is output to store the result. This is normal for us as we iterate through the input, particularly with very large input buffers.

Introduce `git_prefixncmp` that will search up to the first `n` characters of a string to see if it is prefixed by another string. This is useful for examining if a non-null terminated character array is prefixed by a particular substring. Consolidate the various implementations of `git__prefixcmp` around a single core implementation and add some test cases to validate its behavior.

Introduce a test for very large objects in the ODB. Write a large object (5 GB) and ensure that the write succeeds and provides us the expected object ID. Introduce a test that writes that file and ensures that we can subsequently read it.

Introduce an internal API to get the object type based on a length-specified (not null terminated) string representation. This can be used to compare the (space terminated) object type name in a loose object. Reimplement `git_object_string2type` based on this API.

zlib will only inflate/deflate an `int`s worth of data at a time. We need to loop through large files in order to ensure that we inflate the entire file, not just an `int`s worth of data. Thankfully, we already have this loop in our `git_zstream` layer. Handle large objects using the `git_zstream`.

Instead of paging to zlib in INT_MAX sized chunks, we can give it as many as UINT_MAX bytes at a time. zlib doesn't care how big a buffer we give it, this simply results in fewer calls into zlib.

Check the size of objects being read from the loose odb backend and reject those that would not fit in memory with an error message that reflects the actual problem, instead of error'ing later with an unintuitive error message regarding truncation or invalid hashes.

Teach the win32 hash mechanisms to support large files. The hash primitives take at most `ULONG_MAX` bytes at a time. Loop, giving the hash function the maximum supported number of bytes, until we have hashed the entire file.

Teach the CommonCrypto hash mechanisms to support large files. The hash primitives take a `CC_LONG` (aka `uint32_t`) at a time. So loop to give the hash function at most an unsigned 32 bit's worth of bytes until we have hashed the entire file.

Writing very large files may be slow, particularly on inefficient filesystems and when running instrumented code to detect invalid memory accesses (eg within valgrind or similar tools). Introduce `GITTEST_SLOW` so that tests that are slow can be skipped by the CI system.

ethomson · 2017-12-20T16:37:01Z

The new tests were timing out the build during the valgrind run. So I ve added still another environment variable so that we can skip them on our CI runs.

Another option is to just stop running the tests marked "invasive" during CI entirely. But I like running as many tests as is feasible during our CI run, even at the expense of bloating our guard environment variables.

I thought about spinning up a background thread to dump something to the console during the tests, but, you know, ugh. I'm not sure which is worse: tests failing because they took more than 10 minutes to run, or tests that just keep running.

ethomson · 2017-12-29T19:45:22Z

I'd like to unblock users who are facing this. I'm going to merge this unless there are complaints.

pks-t · 2018-01-03T12:01:19Z

Going down the rabbit hole once again

ethomson force-pushed the ethomson/large_loose_blobs branch from 138384b to 42d149f Compare December 14, 2017 13:25

pks-t reviewed Dec 15, 2017

View reviewed changes

ethomson force-pushed the ethomson/large_loose_blobs branch from 42d149f to 79faa70 Compare December 19, 2017 00:25

ethomson force-pushed the ethomson/large_loose_blobs branch 2 times, most recently from 8efb32c to de3ae26 Compare December 19, 2017 01:00

ethomson mentioned this pull request Dec 19, 2017

Streaming read support for the loose ODB backend #4450

Merged

ethomson added 10 commits December 20, 2017 16:08

zstream: treat Z_BUF_ERROR as non-fatal

b7d36ef

zlib will return `Z_BUF_ERROR` whenever there is more input to inflate or deflate than there is output to store the result. This is normal for us as we iterate through the input, particularly with very large input buffers.

odb: test loose reading/writing large objects

dacc329

Introduce a test for very large objects in the ODB. Write a large object (5 GB) and ensure that the write succeeds and provides us the expected object ID. Introduce a test that writes that file and ensures that we can subsequently read it.

zstream: use UINT_MAX sized chunks

8642feb

Instead of paging to zlib in INT_MAX sized chunks, we can give it as many as UINT_MAX bytes at a time. zlib doesn't care how big a buffer we give it, this simply results in fewer calls into zlib.

hash: win32 hash mechanism should support large files

a89560d

Teach the win32 hash mechanisms to support large files. The hash primitives take at most `ULONG_MAX` bytes at a time. Loop, giving the hash function the maximum supported number of bytes, until we have hashed the entire file.

ethomson force-pushed the ethomson/large_loose_blobs branch from de3ae26 to 456e521 Compare December 20, 2017 16:21

ethomson merged commit e14bf97 into master Dec 30, 2017

ethomson mentioned this pull request Apr 10, 2018

"error inflating zlib stream" when reading blobs > 4GB #4615

Closed

ethomson deleted the ethomson/large_loose_blobs branch January 9, 2019 10:16

Uh oh!

Conversation

ethomson commented Dec 12, 2017

Uh oh!

ethomson commented Dec 15, 2017

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethomson commented Dec 19, 2017

Uh oh!

ethomson commented Dec 19, 2017

Uh oh!

ethomson commented Dec 20, 2017

Uh oh!

ethomson commented Dec 29, 2017

Uh oh!

pks-t commented Jan 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants