git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Jeremy Linton <lintonrjeremy@gmail.com>,
	Git List <git@vger.kernel.org>,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v2] packfile: Correct zlib buffer handling
Date: Wed, 13 Jun 2018 11:38:13 -0700	[thread overview]
Message-ID: <xmqqtvq65y7e.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <CAPig+cQ1s7QFjEFrOHMYZR8qja5yTjV5D3ksUXXqFL61YthA3g@mail.gmail.com> (Eric Sunshine's message of "Wed, 13 Jun 2018 13:21:03 -0400")

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


  parent reply	other threads:[~2018-06-13 18:38 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2018-06-13 21:48     ` Jeremy Linton
2018-06-13 21:56       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqtvq65y7e.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=lintonrjeremy@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).