[PATCH 0/4] Fix various integer overflows

[PATCH 0/4] Fix various integer overflows

[Ilari Liusvaara]

Fix integer overflows in patch_delta(), unpack_sha1_rest() and
unpack_compressed_entry().

These at least can cause git to segfault, possibly worse. Operations
that cause integer overflow are not possible to do (even whole virtual
memory space would not be sufficient), so die() instead.

Ilari Liusvaara (4):
  Add xmallocz()
  Fix integer overflow in patch_delta()
  Fix integer overflow in unpack_sha1_rest()
  Fix integer overflow in unpack_compressed_entry()

 git-compat-util.h |    1 +
 patch-delta.c     |    3 +--
 sha1_file.c       |    5 ++---
 wrapper.c         |   12 +++++++++++-
 4 files changed, 15 insertions(+), 6 deletions(-)

[PATCH 1/4] Add xmallocz()

[Ilari Liusvaara]

Add routine for allocating NUL-terminated memory block without risking
integer overflow in addition of +1 for NUL byte.

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 git-compat-util.h |    1 +
 wrapper.c         |   12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 620a7c6..a3c4537 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -348,6 +348,7 @@ extern void release_pack_memory(size_t, int);
 
 extern char *xstrdup(const char *str);
 extern void *xmalloc(size_t size);
+extern void *xmallocz(size_t size);
 extern void *xmemdupz(const void *data, size_t len);
 extern char *xstrndup(const char *str, size_t len);
 extern void *xrealloc(void *ptr, size_t size);
diff --git a/wrapper.c b/wrapper.c
index c9be140..dd7b6ee 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -34,6 +34,16 @@ void *xmalloc(size_t size)
 	return ret;
 }
 
