{"thread":{"id":"15","subject":"[patch] git: fix memory leak in checkout-cache.c","startedAt":"2005-04-14T11:26:38Z","lastAt":"2005-04-18T08:28:27Z","messageCount":22,"participants":["Ingo Molnar","Martin Schlemmer","Linus Torvalds","Petr Baudis"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"67","messageId":"20050414112638.GA12593@elte.hu","threadId":"15","inReplyTo":null,"subject":"[patch] git: fix memory leak in checkout-cache.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T11:26:38Z","receivedAt":"2005-04-14T11:26:38Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch fixes a memory leak in checkout-cache.\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- checkout-cache.c.orig\n+++ checkout-cache.c\n@@ -48,6 +48,7 @@ static void create_directories(const cha\n \t\tbuf[len] = 0;\n \t\tmkdir(buf, 0755);\n \t}\n+\tfree(buf);\n }\n \n static int create_file(const char *path, unsigned int mode)\n"},{"id":"68","messageId":"20050414113527.GA13790@elte.hu","threadId":"15","inReplyTo":"20050414112638.GA12593@elte.hu","subject":"[patch] git: fix memory leak #2 in checkout-cache.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T11:35:27Z","receivedAt":"2005-04-14T11:35:27Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch fixes another (very rare) memory leak in checkout-cache.\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- checkout-cache.c.orig\n+++ checkout-cache.c\n@@ -74,6 +74,8 @@ static int write_entry(struct cache_entr\n \n \tnew = read_sha1_file(ce->sha1, type, &size);\n \tif (!new || strcmp(type, \"blob\")) {\n+\t\tif (new)\n+\t\t\tfree(new);\n \t\treturn error(\"checkout-cache: unable to read sha1 file of %s (%s)\",\n \t\t\tce->name, sha1_to_hex(ce->sha1));\n \t}\n"},{"id":"69","messageId":"20050414114344.GA13879@elte.hu","threadId":"15","inReplyTo":"20050414113527.GA13790@elte.hu","subject":"[patch] git: cleanup in ls-tree.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T11:43:44Z","receivedAt":"2005-04-14T11:43:44Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\ncleanup: this patch adds a free() to ls-tree.c.\n\n(Technically it's not a memory leak yet because the buffer is allocated \nonce by the function and then the utility exits - but it's a tad cleaner \nto not leave such assumptions in the code, so that if someone reuses the \nfunction (or extends the utility to include a loop) the uncleanliness \ndoesnt develop into a real memory leak.)\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- ls-tree.c.orig\n+++ ls-tree.c\n@@ -33,6 +33,8 @@ static int list(unsigned char *sha1)\n \t\ttype = S_ISDIR(mode) ? \"tree\" : \"blob\";\n \t\tprintf(\"%03o\\t%s\\t%s\\t%s\\n\", mode, type, sha1_to_hex(sha1), path);\n \t}\n+\tfree(buffer);\n+\n \treturn 0;\n }\n \n"},{"id":"70","messageId":"20050414115422.GA14065@elte.hu","threadId":"15","inReplyTo":"20050414113527.GA13790@elte.hu","subject":"[patch] git: fix memory leaks in read-tree.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T11:54:22Z","receivedAt":"2005-04-14T11:54:22Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch fixes one common, and 4 rare memory leaks in read-tree.c.\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- read-tree.c.orig\n+++ read-tree.c\n@@ -23,23 +23,27 @@ static int read_one_entry(unsigned char \n \n static int read_tree(unsigned char *sha1, const char *base, int baselen)\n {\n-\tvoid *buffer;\n+\tvoid *buffer0, *buffer;\n \tunsigned long size;\n \tchar type[20];\n \n-\tbuffer = read_sha1_file(sha1, type, &size);\n+\tbuffer0 = buffer = read_sha1_file(sha1, type, &size);\n \tif (!buffer)\n \t\treturn -1;\n-\tif (strcmp(type, \"tree\"))\n+\tif (strcmp(type, \"tree\")) {\n+\t\tfree(buffer);\n \t\treturn -1;\n+\t}\n \twhile (size) {\n \t\tint len = strlen(buffer)+1;\n \t\tunsigned char *sha1 = buffer + len;\n \t\tchar *path = strchr(buffer, ' ')+1;\n \t\tunsigned int mode;\n \n-\t\tif (size < len + 20 || sscanf(buffer, \"%o\", &mode) != 1)\n+\t\tif (size < len + 20 || sscanf(buffer, \"%o\", &mode) != 1) {\n+\t\t\tfree(buffer0);\n \t\t\treturn -1;\n+\t\t}\n \n \t\tbuffer = sha1 + 20;\n \t\tsize -= len + 20;\n@@ -53,13 +57,19 @@ static int read_tree(unsigned char *sha1\n \t\t\tnewbase[baselen + pathlen] = '/';\n \t\t\tretval = read_tree(sha1, newbase, baselen + pathlen + 1);\n \t\t\tfree(newbase);\n-\t\t\tif (retval)\n+\t\t\tif (retval) {\n+\t\t\t\tfree(buffer0);\n \t\t\t\treturn -1;\n+\t\t\t}\n \t\t\tcontinue;\n \t\t}\n-\t\tif (read_one_entry(sha1, base, baselen, path, mode) < 0)\n+\t\tif (read_one_entry(sha1, base, baselen, path, mode) < 0) {\n+\t\t\tfree(buffer0);\n \t\t\treturn -1;\n+\t\t}\n \t}\n+\tfree(buffer0);\n+\n \treturn 0;\n }\n \n"},{"id":"71","messageId":"20050414115800.GB14065@elte.hu","threadId":"15","inReplyTo":"20050414113527.GA13790@elte.hu","subject":"[patch] git: fix rare memory leak in rev-tree.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T11:58:00Z","receivedAt":"2005-04-14T11:58:00Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch fixes a rare memory leak in rev-tree.c.\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- rev-tree.c.orig\n+++ rev-tree.c\n@@ -73,8 +73,11 @@ static int parse_commit(unsigned char *s\n \n \t\trev->flags |= SEEN;\n \t\tbuffer = bufptr = read_sha1_file(sha1, type, &size);\n-\t\tif (!buffer || strcmp(type, \"commit\"))\n+\t\tif (!buffer || strcmp(type, \"commit\")) {\n+\t\t\tif (buffer)\n+\t\t\t\tfree(buffer);\n \t\t\treturn -1;\n+\t\t}\n \t\tbufptr += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n \t\twhile (!memcmp(bufptr, \"parent \", 7) && !get_sha1_hex(bufptr+7, parent)) {\n \t\t\tadd_relationship(rev, parent);\n"},{"id":"72","messageId":"20050414120516.GC14065@elte.hu","threadId":"15","inReplyTo":"20050414115800.GB14065@elte.hu","subject":"[patch] git: report parse_commit() errors in rev-tree.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T12:05:16Z","receivedAt":"2005-04-14T12:05:16Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nmake actual use of the parse_commit() return value and print a warning, \ninstead of silently ignoring it. Should never trigger on a valid DB.\n\n(alternatively we might use a die() in the sanity check place and could \nremove all the return code handling?)\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- rev-tree.c.orig\n+++ rev-tree.c\n@@ -64,6 +64,7 @@ static unsigned long parse_commit_date(c\n static int parse_commit(unsigned char *sha1)\n {\n \tstruct revision *rev = lookup_rev(sha1);\n+\tint ret = 0;\n \n \tif (!(rev->flags & SEEN)) {\n \t\tvoid *buffer, *bufptr;\n@@ -81,13 +82,13 @@ static int parse_commit(unsigned char *s\n \t\tbufptr += 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n \t\twhile (!memcmp(bufptr, \"parent \", 7) && !get_sha1_hex(bufptr+7, parent)) {\n \t\t\tadd_relationship(rev, parent);\n-\t\t\tparse_commit(parent);\n+\t\t\tret |= parse_commit(parent);\n \t\t\tbufptr += 48;\t/* \"parent \" + \"hex sha1\" + \"\\n\" */\n \t\t}\n \t\trev->date = parse_commit_date(bufptr);\n \t\tfree(buffer);\n \t}\n-\treturn 0;\t\n+\treturn ret;\n }\n \n static void read_cache_file(const char *path)\n@@ -208,7 +209,8 @@ int main(int argc, char **argv)\n \t\t}\n \t\tif (nr >= MAX_COMMITS || get_sha1_hex(arg, sha1[nr]))\n \t\t\tusage(\"rev-tree [--edges] [--cache <cache-file>] <commit-id> [<commit-id>]\");\n-\t\tparse_commit(sha1[nr]);\n+\t\tif (parse_commit(sha1[nr]))\n+\t\t\tfprintf(stderr, \"warning: rev-tree: bad commit!\\n\");\n \t\tnr++;\n \t}\n \n"},{"id":"73","messageId":"20050414120834.GA14290@elte.hu","threadId":"15","inReplyTo":"20050414112638.GA12593@elte.hu","subject":"[patch] git: fix memory leak in show-diff.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T12:08:34Z","receivedAt":"2005-04-14T12:08:34Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch fixes a memory leak in show-diff.c.\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- show-diff.c.orig\n+++ show-diff.c\n@@ -53,6 +53,7 @@ static void show_diff_empty(struct cache\n \t\t\tprintf(\"\\n\");\n \t\tfflush(stdout);\n \t}\n+\tfree(old);\n }\n \n int main(int argc, char **argv)\n"},{"id":"75","messageId":"20050414121819.GA14380@elte.hu","threadId":"15","inReplyTo":"20050414120834.GA14290@elte.hu","subject":"[patch] git: fix overflow in update-cache.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T12:18:19Z","receivedAt":"2005-04-14T12:18:19Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch fixes a 1-byte overflow in update-cache.c (probably not \nexploitable). A specially crafted db object might trigger this overflow.\n\nthe bug is that normally the 'type' field is parsed by read_sha1_file(), \nvia:\n\n        if (sscanf(buffer, \"%10s %lu\", type, size) != 2)\n\ni.e. 0-10 long strings, which take 1-11 bytes of space. Normally the \ntype strings are stored in char [20] arrays, but in update-cache.c that \nis char [10], so a 1 byte overflow might occur.\n\nThis should not happen with a 'friendly' DB, as the longest type string \n(\"commit\") is 7 bytes long. The fix is to use the customary char [20].\n\n(someone might want to clean those open-coded constants up with a \nTYPE_LEN define, they do tend to cause problems like this. I'm not \nagainst open-coded constants (they make code much more readable), but \nfor fields that get filled in from possibly hostile objects this is \nplaying with fire.)\n\nhey, this might be the first true security fix for GIT? ;-)\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- update-cache.c.orig\n+++ update-cache.c\n@@ -139,7 +139,7 @@ static int compare_data(struct cache_ent\n \tif (fd >= 0) {\n \t\tvoid *buffer;\n \t\tunsigned long size;\n-\t\tchar type[10];\n+\t\tchar type[20];\n \n \t\tbuffer = read_sha1_file(ce->sha1, type, &size);\n \t\tif (buffer) {\n"},{"id":"76","messageId":"20050414122352.GA15049@elte.hu","threadId":"15","inReplyTo":"20050414121819.GA14380@elte.hu","subject":"[patch] cleanup: read_sha1_file() -> malloc_read_sha1_file()","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T12:23:52Z","receivedAt":"2005-04-14T12:23:52Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch renames read_sha1_file() to malloc_read_sha1_file().\n\nThere were a handful of memory-leaks related to read_sha1_file(), and \nsome of those could possibly have been found sooner if the name \nindicated that an implicit malloc() is occurs within read_sha1_file().  \nThis patch is ontop of the previous patches i sent.\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- cat-file.c.orig\n+++ cat-file.c\n@@ -14,7 +14,7 @@ int main(int argc, char **argv)\n \n \tif (argc != 3 || get_sha1_hex(argv[2], sha1))\n \t\tusage(\"cat-file [-t | tagname] <sha1>\");\n-\tbuf = read_sha1_file(sha1, type, &size);\n+\tbuf = malloc_read_sha1_file(sha1, type, &size);\n \tif (!buf)\n \t\tdie(\"cat-file %s: bad file\", argv[2]);\n \tif (!strcmp(\"-t\", argv[1])) {\n--- merge-tree.c.orig\n+++ merge-tree.c\n@@ -11,7 +11,7 @@ static struct tree_entry *read_tree(unsi\n {\n \tchar type[20];\n \tunsigned long size;\n-\tvoid *buf = read_sha1_file(sha1, type, &size);\n+\tvoid *buf = malloc_read_sha1_file(sha1, type, &size);\n \tstruct tree_entry *ret = NULL, **tp = &ret;\n \n \tif (!buf || strcmp(type, \"tree\"))\n--- diff-tree.c.orig\n+++ diff-tree.c\n@@ -62,7 +62,7 @@ static void show_file(const char *prefix\n \t\tchar *newbase = malloc_base(base, path, strlen(path));\n \t\tvoid *tree;\n \n-\t\ttree = read_sha1_file(sha1, type, &size);\n+\t\ttree = malloc_read_sha1_file(sha1, type, &size);\n \t\tif (!tree || strcmp(type, \"tree\"))\n \t\t\tdie(\"corrupt tree sha %s\", sha1_to_hex(sha1));\n \n@@ -164,10 +164,10 @@ static int diff_tree_sha1(const unsigned\n \tchar type[20];\n \tint retval;\n \n-\ttree1 = read_sha1_file(old, type, &size1);\n+\ttree1 = malloc_read_sha1_file(old, type, &size1);\n \tif (!tree1 || strcmp(type, \"tree\"))\n \t\tdie(\"unable to read source tree (%s)\", sha1_to_hex(old));\n-\ttree2 = read_sha1_file(new, type, &size2);\n+\ttree2 = malloc_read_sha1_file(new, type, &size2);\n \tif (!tree2 || strcmp(type, \"tree\"))\n \t\tdie(\"unable to read destination tree (%s)\", sha1_to_hex(new));\n \tretval = diff_tree(tree1, size1, tree2, size2, base);\n--- show-diff.c.orig\n+++ show-diff.c\n@@ -25,7 +25,7 @@ static void show_diff_empty(struct cache\n \tint lines=0;\n \tunsigned char type[20], *p, *end;\n \n-\told = read_sha1_file(ce->sha1, type, &size);\n+\told = malloc_read_sha1_file(ce->sha1, type, &size);\n \tif (size > 0) {\n \t\tint startline = 1;\n \t\tint c = 0;\n@@ -99,7 +99,7 @@ int main(int argc, char **argv)\n \t\tif (silent)\n \t\t\tcontinue;\n \n-\t\tnew = read_sha1_file(ce->sha1, type, &size);\n+\t\tnew = malloc_read_sha1_file(ce->sha1, type, &size);\n \t\tshow_differences(ce->name, new, size);\n \t\tfree(new);\n \t}\n--- read-cache.c.orig\n+++ read-cache.c\n@@ -183,7 +183,7 @@ void * unpack_sha1_file(void *map, unsig\n \treturn buf;\n }\n \n-void * read_sha1_file(const unsigned char *sha1, char *type, unsigned long *size)\n+void * malloc_read_sha1_file(const unsigned char *sha1, char *type, unsigned long *size)\n {\n \tunsigned long mapsize;\n \tvoid *map, *buf;\n--- update-cache.c.orig\n+++ update-cache.c\n@@ -141,7 +141,7 @@ static int compare_data(struct cache_ent\n \t\tunsigned long size;\n \t\tchar type[20];\n \n-\t\tbuffer = read_sha1_file(ce->sha1, type, &size);\n+\t\tbuffer = malloc_read_sha1_file(ce->sha1, type, &size);\n \t\tif (buffer) {\n \t\t\tif (size == expected_size && !strcmp(type, \"blob\"))\n \t\t\t\tmatch = match_data(fd, buffer, size);\n--- ls-tree.c.orig\n+++ ls-tree.c\n@@ -11,7 +11,7 @@ static int list(unsigned char *sha1)\n \tunsigned long size;\n \tchar type[20];\n \n-\tbuffer = read_sha1_file(sha1, type, &size);\n+\tbuffer = malloc_read_sha1_file(sha1, type, &size);\n \tif (!buffer)\n \t\tdie(\"unable to read sha1 file\");\n \tif (strcmp(type, \"tree\"))\n--- checkout-cache.c.orig\n+++ checkout-cache.c\n@@ -72,7 +72,7 @@ static int write_entry(struct cache_entr\n \tlong wrote;\n \tchar type[20];\n \n-\tnew = read_sha1_file(ce->sha1, type, &size);\n+\tnew = malloc_read_sha1_file(ce->sha1, type, &size);\n \tif (!new || strcmp(type, \"blob\")) {\n \t\tif (new)\n \t\t\tfree(new);\n--- cache.h.orig\n+++ cache.h\n@@ -95,7 +95,7 @@ extern int write_sha1_buffer(const unsig\n /* Read and unpack a sha1 file into memory, write memory to a sha1 file */\n extern void * map_sha1_file(const unsigned char *sha1, unsigned long *size);\n extern void * unpack_sha1_file(void *map, unsigned long mapsize, char *type, unsigned long *size);\n-extern void * read_sha1_file(const unsigned char *sha1, char *type, unsigned long *size);\n+extern void * malloc_read_sha1_file(const unsigned char *sha1, char *type, unsigned long *size);\n extern int write_sha1_file(char *buf, unsigned len, unsigned char *return_sha1);\n extern int check_sha1_signature(unsigned char *sha1, void *buf, unsigned long size);\n \n--- rev-tree.c.orig\n+++ rev-tree.c\n@@ -73,7 +73,7 @@ static int parse_commit(unsigned char *s\n \t\tunsigned char parent[20];\n \n \t\trev->flags |= SEEN;\n-\t\tbuffer = bufptr = read_sha1_file(sha1, type, &size);\n+\t\tbuffer = bufptr = malloc_read_sha1_file(sha1, type, &size);\n \t\tif (!buffer || strcmp(type, \"commit\")) {\n \t\t\tif (buffer)\n \t\t\t\tfree(buffer);\n--- read-tree.c.orig\n+++ read-tree.c\n@@ -27,7 +27,7 @@ static int read_tree(unsigned char *sha1\n \tunsigned long size;\n \tchar type[20];\n \n-\tbuffer0 = buffer = read_sha1_file(sha1, type, &size);\n+\tbuffer0 = buffer = malloc_read_sha1_file(sha1, type, &size);\n \tif (!buffer)\n \t\treturn -1;\n \tif (strcmp(type, \"tree\")) {\n\n"},{"id":"77","messageId":"20050414123258.GA15148@elte.hu","threadId":"15","inReplyTo":"20050414120834.GA14290@elte.hu","subject":"[patch] git: fix memory leaks in read-cache.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T12:32:58Z","receivedAt":"2005-04-14T12:32:58Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch fixes a common and a rare memory leak in read-cache.c.\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- read-cache.c.orig\n+++ read-cache.c\n@@ -226,8 +226,11 @@ int write_sha1_file(char *buf, unsigned \n \tSHA1_Update(&c, compressed, size);\n \tSHA1_Final(sha1, &c);\n \n-\tif (write_sha1_buffer(sha1, compressed, size) < 0)\n+\tif (write_sha1_buffer(sha1, compressed, size) < 0) {\n+\t\tfree(compressed);\n \t\treturn -1;\n+\t}\n+\tfree(compressed);\n \tif (returnsha1)\n \t\tmemcpy(returnsha1, sha1, 20);\n \treturn 0;\n"},{"id":"78","messageId":"20050414123934.GA15420@elte.hu","threadId":"15","inReplyTo":"20050414123258.GA15148@elte.hu","subject":"[patch] git: fix memory leak #2 in read-cache.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T12:39:34Z","receivedAt":"2005-04-14T12:39:34Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch fixes a memory leak in read-cache.c: when there's cache entry \ncollision we should free the previous one.\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- read-cache.c.orig\n+++ read-cache.c\n@@ -369,6 +369,7 @@ int add_cache_entry(struct cache_entry *\n \n \t/* existing match? Just replace it */\n \tif (pos >= 0) {\n+\t\tfree(active_cache[pos]);\n \t\tactive_cache[pos] = ce;\n \t\treturn 0;\n \t}\n"},{"id":"79","messageId":"20050414125354.GB15420@elte.hu","threadId":"15","inReplyTo":"20050414121819.GA14380@elte.hu","subject":"[patch] git: fix 1-byte overflow in show-files.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T12:53:54Z","receivedAt":"2005-04-14T12:53:54Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch fixes a 1-byte overflow in show-files.c (looks narrow is is \nprobably not exploitable). A specially crafted db object (tree) might \ntrigger this overflow.\n\n'fullname' is an array of 4096+1 bytes, and we do readdir(), which \nproduces entries that have strings with a length of 0-255 bytes. With a \nlong enough 'base', it's possible to construct a tree with a name in it \nthat has directory whose name ends precisely at offset 4095. At that \npoint this code:\n\n                        case DT_DIR:\n                                memcpy(fullname + baselen + len, \"/\", 2);\n\nwill attempt to append a \"/\" string to the directory name - resulting in \na 1-byte overflow (a zero byte is written to offset 4097, which is \noutside the array).\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- show-files.c.orig\n+++ show-files.c\n@@ -49,7 +49,7 @@ static void read_directory(const char *p\n \n \tif (dir) {\n \t\tstruct dirent *de;\n-\t\tchar fullname[MAXPATHLEN + 1];\n+\t\tchar fullname[MAXPATHLEN + 2]; // +1 byte for trailing slash\n \t\tmemcpy(fullname, base, baselen);\n \n \t\twhile ((de = readdir(dir)) != NULL) {\n\n"},{"id":"80","messageId":"20050414130309.GA18703@elte.hu","threadId":"15","inReplyTo":"20050414121819.GA14380@elte.hu","subject":"[patch] git: fix memory leaks in update-cache.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T13:03:09Z","receivedAt":"2005-04-14T13:03:09Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch fixes two memory leaks in update-cache.c: we didnt free the \ntemporary input and output buffers used for compression.\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- update-cache.c.orig\n+++ update-cache.c\n@@ -23,13 +23,17 @@ static int index_fd(const char *path, in\n \tvoid *metadata = malloc(namelen + 200);\n \tvoid *in;\n \tSHA_CTX c;\n+\tint ret;\n \n \tin = \"\";\n \tif (size)\n \t\tin = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n-\tif (!out || (int)(long)in == -1)\n+\tif (!out || (int)(long)in == -1) {\n+\t\tfree(out);\n+\t\tfree(metadata);\n \t\treturn -1;\n+\t}\n \n \tmemset(&stream, 0, sizeof(stream));\n \tdeflateInit(&stream, Z_BEST_COMPRESSION);\n@@ -58,7 +62,12 @@ static int index_fd(const char *path, in\n \tSHA1_Update(&c, out, stream.total_out);\n \tSHA1_Final(ce->sha1, &c);\n \n-\treturn write_sha1_buffer(ce->sha1, out, stream.total_out);\n+\tret = write_sha1_buffer(ce->sha1, out, stream.total_out);\n+\t\t\n+\tfree(out);\n+\tfree(metadata);\n+\n+\treturn ret;\n }\n \n /*\n"},{"id":"81","messageId":"20050414130829.GB18703@elte.hu","threadId":"15","inReplyTo":"20050414130309.GA18703@elte.hu","subject":"[patch] git: clean up add_file_to_cache() in update-cache.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T13:08:29Z","receivedAt":"2005-04-14T13:08:29Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch cleans up add_file_to_cache() to free up all memory it \nallocates. This has no significance right now as the only user of \nadd_file_to_cache() die()s immediately in the 'leak' paths - but if the \nfunction is ever used without dying then this uncleanliness could lead \nto a memory leak.\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- update-cache.c.orig\n+++ update-cache.c\n@@ -120,10 +120,17 @@ static int add_file_to_cache(char *path)\n \tce->st_size = st.st_size;\n \tce->namelen = namelen;\n \n-\tif (index_fd(path, namelen, ce, fd, &st) < 0)\n+\tif (index_fd(path, namelen, ce, fd, &st) < 0) {\n+\t\tfree(ce);\n \t\treturn -1;\n+\t}\n \n-\treturn add_cache_entry(ce, allow_add);\n+\tif (add_cache_entry(ce, allow_add)) {\n+\t\tfree(ce);\n+\t\treturn -1;\n+\t}\n+\n+\treturn 0;\n }\n \n static int match_data(int fd, void *buffer, unsigned long size)\n\n"},{"id":"82","messageId":"20050414131232.GA21543@elte.hu","threadId":"15","inReplyTo":"20050414120834.GA14290@elte.hu","subject":"[patch] git: fix memory leak in write-tree.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T13:12:32Z","receivedAt":"2005-04-14T13:12:32Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthis patch fixes a memory leak in write-tree.c's write_tree() function - \nwe didnt free the temporary output buffer. Depending on the size of the \ntree written out this could leak a significant amount of RAM.\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- write-tree.c.orig\n+++ write-tree.c\n@@ -33,12 +33,12 @@ static int write_tree(struct cache_entry\n {\n \tunsigned char subdir_sha1[20];\n \tunsigned long size, offset;\n-\tchar *buffer;\n+\tchar *buffer0, *buffer;\n \tint i, nr;\n \n \t/* Guess at some random initial size */\n \tsize = 8192;\n-\tbuffer = malloc(size);\n+\tbuffer0 = buffer = malloc(size);\n \toffset = ORIG_OFFSET;\n \n \tnr = 0;\n@@ -97,6 +97,8 @@ static int write_tree(struct cache_entry\n \toffset -= i;\n \n \twrite_sha1_file(buffer, offset, returnsha1);\n+\tfree(buffer0);\n+\n \treturn nr;\n }\n \n"},{"id":"83","messageId":"20050414132550.GA25496@elte.hu","threadId":"15","inReplyTo":"20050414123934.GA15420@elte.hu","subject":"Re: [patch] git: fix memory leak #2 in read-cache.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T13:25:50Z","receivedAt":"2005-04-14T13:25:50Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\n* Ingo Molnar <mingo@elte.hu> wrote:\n\n> this patch fixes a memory leak in read-cache.c: when there's cache \n> entry collision we should free the previous one.\n\n> +\t\tfree(active_cache[pos]);\n>  \t\tactive_cache[pos] = ce;\n\ni'm having second thoughs about this one: active_cache entries are not \nalways malloc()-ed - e.g. read_cache() will construct them from the \nmmap() of the index file. Which must not be free()d!\n\none safe solution would be to malloc() all these entries and copy them \nover from the index file? Slightly slower but safer and free()-able when \nupdate-cache.c notices a collision. The (tested) patch below does this.\n\nthis would also make Martin Schlemmer's update-cache.c fix safe.\n\n(without this second patch, free(active_cache[pos]) might crash, and \nthat crash is would possibly be remote exploitable via a special \nrepository that tricks the index file to look in a certain way.)\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- read-cache.c.orig\n+++ read-cache.c\n@@ -453,10 +453,17 @@ int read_cache(void)\n \n \toffset = sizeof(*hdr);\n \tfor (i = 0; i < hdr->entries; i++) {\n-\t\tstruct cache_entry *ce = map + offset;\n+\t\tstruct cache_entry *ce = map + offset, *tmp;\n \t\toffset = offset + ce_size(ce);\n-\t\tactive_cache[i] = ce;\n+\n+\t\ttmp = malloc(ce_size(ce));\n+\t\tif (!tmp)\n+\t\t\treturn error(\"malloc failed\");\n+\t\tmemcpy(tmp, ce, ce_size(ce));\n+\t\tactive_cache[i] = tmp;\n \t}\n+\tmunmap(map, size);\n+\n \treturn active_nr;\n \n unmap:\n\n"},{"id":"84","messageId":"20050414133134.GB21543@elte.hu","threadId":"15","inReplyTo":"20050414130309.GA18703@elte.hu","subject":"[patch] git: fix memory leak #3 in update-cache.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T13:31:34Z","receivedAt":"2005-04-14T13:31:34Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\nthe patch below fixes a third memory leak in update-cache.c. (the \nmmap-ed file needs to be unmapped too) Ontop of the previous \nupdate-cache.c patches.\n\n\tIngo\n\nSigned-off-by: Ingo Molnar <mingo@elte.hu>\n\n--- update-cache.c.orig\n+++ update-cache.c\n@@ -32,6 +32,8 @@ static int index_fd(const char *path, in\n \tif (!out || (int)(long)in == -1) {\n \t\tfree(out);\n \t\tfree(metadata);\n+\t\tif ((int)(long)in != -1 && size)\n+\t\t\tmunmap(in, size);\n \t\treturn -1;\n \t}\n \n@@ -66,6 +68,8 @@ static int index_fd(const char *path, in\n \t\t\n \tfree(out);\n \tfree(metadata);\n+\tif (size)\n+\t\tmunmap(in, size);\n \n \treturn ret;\n }\n"},{"id":"86","messageId":"1113488724.23299.106.camel@nosferatu.lan","threadId":"15","inReplyTo":"20050414132550.GA25496@elte.hu","subject":"Re: [patch] git: fix memory leak #2 in read-cache.c","fromName":"Martin Schlemmer","fromEmail":"azarah@nosferatu.za.org","sentAt":"2005-04-14T14:25:24Z","receivedAt":"2005-04-14T14:25:24Z","isPatch":true,"sender":{"key":"azarah@nosferatu.za.org","avatar":null},"body":"On Thu, 2005-04-14 at 15:25 +0200, Ingo Molnar wrote:\n> * Ingo Molnar <mingo@elte.hu> wrote:\n> \n> > this patch fixes a memory leak in read-cache.c: when there's cache \n> > entry collision we should free the previous one.\n> \n> > +\t\tfree(active_cache[pos]);\n> >  \t\tactive_cache[pos] = ce;\n> \n> i'm having second thoughs about this one: active_cache entries are not \n> always malloc()-ed - e.g. read_cache() will construct them from the \n> mmap() of the index file. Which must not be free()d!\n> \n> one safe solution would be to malloc() all these entries and copy them \n> over from the index file? Slightly slower but safer and free()-able when \n> update-cache.c notices a collision. The (tested) patch below does this.\n> \n> this would also make Martin Schlemmer's update-cache.c fix safe.\n> \n\nOk, bad me it seems - assumed it was malloc'd.\n\n\n-- \nMartin Schlemmer\n\n"},{"id":"87","messageId":"Pine.LNX.4.58.0504140804480.7211@ppc970.osdl.org","threadId":"15","inReplyTo":"20050414123934.GA15420@elte.hu","subject":"Re: [patch] git: fix memory leak #2 in read-cache.c","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-04-14T15:11:47Z","receivedAt":"2005-04-14T15:11:47Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 14 Apr 2005, Ingo Molnar wrote:\n> \n> this patch fixes a memory leak in read-cache.c: when there's cache entry \n> collision we should free the previous one.\n\nAs you already noticed \"read_cache()\" normally just populates the\nactive-cache with pointers to the mmap'ed \"active\" file.\n\nWhether that is a good idea or not, I do not know. But I do know that the \nactive file is the single biggest file we work with (ie 1.6MB in size), so \nsince _most_ tools just read it and modify a very small number of entries, \nit seemed like a good idea.\n\nIn other words, if the common case is that we update a couple of entries\nin the active cache, we actually saved 1.6MB (+ malloc overhead for the 17\n_thousand_ allocations) by my approach.\n\nAnd the leak? There's none. We never actually update an existing entry \nthat was allocated with malloc(), unless the user does something stupid. \nIn other words, the only case where there is a \"leak\" is when the user \ndoes something like\n\n\tupdate-cache file file file file file file .. \n\nwith the same file listed several times.\n\nAnd dammit, the whole point of doing stuff in user space is that the \nkernel takes care of business. Unlike kernel work, leaking is ok. You just \nhave to make sure that it is limited enough to to not be a problem. I'm \nsaying that in this case we're _better_ off leaking, because the mmap() \ntrick saves us more memory than the leak can ever leak.\n\n(The command line is limited to 128kB or so, which means that the most \nfiles you _can_ add with a single update-cache is _less_ than the mmap \nwin).\n\nIt was _such_ a relief to program in user mode for a change. Not having to \ncare about the small stuff is wonderful.\n\n\t\tLinus\n"},{"id":"88","messageId":"20050414151456.GA11385@elte.hu","threadId":"15","inReplyTo":"Pine.LNX.4.58.0504140804480.7211@ppc970.osdl.org","subject":"Re: [patch] git: fix memory leak #2 in read-cache.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-14T15:14:56Z","receivedAt":"2005-04-14T15:14:56Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\n* Linus Torvalds <torvalds@osdl.org> wrote:\n\n> In other words, if the common case is that we update a couple of \n> entries in the active cache, we actually saved 1.6MB (+ malloc \n> overhead for the 17 _thousand_ allocations) by my approach.\n> \n> And the leak? There's none. We never actually update an existing entry \n> that was allocated with malloc(), unless the user does something \n> stupid.  In other words, the only case where there is a \"leak\" is when \n> the user does something like\n> \n> \tupdate-cache file file file file file file .. \n> \n> with the same file listed several times.\n\nfair enough - as long as this is only used in a scripted environment, \nand not via some library and not within a repository server, web \nbackend, etc.\n\n\tIngo\n"},{"id":"578","messageId":"20050417235920.GY1461@pasky.ji.cz","threadId":"15","inReplyTo":"20050414125354.GB15420@elte.hu","subject":"Re: [patch] git: fix 1-byte overflow in show-files.c","fromName":"Petr Baudis","fromEmail":"pasky@ucw.cz","sentAt":"2005-04-17T23:59:20Z","receivedAt":"2005-04-17T23:59:20Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Thu, Apr 14, 2005 at 02:53:54PM CEST, I got a letter\nwhere Ingo Molnar <mingo@elte.hu> told me that...\n> \n> this patch fixes a 1-byte overflow in show-files.c (looks narrow is is \n> probably not exploitable). A specially crafted db object (tree) might \n> trigger this overflow.\n> \n> 'fullname' is an array of 4096+1 bytes, and we do readdir(), which \n> produces entries that have strings with a length of 0-255 bytes. With a \n> long enough 'base', it's possible to construct a tree with a name in it \n> that has directory whose name ends precisely at offset 4095. At that \n> point this code:\n> \n>                         case DT_DIR:\n>                                 memcpy(fullname + baselen + len, \"/\", 2);\n> \n> will attempt to append a \"/\" string to the directory name - resulting in \n> a 1-byte overflow (a zero byte is written to offset 4097, which is \n> outside the array).\n\nThe name ends precisely at offset 4095 with its NUL character:\n\n     {PATH_MAX}\n     Maximum number of bytes in a pathname, including the terminating\nnull character.\n[ http://www.opengroup.org/onlinepubs/009695399/basedefs/limits.h.html ]\n\nSo, if I'm not mistaken, '/' will be written at offset 4095 instead of\nthe NUL and the NUL will be written at 4096. Everything's fine, right?\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nC++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor\n"},{"id":"635","messageId":"20050418082827.GA21612@elte.hu","threadId":"15","inReplyTo":"20050417235920.GY1461@pasky.ji.cz","subject":"Re: [patch] git: fix 1-byte overflow in show-files.c","fromName":"Ingo Molnar","fromEmail":"mingo@elte.hu","sentAt":"2005-04-18T08:28:27Z","receivedAt":"2005-04-18T08:28:27Z","isPatch":true,"sender":{"key":"mingo@elte.hu","avatar":null},"body":"\n* Petr Baudis <pasky@ucw.cz> wrote:\n\n> > will attempt to append a \"/\" string to the directory name - resulting in \n> > a 1-byte overflow (a zero byte is written to offset 4097, which is \n> > outside the array).\n> \n> The name ends precisely at offset 4095 with its NUL character:\n> \n>      {PATH_MAX}\n>      Maximum number of bytes in a pathname, including the terminating\n> null character.\n> [ http://www.opengroup.org/onlinepubs/009695399/basedefs/limits.h.html ]\n> \n> So, if I'm not mistaken, '/' will be written at offset 4095 instead of \n> the NUL and the NUL will be written at 4096. Everything's fine, right?\n\nyeah, you are right - ignore this patch.\n\n\tIngo\n"}]}