{"thread":{"id":"27584","subject":"[PATCH] sha1_file.c: zlib can only process 4GB at a time","startedAt":"2011-06-08T19:03:35Z","lastAt":"2011-06-09T20:11:58Z","messageCount":3,"participants":["Junio C Hamano","Andreas Schwab"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"169683","messageId":"7v1uz42liw.fsf@alter.siamese.dyndns.org","threadId":"27584","inReplyTo":null,"subject":"[PATCH] sha1_file.c: zlib can only process 4GB at a time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-08T19:03:35Z","receivedAt":"2011-06-08T19:03:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The size of objects we read from the repository and data we try to put\ninto the repository are represented in \"unsigned long\", so that on larger\narchitectures we can handle objects that weigh more than 4GB.\n\nBut the interface defined in zlib.h to communicate with inflate/deflate\nlimits avail_in (how many bytes of input are we calling zlib with) and\navail_out (how many bytes of output from zlib are we ready to accept)\nfields effectively to 4GB by defining their type to be uInt.\n\nIn many places in our code, we allocate a large buffer (e.g. mmap'ing a\nlarge loose object file) and tell zlib its size by assigning the size to\navail_in field of the stream, but that will truncate the high octets of\nthe real size.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * There are a lot more places than just this call site, but I wanted to\n   send this out to get a quick sanity check by other people if the\n   approach of fixing these issues is sane.\n\n   Of course, we should be using streaming for more codepath, but that is\n   totally a separate issue. We would want our code to work correctly when\n   streaming is not used and your machine is beefy enough with zetabytes\n   of ram, and that is the topic of this patch.\n\n cache.h     |   11 +++++++++++\n sha1_file.c |   11 +++++++++--\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e11cf6a..b917ae5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -24,6 +24,17 @@ void git_inflate_init(z_streamp strm);\n void git_inflate_end(z_streamp strm);\n int git_inflate(z_streamp strm, int flush);\n \n+/*\n+ * avail_in and avail_out are counted in uInt, which typically limits\n+ * the size of the buffer we can use to 4GB when interacting with zlib\n+ * in a single call to inflate/deflate.\n+ */\n+#define ZLIB_BUF_MAX ((1UL << ((sizeof(uInt) * 8))) - 1)\n+static inline uInt zlib_buf_cap(unsigned long len)\n+{\n+\treturn (ZLIB_BUF_MAX < len ? ZLIB_BUF_MAX : len);\n+}\n+\n #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n #define DTYPE(de)\t((de)->d_type)\n #else\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 064a330..51236ab 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2429,6 +2429,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \tunsigned char parano_sha1[20];\n \tchar *filename;\n \tstatic char tmpfile[PATH_MAX];\n+\tunsigned long bytes_to_deflate;\n \n \tfilename = sha1_file_name(sha1);\n \tfd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);\n@@ -2454,14 +2455,20 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \tgit_SHA1_Update(&c, hdr, hdrlen);\n \n \t/* Then the data itself.. */\n+\tbytes_to_deflate = len;\n \tstream.next_in = (void *)buf;\n-\tstream.avail_in = len;\n+\tstream.avail_in = zlib_buf_cap(bytes_to_deflate);\n \tdo {\n \t\tunsigned char *in0 = stream.next_in;\n+\t\tsize_t consumed;\n+\n \t\tret = deflate(&stream, Z_FINISH);\n-\t\tgit_SHA1_Update(&c, in0, stream.next_in - in0);\n+\t\tconsumed = stream.next_in - in0;\n+\t\tgit_SHA1_Update(&c, in0, consumed);\n \t\tif (write_buffer(fd, compressed, stream.next_out - compressed) < 0)\n \t\t\tdie(\"unable to write sha1 file\");\n+\t\tbytes_to_deflate -= consumed;\n+\t\tstream.avail_in = zlib_buf_cap(bytes_to_deflate);\n \t\tstream.next_out = compressed;\n \t\tstream.avail_out = sizeof(compressed);\n \t} while (ret == Z_OK);\n"},{"id":"169684","messageId":"m2vcwgf80z.fsf@igel.home","threadId":"27584","inReplyTo":"7v1uz42liw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] sha1_file.c: zlib can only process 4GB at a time","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-06-08T19:16:44Z","receivedAt":"2011-06-08T19:16:44Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> +#define ZLIB_BUF_MAX ((1UL << ((sizeof(uInt) * 8))) - 1)\n\nThis is undefined if unsigned long has 32 bits.\n\n#define ZLIB_BUF_MAX ((uInt)-1)\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"169808","messageId":"7vzklqwyr5.fsf@alter.siamese.dyndns.org","threadId":"27584","inReplyTo":"7v1uz42liw.fsf@alter.siamese.dyndns.org","subject":"[PATCH] unpack-objects: zlib can only process 4GB at a time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-09T20:11:58Z","receivedAt":"2011-06-09T20:11:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This codepath tried to tell zlib that we have allocated a large enough\noutput buffer and expected a single call to inflate() to succeed, but\nzlib cannot expand into a buffer that is larger than 4GB at a time.\n\nIdeally we should be transferring a better cue between send-pack and\nreceive-pack (or fetch-pack and upload-pack) to tell that we would be\nbetter off not to explode into individual loose objects, but that is not\nan excuse to leave our use of zlib broken and fail to unpack large objects\nin a pack even on a beefy machine with enough memory.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/unpack-objects.c |    7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex f63973c..b208d6e 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -92,18 +92,22 @@ static void *get_data(unsigned long size)\n {\n \tz_stream stream;\n \tvoid *buf = xmalloc(size);\n+\tunsigned long bytes_to_inflate;\n \n \tmemset(&stream, 0, sizeof(stream));\n \n \tstream.next_out = buf;\n-\tstream.avail_out = size;\n+\tbytes_to_inflate = size;\n+\tstream.avail_out = zlib_buf_cap(bytes_to_inflate);\n \tstream.next_in = fill(1);\n \tstream.avail_in = len;\n \tgit_inflate_init(&stream);\n \n \tfor (;;) {\n+\t\tunsigned char *out0 = stream.next_out;\n \t\tint ret = git_inflate(&stream, 0);\n \t\tuse(len - stream.avail_in);\n+\t\tbytes_to_inflate -= (stream.next_out - out0);\n \t\tif (stream.total_out == size && ret == Z_STREAM_END)\n \t\t\tbreak;\n \t\tif (ret != Z_OK) {\n@@ -117,6 +121,7 @@ static void *get_data(unsigned long size)\n \t\t}\n \t\tstream.next_in = fill(1);\n \t\tstream.avail_in = len;\n+\t\tstream.avail_out = zlib_buf_cap(bytes_to_inflate);\n \t}\n \tgit_inflate_end(&stream);\n \treturn buf;\n-- \n1.7.6.rc1.118.ge175b4a\n"}]}