{"thread":{"id":"48705","subject":"[PATCH v2] packfile: Correct zlib buffer handling","startedAt":"2018-06-13T14:22:33Z","lastAt":"2018-06-13T21:56:40Z","messageCount":7,"participants":["Jeremy Linton","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"350057","messageId":"20180613142207.14385-1-lintonrjeremy@gmail.com","threadId":"48705","inReplyTo":null,"subject":"[PATCH v2] packfile: Correct zlib buffer handling","fromName":"Jeremy Linton","fromEmail":"lintonrjeremy@gmail.com","sentAt":"2018-06-13T14:22:07Z","receivedAt":"2018-06-13T14:22:33Z","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 causes a problem, that more recent zlib patches have been\npoisoning the unconsumed portions of the buffer which overwrites\nthe null, while correctly returning length and status.\n\nLet's replace the null at the end of the buffer to assure that\nif its been overwritten by zlib it doesn't result in problems for\ngit.\n\nSigned-off-by: Jeremy Linton \n---\n packfile.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/packfile.c b/packfile.c\nindex 7c1a2519f..8db5d90ca 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1433,6 +1433,8 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \t\treturn NULL;\n \t}\n \n+\tbuffer[size] = 0; /* assure that the buffer is still terminated */\n+\n \treturn buffer;\n }\n \n-- \n2.13.6\n\n"},{"id":"350068","messageId":"CAPig+cQ1s7QFjEFrOHMYZR8qja5yTjV5D3ksUXXqFL61YthA3g@mail.gmail.com","threadId":"48705","inReplyTo":"20180613142207.14385-1-lintonrjeremy@gmail.com","subject":"Re: [PATCH v2] packfile: Correct zlib buffer handling","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-13T17:21:03Z","receivedAt":"2018-06-13T17:21:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"A couple comments if you happen to re-roll...\n\nOn Wed, Jun 13, 2018 at 10:22 AM Jeremy Linton wrote:\n> The buffer being passed to zlib includes a null terminator that\n\nOn this project, the character mnemonic \"NUL\" is typically used, not\n\"null\" or \"NULL\" (which is typically reserved for pointers), so:\ns/null/NUL/g\n\n> git needs to keep in place. unpack_compressed_entry() attempts to\n> detect the case that the source buffer hasn't been fully consumed\n> by checking to see if the destination buffer has been over consumed.\n>\n> This causes a problem, that more recent zlib patches have been\n> poisoning the unconsumed portions of the buffer which overwrites\n> the null, while correctly returning length and status.\n>\n> Let's replace the null at the end of the buffer to assure that\n> if its been overwritten by zlib it doesn't result in problems for\n> git.\n>\n> Signed-off-by: Jeremy Linton \n> ---\n> diff --git a/packfile.c b/packfile.c\n> @@ -1433,6 +1433,8 @@ static void *unpack_compressed_entry(struct packed_git *p,\n> + buffer[size] = 0; /* assure that the buffer is still terminated */\n\nI think we normally use '\\0' for NUL on this project rather than simply 0.\n\nThe comment is also effectively pure noise since it merely repeats\nwhat the code already states clearly (especially when the code says\n\"buffer[size] = '\\0';\"), so dropping the comment altogether would be\nreasonable.\n"},{"id":"350075","messageId":"xmqqy3fi5ygz.fsf@gitster-ct.c.googlers.com","threadId":"48705","inReplyTo":"CAPig+cQ1s7QFjEFrOHMYZR8qja5yTjV5D3ksUXXqFL61YthA3g@mail.gmail.com","subject":"Re: [PATCH v2] packfile: Correct zlib buffer handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-13T18:32:28Z","receivedAt":"2018-06-13T18:32:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> A couple comments if you happen to re-roll...\n>\n> On Wed, Jun 13, 2018 at 10:22 AM Jeremy Linton wrote:\n>> The buffer being passed to zlib includes a null terminator that\n>\n> On this project, the character mnemonic \"NUL\" is typically used, not\n> \"null\" or \"NULL\" (which is typically reserved for pointers), so:\n> s/null/NUL/g\n\nCorrect but I did not think it is a per-project preference; rather,\n\"NUL is the name of the byte\" is universal ;-)\n\n>> diff --git a/packfile.c b/packfile.c\n>> @@ -1433,6 +1433,8 @@ static void *unpack_compressed_entry(struct packed_git *p,\n>> + buffer[size] = 0; /* assure that the buffer is still terminated */\n>\n> I think we normally use '\\0' for NUL on this project rather than simply 0.\n>\n> The comment is also effectively pure noise since it merely repeats\n> what the code already states clearly (especially when the code says\n> \"buffer[size] = '\\0';\"), so dropping the comment altogether would be\n> reasonable.\n\nBoth are sensible suggestions. Thanks for making them.\n\n"},{"id":"350077","messageId":"xmqqtvq65y7e.fsf@gitster-ct.c.googlers.com","threadId":"48705","inReplyTo":"CAPig+cQ1s7QFjEFrOHMYZR8qja5yTjV5D3ksUXXqFL61YthA3g@mail.gmail.com","subject":"Re: [PATCH v2] packfile: Correct zlib buffer handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-13T18:38:13Z","receivedAt":"2018-06-13T18:38:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> + buffer[size] = 0; /* assure that the buffer is still terminated */\n>\n> I think we normally use '\\0' for NUL on this project rather than simply 0.\n>\n> The comment is also effectively pure noise since it merely repeats\n> what the code already states clearly (especially when the code says\n> \"buffer[size] = '\\0';\"), so dropping the comment altogether would be\n> reasonable.\n\nActually, I'd prefer to have comment there, but not about \"what this\nline does\" (which is useless, as you pointed out) but about \"why do\nwe do this seemingly redundant clearing\".\n\nHere is what I tentatively came up with.\n\n-- >8 --\nFrom: Jeremy Linton \nDate: Wed, 13 Jun 2018 09:22:07 -0500\nSubject: [PATCH] packfile: correct zlib buffer handling\n\nThe buffer being passed to zlib includes a NUL terminator that git\nneeds to keep in place. unpack_compressed_entry() attempts to detect\nthe case that the source buffer hasn't been fully consumed by\nchecking to see if the destination buffer has been over consumed.\n\nThis causes a problem, that more recent zlib patches have been\npoisoning the unconsumed portions of the buffer which overwrites\nthe NUL byte, while correctly returning length and status.\n\nLet's place the NUL at the end of the buffer after inflate returns\nto assure that it doesn't result in problems for git even if its\nbeen overwritten by zlib.\n\nSigned-off-by: Jeremy Linton \nSigned-off-by: Junio C Hamano \n---\n packfile.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/packfile.c b/packfile.c\nindex 4a5fe7ab18..d555699217 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1422,6 +1422,9 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \t\treturn NULL;\n \t}\n \n+\t/* versions of zlib can clobber unconsumed portion of outbuf */\n+\tbuffer[size] = '\\0';\n+\n \treturn buffer;\n }\n \n-- \n2.18.0-rc1-1-g6f333ff2fb\n\n"},{"id":"350078","messageId":"CAPig+cR5kMissKxsA8-0kvZGPmzm=KiPcyhhH9kQBR8EN-GsLg@mail.gmail.com","threadId":"48705","inReplyTo":"xmqqy3fi5ygz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] packfile: Correct zlib buffer handling","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-13T18:40:36Z","receivedAt":"2018-06-13T18:40:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 13, 2018 at 2:32 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > On this project, the character mnemonic \"NUL\" is typically used, not\n> > \"null\" or \"NULL\" (which is typically reserved for pointers), so:\n> > s/null/NUL/g\n>\n> Correct but I did not think it is a per-project preference; rather,\n> \"NUL is the name of the byte\" is universal ;-)\n\nYes, the _mnemonic_ NUL is universal, but the character itself is\nsometimes named or described as the \"null character\". I was just being\npedantic when \"this project\", by which I meant that we (on this\nproject) prefer the mnemonic \"NUL\" over longhand \"null character\",\nwhereas other projects may perhaps prefer \"null character\" or not\ncare.\n"},{"id":"350094","messageId":"CAEFTgix0pasg38cb2_qrN+iPgVuq-BX9b7q+qC5Ep9iFbS=Szg@mail.gmail.com","threadId":"48705","inReplyTo":"xmqqtvq65y7e.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] packfile: Correct zlib buffer handling","fromName":"Jeremy Linton","fromEmail":"lintonrjeremy@gmail.com","sentAt":"2018-06-13T21:48:12Z","receivedAt":"2018-06-13T21:48:18Z","isPatch":true,"sender":{"key":"lintonrjeremy@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1294586?v=4"},"body":"Hi,\n\nOn Wed, Jun 13, 2018 at 1:38 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>>> + buffer[size] = 0; /* assure that the buffer is still terminated */\n>>\n>> I think we normally use '\\0' for NUL on this project rather than simply 0.\n>>\n>> The comment is also effectively pure noise since it merely repeats\n>> what the code already states clearly (especially when the code says\n>> \"buffer[size] = '\\0';\"), so dropping the comment altogether would be\n>> reasonable.\n>\n> Actually, I'd prefer to have comment there, but not about \"what this\n> line does\" (which is useless, as you pointed out) but about \"why do\n> we do this seemingly redundant clearing\".\n>\n> Here is what I tentatively came up with.\n>\n> -- >8 --\n> From: Jeremy Linton \n> Date: Wed, 13 Jun 2018 09:22:07 -0500\n> Subject: [PATCH] packfile: correct zlib buffer handling\n>\n> The buffer being passed to zlib includes a NUL terminator that git\n> needs to keep in place. unpack_compressed_entry() attempts to detect\n> the case that the source buffer hasn't been fully consumed by\n> checking to see if the destination buffer has been over consumed.\n>\n> This causes a problem, that more recent zlib patches have been\n> poisoning the unconsumed portions of the buffer which overwrites\n> the NUL byte, while correctly returning length and status.\n>\n> Let's place the NUL at the end of the buffer after inflate returns\n> to assure that it doesn't result in problems for git even if its\n> been overwritten by zlib.\n>\n> Signed-off-by: Jeremy Linton \n> Signed-off-by: Junio C Hamano \n> ---\n> packfile.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/packfile.c b/packfile.c\n> index 4a5fe7ab18..d555699217 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1422,6 +1422,9 @@ static void *unpack_compressed_entry(struct packed_git *p,\n> return NULL;\n> }\n>\n> + /* versions of zlib can clobber unconsumed portion of outbuf */\n> + buffer[size] = '\\0';\n> +\n> return buffer;\n> }\n>\n> --\n> 2.18.0-rc1-1-g6f333ff2fb\n\nThis is all fine with me, the original comment was an attempt to\nindicate that the original null may not have been there anymore too..\n\nShall I resubmit it as above, or can it be picked up like this?\n"},{"id":"350095","messageId":"xmqqk1r25p0s.fsf@gitster-ct.c.googlers.com","threadId":"48705","inReplyTo":"CAEFTgix0pasg38cb2_qrN+iPgVuq-BX9b7q+qC5Ep9iFbS=Szg@mail.gmail.com","subject":"Re: [PATCH v2] packfile: Correct zlib buffer handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-13T21:56:35Z","receivedAt":"2018-06-13T21:56:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeremy Linton writes:\n\n>> Here is what I tentatively came up with.\n>>\n>> -- >8 --\n>> From: Jeremy Linton \n>> Date: Wed, 13 Jun 2018 09:22:07 -0500\n>> Subject: [PATCH] packfile: correct zlib buffer handling\n>>\n>> The buffer being passed to zlib includes a NUL terminator that git\n>> ...\n>> +\n>> return buffer;\n>> }\n>>\n>> --\n>> 2.18.0-rc1-1-g6f333ff2fb\n>\n> This is all fine with me, the original comment was an attempt to\n> indicate that the original null may not have been there anymore too..\n>\n> Shall I resubmit it as above, or can it be picked up like this?\n\nIf you are happy with what you saw above, I can just make it no\nlonger \"tentative\" and use it as-is, which would save time for both\nof us ;-)\n\nThanks.\n"}]}