threads / patch / 22397

patch, 4 partsFix various integer overflows

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

## tl;dr

13 messages between Jan 26, 2010 and Jan 27, 2010. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Ilari Liusvaara· Jan 26, 2010, 18:24 UTC · lore

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(-)
Ilari Liusvaara· Jan 26, 2010, 18:24 UTC · re: Ilari Liusvaara · lore

[PATCH 1/4] Add xmallocz()

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(-)
Show changes to 2 files +12 −1

git-compat-util.h, wrapper.c

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
Bill Lear· Jan 26, 2010, 20:37 UTC · re: Ilari Liusvaara · lore

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

On Tuesday, January 26, 2010 at 20:24:12 (+0200) Ilari Liusvaara writes:
Show 10 quoted lines
>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
Junio C Hamano· Jan 26, 2010, 20:47 UTC · re: Bill Lear · lore

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

Bill Lear <rael@zopyra.com> writes:
Show 22 quoted lines
> 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?

Junio C Hamano· Jan 26, 2010, 20:52 UTC · re: Junio C Hamano · lore

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

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.
Ilari Liusvaara· Jan 26, 2010, 21:13 UTC · re: Junio C Hamano · lore

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

On Tue, Jan 26, 2010 at 12:52:00PM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> 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
Bill Lear· Jan 26, 2010, 20:56 UTC · re: Junio C Hamano · lore

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

On Tuesday, January 26, 2010 at 12:47:02 (-0800) Junio C Hamano writes:
Show 36 quoted lines
>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
Ilari Liusvaara· Jan 26, 2010, 18:24 UTC · re: Ilari Liusvaara · lore

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

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 patch-delta.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
Show changes to patch-delta.c +1 −2
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
Ilari Liusvaara· Jan 26, 2010, 18:24 UTC · re: Ilari Liusvaara · lore

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

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 sha1_file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to sha1_file.c +1 −1
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
Ilari Liusvaara· Jan 26, 2010, 18:24 UTC · re: Ilari Liusvaara · lore

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

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 sha1_file.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
Show changes to sha1_file.c +1 −2
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
Stephen R. van den Berg· Jan 27, 2010, 08:59 UTC · re: Junio C Hamano · lore

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

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?"
Ilari Liusvaara· Jan 27, 2010, 09:57 UTC · re: Stephen R. van den Berg · lore

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

On Wed, Jan 27, 2010 at 09:59:52AM +0100, Stephen R. van den Berg wrote:
Show 5 quoted lines
> 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

← back to recent threads