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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 13, 2018, 18:38 UTC
Message-ID: <xmqqtvq65y7e.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAPig+cQ1s7QFjEFrOHMYZR8qja5yTjV5D3ksUXXqFL61YthA3g@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 8 quoted lines

>> +       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