git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH v2] packfile: Correct zlib buffer handling
@ 2018-06-13 14:22 Jeremy Linton
  2018-06-13 17:21 ` Eric Sunshine
  0 siblings, 1 reply; 7+ messages in thread
From: Jeremy Linton @ 2018-06-13 14:22 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, gitster, Jeremy Linton

The buffer being passed to zlib includes a null 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 null, while correctly returning length and status.

Let's replace the null at the end of the buffer to assure that
if its been overwritten by zlib it doesn't result in problems for
git.

Signed-off-by: Jeremy Linton <lintonrjeremy@gmail.com>
---
 packfile.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/packfile.c b/packfile.c
index 7c1a2519f..8db5d90ca 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1433,6 +1433,8 @@ static void *unpack_compressed_entry(struct packed_git *p,
 		return NULL;
 	}
 
+	buffer[size] = 0; /* assure that the buffer is still terminated */
+
 	return buffer;
 }
 
-- 
2.13.6


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] packfile: Correct zlib buffer handling
  2018-06-13 14:22 [PATCH v2] packfile: Correct zlib buffer handling Jeremy Linton
@ 2018-06-13 17:21 ` Eric Sunshine
  2018-06-13 18:32   ` Junio C Hamano
  2018-06-13 18:38   ` Junio C Hamano
  0 siblings, 2 replies; 7+ messages in thread
From: Eric Sunshine @ 2018-06-13 17:21 UTC (permalink / raw)
  To: Jeremy Linton; +Cc: Git List, Jonathan Tan, Junio C Hamano

A couple comments if you happen to re-roll...

On Wed, Jun 13, 2018 at 10:22 AM Jeremy Linton <lintonrjeremy@gmail.com> wrote:
> The buffer being passed to zlib includes a null terminator that

On this project, the character mnemonic "NUL" is typically used, not
"null" or "NULL" (which is typically reserved for pointers), so:
s/null/NUL/g

> 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 null, while correctly returning length and status.
>
> Let's replace the null at the end of the buffer to assure that
> if its been overwritten by zlib it doesn't result in problems for
> git.
>
> Signed-off-by: Jeremy Linton <lintonrjeremy@gmail.com>
> ---
> diff --git a/packfile.c b/packfile.c
> @@ -1433,6 +1433,8 @@ static void *unpack_compressed_entry(struct packed_git *p,
> +       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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] packfile: Correct zlib buffer handling
  2018-06-13 17:21 ` Eric Sunshine
@ 2018-06-13 18:32   ` Junio C Hamano
  2018-06-13 18:40     ` Eric Sunshine
  2018-06-13 18:38   ` Junio C Hamano
  1 sibling, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2018-06-13 18:32 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Jeremy Linton, Git List, Jonathan Tan

Eric Sunshine <sunshine@sunshineco.com> writes:

> A couple comments if you happen to re-roll...
>
> On Wed, Jun 13, 2018 at 10:22 AM Jeremy Linton <lintonrjeremy@gmail.com> wrote:
>> The buffer being passed to zlib includes a null terminator that
>
> On this project, the character mnemonic "NUL" is typically used, not
> "null" or "NULL" (which is typically reserved for pointers), so:
> s/null/NUL/g

Correct but I did not think it is a per-project preference; rather,
"NUL is the name of the byte" is universal ;-)

>> diff --git a/packfile.c b/packfile.c
>> @@ -1433,6 +1433,8 @@ static void *unpack_compressed_entry(struct packed_git *p,
>> +       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.

Both are sensible suggestions.  Thanks for making them.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] packfile: Correct zlib buffer handling
  2018-06-13 17:21 ` Eric Sunshine
  2018-06-13 18:32   ` Junio C Hamano
@ 2018-06-13 18:38   ` Junio C Hamano
  2018-06-13 21:48     ` Jeremy Linton
  1 sibling, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2018-06-13 18:38 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Jeremy Linton, Git List, Jonathan Tan

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] packfile: Correct zlib buffer handling
  2018-06-13 18:32   ` Junio C Hamano
@ 2018-06-13 18:40     ` Eric Sunshine
  0 siblings, 0 replies; 7+ messages in thread
From: Eric Sunshine @ 2018-06-13 18:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeremy Linton, Git List, Jonathan Tan

On Wed, Jun 13, 2018 at 2:32 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On this project, the character mnemonic "NUL" is typically used, not
> > "null" or "NULL" (which is typically reserved for pointers), so:
> > s/null/NUL/g
>
> Correct but I did not think it is a per-project preference; rather,
> "NUL is the name of the byte" is universal ;-)

Yes, the _mnemonic_ NUL is universal, but the character itself is
sometimes named or described as the "null character". I was just being
pedantic when "this project", by which I meant that we (on this
project) prefer the mnemonic "NUL" over longhand "null character",
whereas other projects may perhaps prefer "null character" or not
care.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] packfile: Correct zlib buffer handling
  2018-06-13 18:38   ` Junio C Hamano
@ 2018-06-13 21:48     ` Jeremy Linton
  2018-06-13 21:56       ` Junio C Hamano
  0 siblings, 1 reply; 7+ messages in thread
From: Jeremy Linton @ 2018-06-13 21:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Eric Sunshine, Git List, Jonathan Tan

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?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] packfile: Correct zlib buffer handling
  2018-06-13 21:48     ` Jeremy Linton
@ 2018-06-13 21:56       ` Junio C Hamano
  0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2018-06-13 21:56 UTC (permalink / raw)
  To: Jeremy Linton; +Cc: Eric Sunshine, Git List, Jonathan Tan

Jeremy Linton <lintonrjeremy@gmail.com> writes:

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

If you are happy with what you saw above, I can just make it no
longer "tentative" and use it as-is, which would save time for both
of us ;-)

Thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-13 14:22 [PATCH v2] packfile: Correct zlib buffer handling Jeremy Linton
2018-06-13 17:21 ` Eric Sunshine
2018-06-13 18:32   ` Junio C Hamano
2018-06-13 18:40     ` Eric Sunshine
2018-06-13 18:38   ` Junio C Hamano
2018-06-13 21:48     ` Jeremy Linton
2018-06-13 21:56       ` Junio C Hamano

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox