{"thread":{"id":"8168","subject":"[PATCH] git name-rev writes beyond the end of malloc() with large generations","startedAt":"2007-05-15T16:33:25Z","lastAt":"2007-05-15T19:09:08Z","messageCount":2,"participants":["Andy Whitcroft","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"42232","messageId":"2be2ad34be511217dc735a15490f4536@pinky","threadId":"8168","inReplyTo":null,"subject":"[PATCH] git name-rev writes beyond the end of malloc() with large generations","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2007-05-15T16:33:25Z","receivedAt":"2007-05-15T16:33:25Z","isPatch":true,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"\nWhen using git name-rev on my kernel tree I triggered a malloc()\ncorruption warning from glibc.\n\napw@pinky$ git log --pretty=one $N/base.. | git name-rev --stdin\n*** glibc detected *** malloc(): memory corruption: 0x0bff8950 ***\nAborted\n\nThis comes from name_rev() which is building the name of the revision\nin a malloc'd string, which it sprintf's into:\n\n\tchar *new_name = xmalloc(len + 8);\n\t[...]\n\t\tsprintf(new_name, \"%.*s~%d^%d\", len, tip_name,\n\t\t\t\tgeneration, parent_number);\n\nThis allocation is only sufficient if the generation number is\nless than 5 digits, in my case generation was 13432.  In reality\nparent_number can be up to 16 so that also can require two digits,\nreducing us to 3 digits before we are at risk of blowing this\nallocation.\n\nThis patch introduces a decimal_length() which approximates the\nnumber of digits a type may hold, it produces the following:\n\nType                 Longest Value          Len  Est\n----                 -------------          ---  ---\nunsigned char        256                      3    4\nunsigned short       65536                    5    6\nunsigned long        4294967296              10   11\nunsigned long long   18446744073709551616    20   21\nchar                 -128                     4    4\nshort                -32768                   6    6\nlong                 -2147483648             11   11\nlong long            -9223372036854775808    20   21\n\nThis is then used to size the new_name.\n\nSigned-off-by: Andy Whitcroft <apw@shadowen.org>\n---\n\n\tThis patch is against current next.  I have confirmed that\n\tat least GCC can optimise this away to a constant.\n---\ndiff --git a/builtin-name-rev.c b/builtin-name-rev.c\nindex c022224..ef16385 100644\n--- a/builtin-name-rev.c\n+++ b/builtin-name-rev.c\n@@ -58,7 +58,10 @@ copy_data:\n \t\t\tparents = parents->next, parent_number++) {\n \t\tif (parent_number > 1) {\n \t\t\tint len = strlen(tip_name);\n-\t\t\tchar *new_name = xmalloc(len + 8);\n+\t\t\tchar *new_name = xmalloc(len +\n+\t\t\t\t1 + decimal_length(generation) +  /* ~<n> */\n+\t\t\t\t1 + 2 +\t\t\t\t  /* ^NN */\n+\t\t\t\t1);\n \n \t\t\tif (len > 2 && !strcmp(tip_name + len - 2, \"^0\"))\n \t\t\t\tlen -= 2;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c08688c..25b8274 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -19,6 +19,9 @@\n #define TYPEOF(x)\n #endif\n \n+/* Approximation of the length of the decimal representation of this type. */\n+#define decimal_length(x)\t((int)(sizeof(x) * 2.56 + 0.5) + 1)\n+\n #define MSB(x, bits) ((x) & TYPEOF(x)(~0ULL << (sizeof(x) * 8 - (bits))))\n \n #if !defined(__APPLE__) && !defined(__FreeBSD__)\n"},{"id":"42243","messageId":"7vps51j44r.fsf@assigned-by-dhcp.cox.net","threadId":"8168","inReplyTo":"2be2ad34be511217dc735a15490f4536@pinky","subject":"Re: [PATCH] git name-rev writes beyond the end of malloc() with large generations","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-15T19:09:08Z","receivedAt":"2007-05-15T19:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Whitcroft <apw@shadowen.org> writes:\n\n> When using git name-rev on my kernel tree I triggered a malloc()\n> corruption warning from glibc.\n>\n> apw@pinky$ git log --pretty=one $N/base.. | git name-rev --stdin\n> *** glibc detected *** malloc(): memory corruption: 0x0bff8950 ***\n> Aborted\n>\n> This comes from name_rev() which is building the name of the revision\n> in a malloc'd string, which it sprintf's into:\n>\n> \tchar *new_name = xmalloc(len + 8);\n> \t[...]\n> \t\tsprintf(new_name, \"%.*s~%d^%d\", len, tip_name,\n> \t\t\t\tgeneration, parent_number);\n>\n> This allocation is only sufficient if the generation number is\n> less than 5 digits, in my case generation was 13432.  In reality\n> parent_number can be up to 16 so that also can require two digits,\n> reducing us to 3 digits before we are at risk of blowing this\n> allocation.\n>\n> This patch introduces a decimal_length() which approximates the\n> number of digits a type may hold, it produces the following:\n> ...\n\nDoes this attempt to cramp down to what's only necessary really\nmatter in practice in the light of malloc overhead?\n\nIt does futureproof against an insanely long \"long long\" on\nfuture architectures, but I am not sure if we care either.  Why\nnot just raise 8 to 25 or something and be done with it?\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index c08688c..25b8274 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -19,6 +19,9 @@\n>  #define TYPEOF(x)\n>  #endif\n>  \n> +/* Approximation of the length of the decimal representation of this type. */\n> +#define decimal_length(x)\t((int)(sizeof(x) * 2.56 + 0.5) + 1)\n> +\n>  #define MSB(x, bits) ((x) & TYPEOF(x)(~0ULL << (sizeof(x) * 8 - (bits))))\n>  \n>  #if !defined(__APPLE__) && !defined(__FreeBSD__)\n\nHaving said that, clever and clean math and use of compiler's\nability always attracts me, so maybe I would end up applying\nthis as is.\n"}]}