# [PATCH] git name-rev writes beyond the end of malloc() with large generations 2 messages from 2007-05-15 to 2007-05-15. Participants: Andy Whitcroft, Junio C Hamano. Thread: https://gitlist.dev/t/8168 ## Andy Whitcroft, 2007-05-15 16:33 Subject: [PATCH] git name-rev writes beyond the end of malloc() with large generations Message-ID: <2be2ad34be511217dc735a15490f4536@pinky> URL: https://gitlist.dev/e/2be2ad34be511217dc735a15490f4536%40pinky ``` When using git name-rev on my kernel tree I triggered a malloc() corruption warning from glibc. apw@pinky$ git log --pretty=one $N/base.. | git name-rev --stdin *** glibc detected *** malloc(): memory corruption: 0x0bff8950 *** Aborted This comes from name_rev() which is building the name of the revision in a malloc'd string, which it sprintf's into: char *new_name = xmalloc(len + 8); [...] sprintf(new_name, "%.*s~%d^%d", len, tip_name, generation, parent_number); This allocation is only sufficient if the generation number is less than 5 digits, in my case generation was 13432. In reality parent_number can be up to 16 so that also can require two digits, reducing us to 3 digits before we are at risk of blowing this allocation. This patch introduces a decimal_length() which approximates the number of digits a type may hold, it produces the following: Type Longest Value Len Est ---- ------------- --- --- unsigned char 256 3 4 unsigned short 65536 5 6 unsigned long 4294967296 10 11 unsigned long long 18446744073709551616 20 21 char -128 4 4 short -32768 6 6 long -2147483648 11 11 long long -9223372036854775808 20 21 This is then used to size the new_name. Signed-off-by: Andy Whitcroft --- This patch is against current next. I have confirmed that at least GCC can optimise this away to a constant. --- diff --git a/builtin-name-rev.c b/builtin-name-rev.c index c022224..ef16385 100644 --- a/builtin-name-rev.c +++ b/builtin-name-rev.c @@ -58,7 +58,10 @@ copy_data: parents = parents->next, parent_number++) { if (parent_number > 1) { int len = strlen(tip_name); - char *new_name = xmalloc(len + 8); + char *new_name = xmalloc(len + + 1 + decimal_length(generation) + /* ~ */ + 1 + 2 + /* ^NN */ + 1); if (len > 2 && !strcmp(tip_name + len - 2, "^0")) len -= 2; diff --git a/git-compat-util.h b/git-compat-util.h index c08688c..25b8274 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -19,6 +19,9 @@ #define TYPEOF(x) #endif +/* Approximation of the length of the decimal representation of this type. */ +#define decimal_length(x) ((int)(sizeof(x) * 2.56 + 0.5) + 1) + #define MSB(x, bits) ((x) & TYPEOF(x)(~0ULL << (sizeof(x) * 8 - (bits)))) #if !defined(__APPLE__) && !defined(__FreeBSD__) ``` ## Junio C Hamano, 2007-05-15 19:09 Subject: Re: [PATCH] git name-rev writes beyond the end of malloc() with large generations Message-ID: <7vps51j44r.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vps51j44r.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <2be2ad34be511217dc735a15490f4536@pinky> ``` Andy Whitcroft writes: > When using git name-rev on my kernel tree I triggered a malloc() > corruption warning from glibc. > > apw@pinky$ git log --pretty=one $N/base.. | git name-rev --stdin > *** glibc detected *** malloc(): memory corruption: 0x0bff8950 *** > Aborted > > This comes from name_rev() which is building the name of the revision > in a malloc'd string, which it sprintf's into: > > char *new_name = xmalloc(len + 8); > [...] > sprintf(new_name, "%.*s~%d^%d", len, tip_name, > generation, parent_number); > > This allocation is only sufficient if the generation number is > less than 5 digits, in my case generation was 13432. In reality > parent_number can be up to 16 so that also can require two digits, > reducing us to 3 digits before we are at risk of blowing this > allocation. > > This patch introduces a decimal_length() which approximates the > number of digits a type may hold, it produces the following: > ... Does this attempt to cramp down to what's only necessary really matter in practice in the light of malloc overhead? It does futureproof against an insanely long "long long" on future architectures, but I am not sure if we care either. Why not just raise 8 to 25 or something and be done with it? > diff --git a/git-compat-util.h b/git-compat-util.h > index c08688c..25b8274 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -19,6 +19,9 @@ > #define TYPEOF(x) > #endif > > +/* Approximation of the length of the decimal representation of this type. */ > +#define decimal_length(x) ((int)(sizeof(x) * 2.56 + 0.5) + 1) > + > #define MSB(x, bits) ((x) & TYPEOF(x)(~0ULL << (sizeof(x) * 8 - (bits)))) > > #if !defined(__APPLE__) && !defined(__FreeBSD__) Having said that, clever and clean math and use of compiler's ability always attracts me, so maybe I would end up applying this as is. ```