+void *xmallocz(size_t size)
+{
+	void *ret;
+	if (size + 1 < size)
+		die("Data too large to fit into virtual memory space.");
+	ret = xmalloc(size + 1);
+	((char*)ret)[size] = 0;
+	return ret;
+}
+
 /*
  * xmemdupz() allocates (len + 1) bytes of memory, duplicates "len" bytes of
  * "data" to the allocated memory, zero terminates the allocated memory,
@@ -42,7 +52,7 @@ void *xmalloc(size_t size)
  */
 void *xmemdupz(const void *data, size_t len)
 {
-	char *p = xmalloc(len + 1);
+	char *p = xmallocz(len);
 	memcpy(p, data, len);
 	p[len] = '\0';
 	return p;
-- 
1.6.6.1.439.gf06b6

Re: [PATCH 1/4] Add xmallocz()

[Bill Lear]

On Tuesday, January 26, 2010 at 20:24:12 (+0200) Ilari Liusvaara writes:
>Add routine for allocating NUL-terminated memory block without risking
>integer overflow in addition of +1 for NUL byte.
>...
> void *xmemdupz(const void *data, size_t len)
> {
>-	char *p = xmalloc(len + 1);
>+	char *p = xmallocz(len);
> 	memcpy(p, data, len);
> 	p[len] = '\0';
> 	return p;

Do you need the statement

 	p[len] = '\0';

any longer in the above?  If not, could you just do this:

void *xmemdupz(const void *data, size_t len)
{
	return memcpy(xmallocz(len), data, len);
}


??


Bill

Re: [PATCH 1/4] Add xmallocz()

[Junio C Hamano]

Bill Lear <rael@zopyra.com> writes:

> On Tuesday, January 26, 2010 at 20:24:12 (+0200) Ilari Liusvaara writes:
>>Add routine for allocating NUL-terminated memory block without risking
>>integer overflow in addition of +1 for NUL byte.
>>...
>> void *xmemdupz(const void *data, size_t len)
>> {
>>-	char *p = xmalloc(len + 1);
>>+	char *p = xmallocz(len);
>> 	memcpy(p, data, len);
>> 	p[len] = '\0';
>> 	return p;
>
> Do you need the statement
>
>  	p[len] = '\0';
>
> any longer in the above?  If not, could you just do this:
>
> void *xmemdupz(const void *data, size_t len)
> {
> 	return memcpy(xmallocz(len), data, len);
> }

I think the intention to name it xmallocz() was "This is to allocate
buffer to hold 'len' bytes worth of stuff, and between the caller and this
function the buffer is arranged to be NUL terminated".  Even though none
of the existing callers of xmalloc() expected the function to do the NUL
termination (hence they do NUL termination themselves), I _think_ Ilari
made the function to do this because its name now ends with "z" that hints
the callers such a NUL-termination might happen inside the function.

I'd rather see the function lose the NUL termination; if that makes the
behaviour inconsistent with its name, perhaps it is better to rename the
function; perhaps xmalloc1() to denote that it overallocates by one?

Re: [PATCH 1/4] Add xmallocz()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I'd rather see the function lose the NUL termination; if that makes the
> behaviour inconsistent with its name, perhaps it is better to rename the
> function; perhaps xmalloc1() to denote that it overallocates by one?

Actually I take that back---all the callers do benefit from the allocator
giving a buffer that is pre-terminated with NUL.

We can also lose "buf[size] = 0" from unpack_sha1_rest() patch.

Re: [PATCH 1/4] Add xmallocz()

[Ilari Liusvaara]

On Tue, Jan 26, 2010 at 12:52:00PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I'd rather see the function lose the NUL termination; if that makes the
> > behaviour inconsistent with its name, perhaps it is better to rename the
> > function; perhaps xmalloc1() to denote that it overallocates by one?
> 
> Actually I take that back---all the callers do benefit from the allocator
> giving a buffer that is pre-terminated with NUL.
> 
> We can also lose "buf[size] = 0" from unpack_sha1_rest() patch.

If that line would be next to xmalloc() line, I would have removed it, but
because it was beyound loop, I was worried that something might be done
to it (and was not in right mood to analyze the logic properly).

And yeah, that nul-termination in xmemdupz is not needed. Oops, missed
that.

-Ilari

Re: [PATCH 1/4] Add xmallocz()

[Bill Lear]

On Tuesday, January 26, 2010 at 12:47:02 (-0800) Junio C Hamano writes:
>Bill Lear <rael@zopyra.com> writes:
>
>> On Tuesday, January 26, 2010 at 20:24:12 (+0200) Ilari Liusvaara writes:
>>>Add routine for allocating NUL-terminated memory block without risking
>>>integer overflow in addition of +1 for NUL byte.
>>>...
>>> void *xmemdupz(const void *data, size_t len)
>>> {
>>>-	char *p = xmalloc(len + 1);
>>>+	char *p = xmallocz(len);
>>> 	memcpy(p, data, len);
>>> 	p[len] = '\0';
>>> 	return p;
>>
>> Do you need the statement
>>
>>  	p[len] = '\0';
>>
>> any longer in the above?  If not, could you just do this:
>>
>> void *xmemdupz(const void *data, size_t len)
>> {
>> 	return memcpy(xmallocz(len), data, len);
>> }
>
>I think the intention to name it xmallocz() was "This is to allocate
>buffer to hold 'len' bytes worth of stuff, and between the caller and this
>function the buffer is arranged to be NUL terminated".  Even though none
>of the existing callers of xmalloc() expected the function to do the NUL
>termination (hence they do NUL termination themselves), I _think_ Ilari
>made the function to do this because its name now ends with "z" that hints
>the callers such a NUL-termination might happen inside the function.
>
>I'd rather see the function lose the NUL termination; if that makes the
>behaviour inconsistent with its name, perhaps it is better to rename the
>function; perhaps xmalloc1() to denote that it overallocates by one?

Why have xmallocz/xmalloc1 lose the NUL termination?  Is it because some
call sites don't need the NUL termination?  [I spotted one, I think, in
the patch series...].


Bill

[PATCH 2/4] Fix integer overflow in patch_delta()

[Ilari Liusvaara]

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 patch-delta.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/patch-delta.c b/patch-delta.c
index e02e13b..d218faa 100644
--- a/patch-delta.c
+++ b/patch-delta.c
@@ -33,8 +33,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 
 	/* now the result size */
 	size = get_delta_hdr_size(&data, top);
-	dst_buf = xmalloc(size + 1);
-	dst_buf[size] = 0;
+	dst_buf = xmallocz(size);
 
 	out = dst_buf;
 	while (data < top) {
-- 
1.6.6.1.439.gf06b6

[PATCH 3/4] Fix integer overflow in unpack_sha1_rest()

[Ilari Liusvaara]

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 sha1_file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 12478a3..39f0844 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1166,7 +1166,7 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon
 static void *unpack_sha1_rest(z_stream *stream, void *buffer, unsigned long size, const unsigned char *sha1)
 {
 	int bytes = strlen(buffer) + 1;
-	unsigned char *buf = xmalloc(1+size);
+	unsigned char *buf = xmallocz(size);
 	unsigned long n;
 	int status = Z_OK;
 
-- 
1.6.6.1.439.gf06b6

[PATCH 4/4] Fix integer overflow in unpack_compressed_entry()

[Ilari Liusvaara]

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 sha1_file.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 39f0844..ea2ea75 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1517,8 +1517,7 @@ static void *unpack_compressed_entry(struct packed_git *p,
 	z_stream stream;
 	unsigned char *buffer, *in;
 
-	buffer = xmalloc(size + 1);
-	buffer[size] = 0;
+	buffer = xmallocz(size);
 	memset(&stream, 0, sizeof(stream));
 	stream.next_out = buffer;
 	stream.avail_out = size + 1;
-- 
1.6.6.1.439.gf06b6

Re: [PATCH 0/4] Fix various integer overflows

[Junio C Hamano]

Looks trivially correct; thanks.

Re: [PATCH 0/4] Fix various integer overflows

[Stephen R. van den Berg]

Junio C Hamano wrote:
>Looks trivially correct; thanks.

I'm just curious, but is this based on an actual bug which someone
experienced, or is this just based on mere theoretical code analysis?
-- 
Sincerely,
           Stephen R. van den Berg.

"Am I paying for this abuse or is it extra?"

Re: [PATCH 0/4] Fix various integer overflows

[Ilari Liusvaara]

On Wed, Jan 27, 2010 at 09:59:52AM +0100, Stephen R. van den Berg wrote:
> Junio C Hamano wrote:
> >Looks trivially correct; thanks.
> 
> I'm just curious, but is this based on an actual bug which someone
> experienced, or is this just based on mere theoretical code analysis?

Theoretical at first, but I did construct packfile that hits one of
those overflows (the one in patch_delta(), 32 bits only).

In real world, hitting this bug would require hitting exactly 2^32-1
byte file, and that is quite rare size for file.

And what can happen with them in real world git usage is different
than what can happen with them if packs are suitably manipulated
("transport streams" and bundles both contain packs in them).

-Ilari