{"thread":{"id":"22397","subject":"[PATCH 0/4] Fix various integer overflows","startedAt":"2010-01-26T18:24:11Z","lastAt":"2010-01-27T09:57:48Z","messageCount":13,"participants":["Ilari Liusvaara","Junio C Hamano","Bill Lear","Stephen R. van den Berg"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"132700","messageId":"1264530255-4682-1-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22397","inReplyTo":null,"subject":"[PATCH 0/4] Fix various integer overflows","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-26T18:24:11Z","receivedAt":"2010-01-26T18:24:11Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Fix integer overflows in patch_delta(), unpack_sha1_rest() and\nunpack_compressed_entry().\n\nThese at least can cause git to segfault, possibly worse. Operations\nthat cause integer overflow are not possible to do (even whole virtual\nmemory space would not be sufficient), so die() instead.\n\nIlari Liusvaara (4):\n Add xmallocz()\n Fix integer overflow in patch_delta()\n Fix integer overflow in unpack_sha1_rest()\n Fix integer overflow in unpack_compressed_entry()\n\n git-compat-util.h | 1 +\n patch-delta.c | 3 +--\n sha1_file.c | 5 ++---\n wrapper.c | 12 +++++++++++-\n 4 files changed, 15 insertions(+), 6 deletions(-)\n"},{"id":"132701","messageId":"1264530255-4682-2-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22397","inReplyTo":"1264530255-4682-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[PATCH 1/4] Add xmallocz()","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-26T18:24:12Z","receivedAt":"2010-01-26T18:24:12Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Add routine for allocating NUL-terminated memory block without risking\ninteger overflow in addition of +1 for NUL byte.\n\nSigned-off-by: Ilari Liusvaara \n---\n git-compat-util.h | 1 +\n wrapper.c | 12 +++++++++++-\n 2 files changed, 12 insertions(+), 1 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 620a7c6..a3c4537 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -348,6 +348,7 @@ extern void release_pack_memory(size_t, int);\n \n extern char *xstrdup(const char *str);\n extern void *xmalloc(size_t size);\n+extern void *xmallocz(size_t size);\n extern void *xmemdupz(const void *data, size_t len);\n extern char *xstrndup(const char *str, size_t len);\n extern void *xrealloc(void *ptr, size_t size);\ndiff --git a/wrapper.c b/wrapper.c\nindex c9be140..dd7b6ee 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -34,6 +34,16 @@ void *xmalloc(size_t size)\n \treturn ret;\n }\n \n+void *xmallocz(size_t size)\n+{\n+\tvoid *ret;\n+\tif (size + 1 < size)\n+\t\tdie(\"Data too large to fit into virtual memory space.\");\n+\tret = xmalloc(size + 1);\n+\t((char*)ret)[size] = 0;\n+\treturn ret;\n+}\n+\n /*\n * xmemdupz() allocates (len + 1) bytes of memory, duplicates \"len\" bytes of\n * \"data\" to the allocated memory, zero terminates the allocated memory,\n@@ -42,7 +52,7 @@ void *xmalloc(size_t size)\n */\n void *xmemdupz(const void *data, size_t len)\n {\n-\tchar *p = xmalloc(len + 1);\n+\tchar *p = xmallocz(len);\n \tmemcpy(p, data, len);\n \tp[len] = '\\0';\n \treturn p;\n-- \n1.6.6.1.439.gf06b6\n"},{"id":"132703","messageId":"1264530255-4682-3-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22397","inReplyTo":"1264530255-4682-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[PATCH 2/4] Fix integer overflow in patch_delta()","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-26T18:24:13Z","receivedAt":"2010-01-26T18:24:13Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"\nSigned-off-by: Ilari Liusvaara \n---\n patch-delta.c | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/patch-delta.c b/patch-delta.c\nindex e02e13b..d218faa 100644\n--- a/patch-delta.c\n+++ b/patch-delta.c\n@@ -33,8 +33,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,\n \n \t/* now the result size */\n \tsize = get_delta_hdr_size(&data, top);\n-\tdst_buf = xmalloc(size + 1);\n-\tdst_buf[size] = 0;\n+\tdst_buf = xmallocz(size);\n \n \tout = dst_buf;\n \twhile (data < top) {\n-- \n1.6.6.1.439.gf06b6\n"},{"id":"132702","messageId":"1264530255-4682-4-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22397","inReplyTo":"1264530255-4682-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[PATCH 3/4] Fix integer overflow in unpack_sha1_rest()","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-26T18:24:14Z","receivedAt":"2010-01-26T18:24:14Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"\nSigned-off-by: Ilari Liusvaara \n---\n sha1_file.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 12478a3..39f0844 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1166,7 +1166,7 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon\n static void *unpack_sha1_rest(z_stream *stream, void *buffer, unsigned long size, const unsigned char *sha1)\n {\n \tint bytes = strlen(buffer) + 1;\n-\tunsigned char *buf = xmalloc(1+size);\n+\tunsigned char *buf = xmallocz(size);\n \tunsigned long n;\n \tint status = Z_OK;\n \n-- \n1.6.6.1.439.gf06b6\n"},{"id":"132706","messageId":"1264530255-4682-5-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22397","inReplyTo":"1264530255-4682-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[PATCH 4/4] Fix integer overflow in unpack_compressed_entry()","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-26T18:24:15Z","receivedAt":"2010-01-26T18:24:15Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"\nSigned-off-by: Ilari Liusvaara \n---\n sha1_file.c | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 39f0844..ea2ea75 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1517,8 +1517,7 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tz_stream stream;\n \tunsigned char *buffer, *in;\n \n-\tbuffer = xmalloc(size + 1);\n-\tbuffer[size] = 0;\n+\tbuffer = xmallocz(size);\n \tmemset(&stream, 0, sizeof(stream));\n \tstream.next_out = buffer;\n \tstream.avail_out = size + 1;\n-- \n1.6.6.1.439.gf06b6\n"},{"id":"132709","messageId":"7vk4v4zlhg.fsf@alter.siamese.dyndns.org","threadId":"22397","inReplyTo":"1264530255-4682-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [PATCH 0/4] Fix various integer overflows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-26T19:58:51Z","receivedAt":"2010-01-26T19:58:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Looks trivially correct; thanks.\n"},{"id":"132712","messageId":"19295.21148.182245.516321@blake.zopyra.com","threadId":"22397","inReplyTo":"1264530255-4682-2-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [PATCH 1/4] Add xmallocz()","fromName":"Bill Lear","fromEmail":"rael@zopyra.com","sentAt":"2010-01-26T20:37:48Z","receivedAt":"2010-01-26T20:37:48Z","isPatch":true,"sender":{"key":"rael@zopyra.com","avatar":"https://gravatar.com/avatar/c4f2d2790ca3828d3b4e7dfebabf61d2fe94fd82fa49cdac2a5295dd2d46a874?d=mp&s=160"},"body":"On Tuesday, January 26, 2010 at 20:24:12 (+0200) Ilari Liusvaara writes:\n>Add routine for allocating NUL-terminated memory block without risking\n>integer overflow in addition of +1 for NUL byte.\n>...\n> void *xmemdupz(const void *data, size_t len)\n> {\n>-\tchar *p = xmalloc(len + 1);\n>+\tchar *p = xmallocz(len);\n> \tmemcpy(p, data, len);\n> \tp[len] = '\\0';\n> \treturn p;\n\nDo you need the statement\n\n \tp[len] = '\\0';\n\nany longer in the above? If not, could you just do this:\n\nvoid *xmemdupz(const void *data, size_t len)\n{\n\treturn memcpy(xmallocz(len), data, len);\n}\n\n\n??\n\n\nBill\n"},{"id":"132713","messageId":"7v1vhczj95.fsf@alter.siamese.dyndns.org","threadId":"22397","inReplyTo":"19295.21148.182245.516321@blake.zopyra.com","subject":"Re: [PATCH 1/4] Add xmallocz()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-26T20:47:02Z","receivedAt":"2010-01-26T20:47:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bill Lear writes:\n\n> On Tuesday, January 26, 2010 at 20:24:12 (+0200) Ilari Liusvaara writes:\n>>Add routine for allocating NUL-terminated memory block without risking\n>>integer overflow in addition of +1 for NUL byte.\n>>...\n>> void *xmemdupz(const void *data, size_t len)\n>> {\n>>-\tchar *p = xmalloc(len + 1);\n>>+\tchar *p = xmallocz(len);\n>> \tmemcpy(p, data, len);\n>> \tp[len] = '\\0';\n>> \treturn p;\n>\n> Do you need the statement\n>\n> \tp[len] = '\\0';\n>\n> any longer in the above? If not, could you just do this:\n>\n> void *xmemdupz(const void *data, size_t len)\n> {\n> \treturn memcpy(xmallocz(len), data, len);\n> }\n\nI think the intention to name it xmallocz() was \"This is to allocate\nbuffer to hold 'len' bytes worth of stuff, and between the caller and this\nfunction the buffer is arranged to be NUL terminated\". Even though none\nof the existing callers of xmalloc() expected the function to do the NUL\ntermination (hence they do NUL termination themselves), I _think_ Ilari\nmade the function to do this because its name now ends with \"z\" that hints\nthe callers such a NUL-termination might happen inside the function.\n\nI'd rather see the function lose the NUL termination; if that makes the\nbehaviour inconsistent with its name, perhaps it is better to rename the\nfunction; perhaps xmalloc1() to denote that it overallocates by one?\n"},{"id":"132714","messageId":"7vwrz4y4gf.fsf@alter.siamese.dyndns.org","threadId":"22397","inReplyTo":"7v1vhczj95.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] Add xmallocz()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-26T20:52:00Z","receivedAt":"2010-01-26T20:52:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I'd rather see the function lose the NUL termination; if that makes the\n> behaviour inconsistent with its name, perhaps it is better to rename the\n> function; perhaps xmalloc1() to denote that it overallocates by one?\n\nActually I take that back---all the callers do benefit from the allocator\ngiving a buffer that is pre-terminated with NUL.\n\nWe can also lose \"buf[size] = 0\" from unpack_sha1_rest() patch.\n"},{"id":"132715","messageId":"19295.22246.788034.159299@blake.zopyra.com","threadId":"22397","inReplyTo":"7v1vhczj95.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] Add xmallocz()","fromName":"Bill Lear","fromEmail":"rael@zopyra.com","sentAt":"2010-01-26T20:56:06Z","receivedAt":"2010-01-26T20:56:06Z","isPatch":true,"sender":{"key":"rael@zopyra.com","avatar":"https://gravatar.com/avatar/c4f2d2790ca3828d3b4e7dfebabf61d2fe94fd82fa49cdac2a5295dd2d46a874?d=mp&s=160"},"body":"On Tuesday, January 26, 2010 at 12:47:02 (-0800) Junio C Hamano writes:\n>Bill Lear writes:\n>\n>> On Tuesday, January 26, 2010 at 20:24:12 (+0200) Ilari Liusvaara writes:\n>>>Add routine for allocating NUL-terminated memory block without risking\n>>>integer overflow in addition of +1 for NUL byte.\n>>>...\n>>> void *xmemdupz(const void *data, size_t len)\n>>> {\n>>>-\tchar *p = xmalloc(len + 1);\n>>>+\tchar *p = xmallocz(len);\n>>> \tmemcpy(p, data, len);\n>>> \tp[len] = '\\0';\n>>> \treturn p;\n>>\n>> Do you need the statement\n>>\n>> \tp[len] = '\\0';\n>>\n>> any longer in the above? If not, could you just do this:\n>>\n>> void *xmemdupz(const void *data, size_t len)\n>> {\n>> \treturn memcpy(xmallocz(len), data, len);\n>> }\n>\n>I think the intention to name it xmallocz() was \"This is to allocate\n>buffer to hold 'len' bytes worth of stuff, and between the caller and this\n>function the buffer is arranged to be NUL terminated\". Even though none\n>of the existing callers of xmalloc() expected the function to do the NUL\n>termination (hence they do NUL termination themselves), I _think_ Ilari\n>made the function to do this because its name now ends with \"z\" that hints\n>the callers such a NUL-termination might happen inside the function.\n>\n>I'd rather see the function lose the NUL termination; if that makes the\n>behaviour inconsistent with its name, perhaps it is better to rename the\n>function; perhaps xmalloc1() to denote that it overallocates by one?\n\nWhy have xmallocz/xmalloc1 lose the NUL termination? Is it because some\ncall sites don't need the NUL termination? [I spotted one, I think, in\nthe patch series...].\n\n\nBill\n"},{"id":"132716","messageId":"20100126211347.GA9194@Knoppix","threadId":"22397","inReplyTo":"7vwrz4y4gf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] Add xmallocz()","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-26T21:13:48Z","receivedAt":"2010-01-26T21:13:48Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Tue, Jan 26, 2010 at 12:52:00PM -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > I'd rather see the function lose the NUL termination; if that makes the\n> > behaviour inconsistent with its name, perhaps it is better to rename the\n> > function; perhaps xmalloc1() to denote that it overallocates by one?\n> \n> Actually I take that back---all the callers do benefit from the allocator\n> giving a buffer that is pre-terminated with NUL.\n> \n> We can also lose \"buf[size] = 0\" from unpack_sha1_rest() patch.\n\nIf that line would be next to xmalloc() line, I would have removed it, but\nbecause it was beyound loop, I was worried that something might be done\nto it (and was not in right mood to analyze the logic properly).\n\nAnd yeah, that nul-termination in xmemdupz is not needed. Oops, missed\nthat.\n\n-Ilari\n"},{"id":"132759","messageId":"20100127085952.GA21535@cuci.nl","threadId":"22397","inReplyTo":"7vk4v4zlhg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/4] Fix various integer overflows","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2010-01-27T08:59:52Z","receivedAt":"2010-01-27T08:59:52Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n>Looks trivially correct; thanks.\n\nI'm just curious, but is this based on an actual bug which someone\nexperienced, or is this just based on mere theoretical code analysis?\n-- \nSincerely,\n Stephen R. van den Berg.\n\n\"Am I paying for this abuse or is it extra?\"\n"},{"id":"132762","messageId":"20100127095748.GA9992@Knoppix","threadId":"22397","inReplyTo":"20100127085952.GA21535@cuci.nl","subject":"Re: [PATCH 0/4] Fix various integer overflows","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-27T09:57:48Z","receivedAt":"2010-01-27T09:57:48Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Wed, Jan 27, 2010 at 09:59:52AM +0100, Stephen R. van den Berg wrote:\n> Junio C Hamano wrote:\n> >Looks trivially correct; thanks.\n> \n> I'm just curious, but is this based on an actual bug which someone\n> experienced, or is this just based on mere theoretical code analysis?\n\nTheoretical at first, but I did construct packfile that hits one of\nthose overflows (the one in patch_delta(), 32 bits only).\n\nIn real world, hitting this bug would require hitting exactly 2^32-1\nbyte file, and that is quite rare size for file.\n\nAnd what can happen with them in real world git usage is different\nthan what can happen with them if packs are suitably manipulated\n(\"transport streams\" and bundles both contain packs in them).\n\n-Ilari\n"}]}