{"thread":{"id":"48573","subject":"[PATCH] packfile: Correct zlib buffer handling","startedAt":"2018-05-25T23:04:23Z","lastAt":"2018-06-13T01:04:47Z","messageCount":7,"participants":["Jeremy Linton","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"348569","messageId":"20180525225601.22575-1-lintonrjeremy@gmail.com","threadId":"48573","inReplyTo":null,"subject":"[PATCH] packfile: Correct zlib buffer handling","fromName":"Jeremy Linton","fromEmail":"lintonrjeremy@gmail.com","sentAt":"2018-05-25T22:56:01Z","receivedAt":"2018-05-25T23:04:23Z","isPatch":true,"sender":{"key":"lintonrjeremy@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1294586?v=4"},"body":"The buffer being passed to zlib includes a null terminator that\ngit needs to keep in place. unpack_compressed_entry() attempts to\ndetect the case that the source buffer hasn't been fully consumed\nby checking to see if the destination buffer has been over consumed.\n\nThis yields two problems, first a single byte overrun won't be detected\nproperly because the Z_STREAM_END will then be set, but the null\nterminator will have been overwritten. The other problem is that\nmore recent zlib patches have been poisoning the unconsumed portions\nof the buffers which also overwrites the null, while correctly\nreturning length and status.\n\nLets rely on the fact that the source buffer will only be fully\nconsumed when the when the destination buffer is inflated to the\ncorrect size. We can do this by passing zlib the correct buffer size\nand properly checking the return status. The latter check actually\nalready exists if the buffer size is correct.\n\nSigned-off-by: Jeremy Linton \n---\n packfile.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 7c1a2519f..245eb3204 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \t\treturn NULL;\n \tmemset(&stream, 0, sizeof(stream));\n \tstream.next_out = buffer;\n-\tstream.avail_out = size + 1;\n+\tstream.avail_out = size;\n \n \tgit_inflate_init(&stream);\n \tdo {\n@@ -1424,7 +1424,7 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n \t\tif (!stream.avail_out)\n-\t\t\tbreak; /* the payload is larger than it should be */\n+\t\t\tbreak; /* done, st indicates if source fully consumed */\n \t\tcurpos += stream.next_in - in;\n \t} while (st == Z_OK || st == Z_BUF_ERROR);\n \tgit_inflate_end(&stream);\n-- \n2.13.6\n\n"},{"id":"348579","messageId":"CACsJy8C4zNZAe6Vw-mz8r8HueYdQKs4ZhMOQYX0v7ypfHdmShg@mail.gmail.com","threadId":"48573","inReplyTo":"20180525225601.22575-1-lintonrjeremy@gmail.com","subject":"Re: [PATCH] packfile: Correct zlib buffer handling","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-26T05:51:55Z","receivedAt":"2018-05-26T05:52:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, May 26, 2018 at 12:56 AM, Jeremy Linton wrote:\n> @@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct packed_git *p,\n> return NULL;\n> memset(&stream, 0, sizeof(stream));\n> stream.next_out = buffer;\n> - stream.avail_out = size + 1;\n> + stream.avail_out = size;\n\nYou may want to include in your commit message a reference to\n39eea7bdd9 (Fix incorrect error check while reading deflated pack data\n- 2009-10-21) which adds this plus one with a fascinating story\nbehind.\n-- \nDuy\n"},{"id":"348612","messageId":"xmqq8t86m0m3.fsf@gitster-ct.c.googlers.com","threadId":"48573","inReplyTo":"CACsJy8C4zNZAe6Vw-mz8r8HueYdQKs4ZhMOQYX0v7ypfHdmShg@mail.gmail.com","subject":"Re: [PATCH] packfile: Correct zlib buffer handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-26T23:57:24Z","receivedAt":"2018-05-26T23:57:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sat, May 26, 2018 at 12:56 AM, Jeremy Linton wrote:\n>> @@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct packed_git *p,\n>> return NULL;\n>> memset(&stream, 0, sizeof(stream));\n>> stream.next_out = buffer;\n>> - stream.avail_out = size + 1;\n>> + stream.avail_out = size;\n>\n> You may want to include in your commit message a reference to\n> 39eea7bdd9 (Fix incorrect error check while reading deflated pack data\n> - 2009-10-21) which adds this plus one with a fascinating story\n> behind.\n\nA bit puzzled---are you saying that this recent patch breaks the old\nfix and must be done in some other way?\n"},{"id":"348617","messageId":"CACsJy8CNmDkxzhkdfwjVHjGF4JeUzzPHGnp1x647NxU-GxkxvA@mail.gmail.com","threadId":"48573","inReplyTo":"xmqq8t86m0m3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] packfile: Correct zlib buffer handling","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-27T05:02:05Z","receivedAt":"2018-05-27T05:33:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, May 27, 2018 at 1:57 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> On Sat, May 26, 2018 at 12:56 AM, Jeremy Linton wrote:\n>>> @@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct packed_git *p,\n>>> return NULL;\n>>> memset(&stream, 0, sizeof(stream));\n>>> stream.next_out = buffer;\n>>> - stream.avail_out = size + 1;\n>>> + stream.avail_out = size;\n>>\n>> You may want to include in your commit message a reference to\n>> 39eea7bdd9 (Fix incorrect error check while reading deflated pack data\n>> - 2009-10-21) which adds this plus one with a fascinating story\n>> behind.\n>\n> A bit puzzled---are you saying that this recent patch breaks the old\n> fix and must be done in some other way?\n\nNo. I actually wanted to answer that question when I tried to track\ndown the commit that adds \" + 1\" but I did not spend enough time to\nunderstand the old problem. I guess your puzzle means you didn't think\nit would break anything, which is good.\n-- \nDuy\n"},{"id":"348628","messageId":"xmqqsh6dl3gg.fsf@gitster-ct.c.googlers.com","threadId":"48573","inReplyTo":"CACsJy8CNmDkxzhkdfwjVHjGF4JeUzzPHGnp1x647NxU-GxkxvA@mail.gmail.com","subject":"Re: [PATCH] packfile: Correct zlib buffer handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-27T11:53:35Z","receivedAt":"2018-05-27T11:53:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sun, May 27, 2018 at 1:57 AM, Junio C Hamano wrote:\n>> Duy Nguyen writes:\n>>\n>>> On Sat, May 26, 2018 at 12:56 AM, Jeremy Linton wrote:\n>>>> @@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct packed_git *p,\n>>>> return NULL;\n>>>> memset(&stream, 0, sizeof(stream));\n>>>> stream.next_out = buffer;\n>>>> - stream.avail_out = size + 1;\n>>>> + stream.avail_out = size;\n>>>\n>>> You may want to include in your commit message a reference to\n>>> 39eea7bdd9 (Fix incorrect error check while reading deflated pack data\n>>> - 2009-10-21) which adds this plus one with a fascinating story\n>>> behind.\n>>\n>> A bit puzzled---are you saying that this recent patch breaks the old\n>> fix and must be done in some other way?\n>\n> No. I actually wanted to answer that question when I tried to track\n> down the commit that adds \" + 1\" but I did not spend enough time to\n> understand the old problem. I guess your puzzle means you didn't think\n> it would break anything, which is good.\n\nNo it merely means I am puzzled how the posted patch that goes\ndirectly opposite to what an earlier \"fix\" did is a correct solution\nto anything X-<.\n"},{"id":"348642","messageId":"xmqqk1rolcxg.fsf@gitster-ct.c.googlers.com","threadId":"48573","inReplyTo":"xmqqsh6dl3gg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] packfile: Correct zlib buffer handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-28T02:41:15Z","receivedAt":"2018-05-28T02:43:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Duy Nguyen writes:\n>\n>> On Sun, May 27, 2018 at 1:57 AM, Junio C Hamano wrote:\n>>> Duy Nguyen writes:\n>>>\n>>>> On Sat, May 26, 2018 at 12:56 AM, Jeremy Linton wrote:\n>>>>> @@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct packed_git *p,\n>>>>> return NULL;\n>>>>> memset(&stream, 0, sizeof(stream));\n>>>>> stream.next_out = buffer;\n>>>>> - stream.avail_out = size + 1;\n>>>>> + stream.avail_out = size;\n>>>>\n>>>> You may want to include in your commit message a reference to\n>>>> 39eea7bdd9 (Fix incorrect error check while reading deflated pack data\n>>>> - 2009-10-21) which adds this plus one with a fascinating story\n>>>> behind.\n>>>\n>>> A bit puzzled---are you saying that this recent patch breaks the old\n>>> fix and must be done in some other way?\n>>\n>> No. I actually wanted to answer that question when I tried to track\n>> down the commit that adds \" + 1\" but I did not spend enough time to\n>> understand the old problem. I guess your puzzle means you didn't think\n>> it would break anything, which is good.\n>\n> No it merely means I am puzzled how the posted patch that goes\n> directly opposite to what an earlier \"fix\" did is a correct solution\n> to anything X-<.\n\nSpecifically, I was worried about this assertion:\n\n Lets rely on the fact that the source buffer will only be fully\n consumed when the when the destination buffer is inflated to the\n correct size.\n\nwhich I think is the exact bad thinking that caused troubles for us\nin the past; isn't the explanation in 456cdf6e (\"Fix loose object\nuncompression check.\", 2007-03-19) relevant here?\n\n-\tstream.avail_out = size + 1;\n+\tstream.avail_out = size;\n\t...\n \t\tstream.next_in = in;\n \t\tst = git_inflate(&stream, Z_FINISH);\n \t\tif (!stream.avail_out)\n-\t\t\tbreak; /* the payload is larger than it should be */\n+\t\t\tbreak; /* done, st indicates if source fully consumed */\n \t\tcurpos += stream.next_in - in;\n \t} while (st == Z_OK || st == Z_BUF_ERROR);\n \tgit_inflate_end(&stream);\n \tif ((st != Z_STREAM_END) || stream.total_out != size) {\n \t\tfree(buffer);\n \t\treturn NULL;\n \t}\n\nWith minimum stream.avail_out without slack, when !avail_out, i.e.\nwhen we fully filled the output buffer, it could be that we had\ncorrect input that deflates to the correct size, in which case we\nare happy---st would say Z_STREAM_END, we would leave the loop\nbecause it is neither OK nor BUF_ERROR, and total_out would report\nthe size we expected. Or the input zlib stream may have ended with\nbytes that express \"this concludes the stream\", and the input bytes\nbefore that was sufficient to construct the original payload fully,\nand we may have just fed the bytes before that \"this concludes the\nstream\" to git_inflate().\n\nIn such a case, we haven't consumed all the avail_in. We may\nalready have all the correct output, i.e. !avail_out, but because we\nhaven't consumed the \"this concludes the stream\", st is not\nSTREAM_END in such a case. \n\nOur existing while() loop, with one-byte slack in avail_out, would\nhave let us continue and the next iteration of the loop would have\nconsumed the input without producing any more output (i.e. avail_out\nwould have been left to 1 in both of these final two rounds) and we\nwould have exited the loop. After calling inflate_end(), we would\nhave noticed STREAM_END and correct size and we would have been\nhappy.\n\nThe updated code would handle this latter case rather badly, no? We\nleave the loop early, notice st is not STREAM_END, and be very\nunhappy, because this patch did not give us to consume the very end\nof the input stream and left the loop early.\n\n>> This yields two problems, first a single byte overrun won't be detected\n>> properly because the Z_STREAM_END will then be set, but the null\n>> terminator will have been overwritten.\n\nBecause we compare total_out and size at the end, we would detect it\nas an error in this function, no? Then zlib overwriting NUL would\nnot be a problem, as we would free the buffer and return NULL, no?\n\n>> The other problem is that\n>> more recent zlib patches have been poisoning the unconsumed portions\n>> of the buffers which also overwrites the null, while correctly\n>> returning length and status.\n\nIsn't that a bug in zlib, though? Or do they do that deliberately?\n\nI think a workaround with lower impact would be to manually restore\nNUL at the end of the buffer.\n\n"},{"id":"350037","messageId":"CAEFTgiw03eDXAobWrP4J_CM1uGHoAEUkguV1_agAiNkssCpwyg@mail.gmail.com","threadId":"48573","inReplyTo":"xmqqk1rolcxg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] packfile: Correct zlib buffer handling","fromName":"Jeremy Linton","fromEmail":"lintonrjeremy@gmail.com","sentAt":"2018-06-13T01:04:38Z","receivedAt":"2018-06-13T01:04:47Z","isPatch":true,"sender":{"key":"lintonrjeremy@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1294586?v=4"},"body":"Hi,\n\nSorry about the delay here (bit of a mix-up and didn't reply to the list).\n\n(see inline )\n\nOn Sun, May 27, 2018 at 9:41 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Duy Nguyen writes:\n>>\n>>> On Sun, May 27, 2018 at 1:57 AM, Junio C Hamano wrote:\n\n(trimming)\n\n>\n> Specifically, I was worried about this assertion:\n>\n> Lets rely on the fact that the source buffer will only be fully\n> consumed when the when the destination buffer is inflated to the\n> correct size.\n>\n> which I think is the exact bad thinking that caused troubles for us\n> in the past; isn't the explanation in 456cdf6e (\"Fix loose object\n> uncompression check.\", 2007-03-19) relevant here?\n>\n> - stream.avail_out = size + 1;\n> + stream.avail_out = size;\n> ...\n> stream.next_in = in;\n> st = git_inflate(&stream, Z_FINISH);\n> if (!stream.avail_out)\n> - break; /* the payload is larger than it should be */\n> + break; /* done, st indicates if source fully consumed */\n> curpos += stream.next_in - in;\n> } while (st == Z_OK || st == Z_BUF_ERROR);\n> git_inflate_end(&stream);\n> if ((st != Z_STREAM_END) || stream.total_out != size) {\n> free(buffer);\n> return NULL;\n> }\n>\n> With minimum stream.avail_out without slack, when !avail_out, i.e.\n> when we fully filled the output buffer, it could be that we had\n> correct input that deflates to the correct size, in which case we\n> are happy---st would say Z_STREAM_END, we would leave the loop\n> because it is neither OK nor BUF_ERROR, and total_out would report\n> the size we expected. Or the input zlib stream may have ended with\n> bytes that express \"this concludes the stream\", and the input bytes\n> before that was sufficient to construct the original payload fully,\n> and we may have just fed the bytes before that \"this concludes the\n> stream\" to git_inflate().\n>\n> In such a case, we haven't consumed all the avail_in. We may\n> already have all the correct output, i.e. !avail_out, but because we\n> haven't consumed the \"this concludes the stream\", st is not\n> STREAM_END in such a case.\n\nIf I understand correctly your concerned the avail_in is longer than\nwhat is required to fill the output buffer..\n\nI'm fairly sure that won't result in a Z_STREAM_END, as you rightfully\npoint out, but the loop _will_ terminate due to the output buffer\nbeing full and then since its not Z_STREAM_END the\nunpack_compressed_entry fails, as it should.\n\n>\n> Our existing while() loop, with one-byte slack in avail_out, would\n> have let us continue and the next iteration of the loop would have\n> consumed the input without producing any more output (i.e. avail_out\n> would have been left to 1 in both of these final two rounds) and we\n> would have exited the loop. After calling inflate_end(), we would\n> have noticed STREAM_END and correct size and we would have been\n> happy.\n\nYour assuming that zlib will terminate with an error, but a fully\ndecompressed buffer, because it hasn't consumed the entire input\nbuffer. I don't think that is how it works (its not how the\ndocumentation is written, nor the bits of code i've looked at seem to\nwork, which granted i'm not a zlib maintainer).\n\n\n>\n> The updated code would handle this latter case rather badly, no? We\n> leave the loop early, notice st is not STREAM_END, and be very\n> unhappy, because this patch did not give us to consume the very end\n> of the input stream and left the loop early.\n\nYour correct if the above case is a valid zlib behavior then there\nwould be a problem. But, I don't think the termination is dicated by\ninsufficient output space until there is an attempt to utilize that\nspace.\n\n\n>\n>>> This yields two problems, first a single byte overrun won't be detected\n>>> properly because the Z_STREAM_END will then be set, but the null\n>>> terminator will have been overwritten.\n>\n> Because we compare total_out and size at the end, we would detect it\n> as an error in this function, no? Then zlib overwriting NUL would\n> not be a problem, as we would free the buffer and return NULL, no?\n>\n>>> The other problem is that\n>>> more recent zlib patches have been poisoning the unconsumed portions\n>>> of the buffers which also overwrites the null, while correctly\n>>> returning length and status.\n>\n> Isn't that a bug in zlib, though? Or do they do that deliberately?\n>\n> I think a workaround with lower impact would be to manually restore\n> NUL at the end of the buffer.\n\nI agree, just resetting the NULL its likely safer, and I will repost a\npatch soon which if nothing else makes git more robust to errant zlib\nbehavior.\n"}]}