Re: [PATCH v2] packfile: Correct zlib buffer handling

[Jeremy Linton <lintonrjeremy@gmail.com>]

Hi,

On Wed, Jun 13, 2018 at 1:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> +       buffer[size] = 0; /* assure that the buffer is still terminated */
>>
>> I think we normally use '\0' for NUL on this project rather than simply 0.
>>
>> The comment is also effectively pure noise since it merely repeats
>> what the code already states clearly (especially when the code says
>> "buffer[size] = '\0';"), so dropping the comment altogether would be
>> reasonable.
>
> Actually, I'd prefer to have comment there, but not about "what this
> line does" (which is useless, as you pointed out) but about "why do
> we do this seemingly redundant clearing".
>
> Here is what I tentatively came up with.
>
> -- >8 --
> From: Jeremy Linton <lintonrjeremy@gmail.com>
> Date: Wed, 13 Jun 2018 09:22:07 -0500
> Subject: [PATCH] packfile: correct zlib buffer handling
>
> The buffer being passed to zlib includes a NUL terminator that git
> needs to keep in place. unpack_compressed_entry() attempts to detect
> the case that the source buffer hasn't been fully consumed by
> checking to see if the destination buffer has been over consumed.
>
> This causes a problem, that more recent zlib patches have been
> poisoning the unconsumed portions of the buffer which overwrites
> the NUL byte, while correctly returning length and status.
>
> Let's place the NUL at the end of the buffer after inflate returns
> to assure that it doesn't result in problems for git even if its
> been overwritten by zlib.
>
> Signed-off-by: Jeremy Linton <lintonrjeremy@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  packfile.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/packfile.c b/packfile.c
> index 4a5fe7ab18..d555699217 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1422,6 +1422,9 @@ static void *unpack_compressed_entry(struct packed_git *p,
>                 return NULL;
>         }
>
> +       /* versions of zlib can clobber unconsumed portion of outbuf */
> +       buffer[size] = '\0';
> +
>         return buffer;
>  }
>
> --
> 2.18.0-rc1-1-g6f333ff2fb

This is all fine with me, the original comment was an attempt to
indicate that the original null may not have been there anymore too..

Shall I resubmit it as above, or can it be picked up like this?