# [patch] git: fix memory leak in checkout-cache.c 22 messages from 2005-04-14 to 2005-04-18. Participants: Ingo Molnar, Martin Schlemmer, Linus Torvalds, Petr Baudis. Thread: https://gitlist.dev/t/15 ## Ingo Molnar, 2005-04-14 11:26 Subject: [patch] git: fix memory leak in checkout-cache.c Message-ID: <20050414112638.GA12593@elte.hu> URL: https://gitlist.dev/e/20050414112638.GA12593%40elte.hu ``` this patch fixes a memory leak in checkout-cache. Ingo Signed-off-by: Ingo Molnar --- checkout-cache.c.orig +++ checkout-cache.c @@ -48,6 +48,7 @@ static void create_directories(const cha buf[len] = 0; mkdir(buf, 0755); } + free(buf); } static int create_file(const char *path, unsigned int mode) ``` ## Ingo Molnar, 2005-04-14 11:35 Subject: [patch] git: fix memory leak #2 in checkout-cache.c Message-ID: <20050414113527.GA13790@elte.hu> URL: https://gitlist.dev/e/20050414113527.GA13790%40elte.hu In-Reply-To: <20050414112638.GA12593@elte.hu> ``` this patch fixes another (very rare) memory leak in checkout-cache. Ingo Signed-off-by: Ingo Molnar --- checkout-cache.c.orig +++ checkout-cache.c @@ -74,6 +74,8 @@ static int write_entry(struct cache_entr new = read_sha1_file(ce->sha1, type, &size); if (!new || strcmp(type, "blob")) { + if (new) + free(new); return error("checkout-cache: unable to read sha1 file of %s (%s)", ce->name, sha1_to_hex(ce->sha1)); } ``` ## Ingo Molnar, 2005-04-14 11:43 Subject: [patch] git: cleanup in ls-tree.c Message-ID: <20050414114344.GA13879@elte.hu> URL: https://gitlist.dev/e/20050414114344.GA13879%40elte.hu In-Reply-To: <20050414113527.GA13790@elte.hu> ``` cleanup: this patch adds a free() to ls-tree.c. (Technically it's not a memory leak yet because the buffer is allocated once by the function and then the utility exits - but it's a tad cleaner to not leave such assumptions in the code, so that if someone reuses the function (or extends the utility to include a loop) the uncleanliness doesnt develop into a real memory leak.) Ingo Signed-off-by: Ingo Molnar --- ls-tree.c.orig +++ ls-tree.c @@ -33,6 +33,8 @@ static int list(unsigned char *sha1) type = S_ISDIR(mode) ? "tree" : "blob"; printf("%03o\t%s\t%s\t%s\n", mode, type, sha1_to_hex(sha1), path); } + free(buffer); + return 0; } ``` ## Ingo Molnar, 2005-04-14 11:54 Subject: [patch] git: fix memory leaks in read-tree.c Message-ID: <20050414115422.GA14065@elte.hu> URL: https://gitlist.dev/e/20050414115422.GA14065%40elte.hu In-Reply-To: <20050414113527.GA13790@elte.hu> ``` this patch fixes one common, and 4 rare memory leaks in read-tree.c. Ingo Signed-off-by: Ingo Molnar --- read-tree.c.orig +++ read-tree.c @@ -23,23 +23,27 @@ static int read_one_entry(unsigned char static int read_tree(unsigned char *sha1, const char *base, int baselen) { - void *buffer; + void *buffer0, *buffer; unsigned long size; char type[20]; - buffer = read_sha1_file(sha1, type, &size); + buffer0 = buffer = read_sha1_file(sha1, type, &size); if (!buffer) return -1; - if (strcmp(type, "tree")) + if (strcmp(type, "tree")) { + free(buffer); return -1; + } while (size) { int len = strlen(buffer)+1; unsigned char *sha1 = buffer + len; char *path = strchr(buffer, ' ')+1; unsigned int mode; - if (size < len + 20 || sscanf(buffer, "%o", &mode) != 1) + if (size < len + 20 || sscanf(buffer, "%o", &mode) != 1) { + free(buffer0); return -1; + } buffer = sha1 + 20; size -= len + 20; @@ -53,13 +57,19 @@ static int read_tree(unsigned char *sha1 newbase[baselen + pathlen] = '/'; retval = read_tree(sha1, newbase, baselen + pathlen + 1); free(newbase); - if (retval) + if (retval) { + free(buffer0); return -1; + } continue; } - if (read_one_entry(sha1, base, baselen, path, mode) < 0) + if (read_one_entry(sha1, base, baselen, path, mode) < 0) { + free(buffer0); return -1; + } } + free(buffer0); + return 0; } ``` ## Ingo Molnar, 2005-04-14 11:58 Subject: [patch] git: fix rare memory leak in rev-tree.c Message-ID: <20050414115800.GB14065@elte.hu> URL: https://gitlist.dev/e/20050414115800.GB14065%40elte.hu In-Reply-To: <20050414113527.GA13790@elte.hu> ``` this patch fixes a rare memory leak in rev-tree.c. Ingo Signed-off-by: Ingo Molnar --- rev-tree.c.orig +++ rev-tree.c @@ -73,8 +73,11 @@ static int parse_commit(unsigned char *s rev->flags |= SEEN; buffer = bufptr = read_sha1_file(sha1, type, &size); - if (!buffer || strcmp(type, "commit")) + if (!buffer || strcmp(type, "commit")) { + if (buffer) + free(buffer); return -1; + } bufptr += 46; /* "tree " + "hex sha1" + "\n" */ while (!memcmp(bufptr, "parent ", 7) && !get_sha1_hex(bufptr+7, parent)) { add_relationship(rev, parent); ``` ## Ingo Molnar, 2005-04-14 12:05 Subject: [patch] git: report parse_commit() errors in rev-tree.c Message-ID: <20050414120516.GC14065@elte.hu> URL: https://gitlist.dev/e/20050414120516.GC14065%40elte.hu In-Reply-To: <20050414115800.GB14065@elte.hu> ``` make actual use of the parse_commit() return value and print a warning, instead of silently ignoring it. Should never trigger on a valid DB. (alternatively we might use a die() in the sanity check place and could remove all the return code handling?) Ingo Signed-off-by: Ingo Molnar --- rev-tree.c.orig +++ rev-tree.c @@ -64,6 +64,7 @@ static unsigned long parse_commit_date(c static int parse_commit(unsigned char *sha1) { struct revision *rev = lookup_rev(sha1); + int ret = 0; if (!(rev->flags & SEEN)) { void *buffer, *bufptr; @@ -81,13 +82,13 @@ static int parse_commit(unsigned char *s bufptr += 46; /* "tree " + "hex sha1" + "\n" */ while (!memcmp(bufptr, "parent ", 7) && !get_sha1_hex(bufptr+7, parent)) { add_relationship(rev, parent); - parse_commit(parent); + ret |= parse_commit(parent); bufptr += 48; /* "parent " + "hex sha1" + "\n" */ } rev->date = parse_commit_date(bufptr); free(buffer); } - return 0; + return ret; } static void read_cache_file(const char *path) @@ -208,7 +209,8 @@ int main(int argc, char **argv) } if (nr >= MAX_COMMITS || get_sha1_hex(arg, sha1[nr])) usage("rev-tree [--edges] [--cache ] []"); - parse_commit(sha1[nr]); + if (parse_commit(sha1[nr])) + fprintf(stderr, "warning: rev-tree: bad commit!\n"); nr++; } ``` ## Ingo Molnar, 2005-04-14 12:08 Subject: [patch] git: fix memory leak in show-diff.c Message-ID: <20050414120834.GA14290@elte.hu> URL: https://gitlist.dev/e/20050414120834.GA14290%40elte.hu In-Reply-To: <20050414112638.GA12593@elte.hu> ``` this patch fixes a memory leak in show-diff.c. Ingo Signed-off-by: Ingo Molnar --- show-diff.c.orig +++ show-diff.c @@ -53,6 +53,7 @@ static void show_diff_empty(struct cache printf("\n"); fflush(stdout); } + free(old); } int main(int argc, char **argv) ``` ## Ingo Molnar, 2005-04-14 12:18 Subject: [patch] git: fix overflow in update-cache.c Message-ID: <20050414121819.GA14380@elte.hu> URL: https://gitlist.dev/e/20050414121819.GA14380%40elte.hu In-Reply-To: <20050414120834.GA14290@elte.hu> ``` this patch fixes a 1-byte overflow in update-cache.c (probably not exploitable). A specially crafted db object might trigger this overflow. the bug is that normally the 'type' field is parsed by read_sha1_file(), via: if (sscanf(buffer, "%10s %lu", type, size) != 2) i.e. 0-10 long strings, which take 1-11 bytes of space. Normally the type strings are stored in char [20] arrays, but in update-cache.c that is char [10], so a 1 byte overflow might occur. This should not happen with a 'friendly' DB, as the longest type string ("commit") is 7 bytes long. The fix is to use the customary char [20]. (someone might want to clean those open-coded constants up with a TYPE_LEN define, they do tend to cause problems like this. I'm not against open-coded constants (they make code much more readable), but for fields that get filled in from possibly hostile objects this is playing with fire.) hey, this might be the first true security fix for GIT? ;-) Ingo Signed-off-by: Ingo Molnar --- update-cache.c.orig +++ update-cache.c @@ -139,7 +139,7 @@ static int compare_data(struct cache_ent if (fd >= 0) { void *buffer; unsigned long size; - char type[10]; + char type[20]; buffer = read_sha1_file(ce->sha1, type, &size); if (buffer) { ``` ## Ingo Molnar, 2005-04-14 12:23 Subject: [patch] cleanup: read_sha1_file() -> malloc_read_sha1_file() Message-ID: <20050414122352.GA15049@elte.hu> URL: https://gitlist.dev/e/20050414122352.GA15049%40elte.hu In-Reply-To: <20050414121819.GA14380@elte.hu> ``` this patch renames read_sha1_file() to malloc_read_sha1_file(). There were a handful of memory-leaks related to read_sha1_file(), and some of those could possibly have been found sooner if the name indicated that an implicit malloc() is occurs within read_sha1_file(). This patch is ontop of the previous patches i sent. Ingo Signed-off-by: Ingo Molnar --- cat-file.c.orig +++ cat-file.c @@ -14,7 +14,7 @@ int main(int argc, char **argv) if (argc != 3 || get_sha1_hex(argv[2], sha1)) usage("cat-file [-t | tagname] "); - buf = read_sha1_file(sha1, type, &size); + buf = malloc_read_sha1_file(sha1, type, &size); if (!buf) die("cat-file %s: bad file", argv[2]); if (!strcmp("-t", argv[1])) { --- merge-tree.c.orig +++ merge-tree.c @@ -11,7 +11,7 @@ static struct tree_entry *read_tree(unsi { char type[20]; unsigned long size; - void *buf = read_sha1_file(sha1, type, &size); + void *buf = malloc_read_sha1_file(sha1, type, &size); struct tree_entry *ret = NULL, **tp = &ret; if (!buf || strcmp(type, "tree")) --- diff-tree.c.orig +++ diff-tree.c @@ -62,7 +62,7 @@ static void show_file(const char *prefix char *newbase = malloc_base(base, path, strlen(path)); void *tree; - tree = read_sha1_file(sha1, type, &size); + tree = malloc_read_sha1_file(sha1, type, &size); if (!tree || strcmp(type, "tree")) die("corrupt tree sha %s", sha1_to_hex(sha1)); @@ -164,10 +164,10 @@ static int diff_tree_sha1(const unsigned char type[20]; int retval; - tree1 = read_sha1_file(old, type, &size1); + tree1 = malloc_read_sha1_file(old, type, &size1); if (!tree1 || strcmp(type, "tree")) die("unable to read source tree (%s)", sha1_to_hex(old)); - tree2 = read_sha1_file(new, type, &size2); + tree2 = malloc_read_sha1_file(new, type, &size2); if (!tree2 || strcmp(type, "tree")) die("unable to read destination tree (%s)", sha1_to_hex(new)); retval = diff_tree(tree1, size1, tree2, size2, base); --- show-diff.c.orig +++ show-diff.c @@ -25,7 +25,7 @@ static void show_diff_empty(struct cache int lines=0; unsigned char type[20], *p, *end; - old = read_sha1_file(ce->sha1, type, &size); + old = malloc_read_sha1_file(ce->sha1, type, &size); if (size > 0) { int startline = 1; int c = 0; @@ -99,7 +99,7 @@ int main(int argc, char **argv) if (silent) continue; - new = read_sha1_file(ce->sha1, type, &size); + new = malloc_read_sha1_file(ce->sha1, type, &size); show_differences(ce->name, new, size); free(new); } --- read-cache.c.orig +++ read-cache.c @@ -183,7 +183,7 @@ void * unpack_sha1_file(void *map, unsig return buf; } -void * read_sha1_file(const unsigned char *sha1, char *type, unsigned long *size) +void * malloc_read_sha1_file(const unsigned char *sha1, char *type, unsigned long *size) { unsigned long mapsize; void *map, *buf; --- update-cache.c.orig +++ update-cache.c @@ -141,7 +141,7 @@ static int compare_data(struct cache_ent unsigned long size; char type[20]; - buffer = read_sha1_file(ce->sha1, type, &size); + buffer = malloc_read_sha1_file(ce->sha1, type, &size); if (buffer) { if (size == expected_size && !strcmp(type, "blob")) match = match_data(fd, buffer, size); --- ls-tree.c.orig +++ ls-tree.c @@ -11,7 +11,7 @@ static int list(unsigned char *sha1) unsigned long size; char type[20]; - buffer = read_sha1_file(sha1, type, &size); + buffer = malloc_read_sha1_file(sha1, type, &size); if (!buffer) die("unable to read sha1 file"); if (strcmp(type, "tree")) --- checkout-cache.c.orig +++ checkout-cache.c @@ -72,7 +72,7 @@ static int write_entry(struct cache_entr long wrote; char type[20]; - new = read_sha1_file(ce->sha1, type, &size); + new = malloc_read_sha1_file(ce->sha1, type, &size); if (!new || strcmp(type, "blob")) { if (new) free(new); --- cache.h.orig +++ cache.h @@ -95,7 +95,7 @@ extern int write_sha1_buffer(const unsig /* Read and unpack a sha1 file into memory, write memory to a sha1 file */ extern void * map_sha1_file(const unsigned char *sha1, unsigned long *size); extern void * unpack_sha1_file(void *map, unsigned long mapsize, char *type, unsigned long *size); -extern void * read_sha1_file(const unsigned char *sha1, char *type, unsigned long *size); +extern void * malloc_read_sha1_file(const unsigned char *sha1, char *type, unsigned long *size); extern int write_sha1_file(char *buf, unsigned len, unsigned char *return_sha1); extern int check_sha1_signature(unsigned char *sha1, void *buf, unsigned long size); --- rev-tree.c.orig +++ rev-tree.c @@ -73,7 +73,7 @@ static int parse_commit(unsigned char *s unsigned char parent[20]; rev->flags |= SEEN; - buffer = bufptr = read_sha1_file(sha1, type, &size); + buffer = bufptr = malloc_read_sha1_file(sha1, type, &size); if (!buffer || strcmp(type, "commit")) { if (buffer) free(buffer); --- read-tree.c.orig +++ read-tree.c @@ -27,7 +27,7 @@ static int read_tree(unsigned char *sha1 unsigned long size; char type[20]; - buffer0 = buffer = read_sha1_file(sha1, type, &size); + buffer0 = buffer = malloc_read_sha1_file(sha1, type, &size); if (!buffer) return -1; if (strcmp(type, "tree")) { ``` ## Ingo Molnar, 2005-04-14 12:32 Subject: [patch] git: fix memory leaks in read-cache.c Message-ID: <20050414123258.GA15148@elte.hu> URL: https://gitlist.dev/e/20050414123258.GA15148%40elte.hu In-Reply-To: <20050414120834.GA14290@elte.hu> ``` this patch fixes a common and a rare memory leak in read-cache.c. Ingo Signed-off-by: Ingo Molnar --- read-cache.c.orig +++ read-cache.c @@ -226,8 +226,11 @@ int write_sha1_file(char *buf, unsigned SHA1_Update(&c, compressed, size); SHA1_Final(sha1, &c); - if (write_sha1_buffer(sha1, compressed, size) < 0) + if (write_sha1_buffer(sha1, compressed, size) < 0) { + free(compressed); return -1; + } + free(compressed); if (returnsha1) memcpy(returnsha1, sha1, 20); return 0; ``` ## Ingo Molnar, 2005-04-14 12:39 Subject: [patch] git: fix memory leak #2 in read-cache.c Message-ID: <20050414123934.GA15420@elte.hu> URL: https://gitlist.dev/e/20050414123934.GA15420%40elte.hu In-Reply-To: <20050414123258.GA15148@elte.hu> ``` this patch fixes a memory leak in read-cache.c: when there's cache entry collision we should free the previous one. Ingo Signed-off-by: Ingo Molnar --- read-cache.c.orig +++ read-cache.c @@ -369,6 +369,7 @@ int add_cache_entry(struct cache_entry * /* existing match? Just replace it */ if (pos >= 0) { + free(active_cache[pos]); active_cache[pos] = ce; return 0; } ``` ## Ingo Molnar, 2005-04-14 12:53 Subject: [patch] git: fix 1-byte overflow in show-files.c Message-ID: <20050414125354.GB15420@elte.hu> URL: https://gitlist.dev/e/20050414125354.GB15420%40elte.hu In-Reply-To: <20050414121819.GA14380@elte.hu> ``` this patch fixes a 1-byte overflow in show-files.c (looks narrow is is probably not exploitable). A specially crafted db object (tree) might trigger this overflow. 'fullname' is an array of 4096+1 bytes, and we do readdir(), which produces entries that have strings with a length of 0-255 bytes. With a long enough 'base', it's possible to construct a tree with a name in it that has directory whose name ends precisely at offset 4095. At that point this code: case DT_DIR: memcpy(fullname + baselen + len, "/", 2); will attempt to append a "/" string to the directory name - resulting in a 1-byte overflow (a zero byte is written to offset 4097, which is outside the array). Ingo Signed-off-by: Ingo Molnar --- show-files.c.orig +++ show-files.c @@ -49,7 +49,7 @@ static void read_directory(const char *p if (dir) { struct dirent *de; - char fullname[MAXPATHLEN + 1]; + char fullname[MAXPATHLEN + 2]; // +1 byte for trailing slash memcpy(fullname, base, baselen); while ((de = readdir(dir)) != NULL) { ``` ## Ingo Molnar, 2005-04-14 13:03 Subject: [patch] git: fix memory leaks in update-cache.c Message-ID: <20050414130309.GA18703@elte.hu> URL: https://gitlist.dev/e/20050414130309.GA18703%40elte.hu In-Reply-To: <20050414121819.GA14380@elte.hu> ``` this patch fixes two memory leaks in update-cache.c: we didnt free the temporary input and output buffers used for compression. Ingo Signed-off-by: Ingo Molnar --- update-cache.c.orig +++ update-cache.c @@ -23,13 +23,17 @@ static int index_fd(const char *path, in void *metadata = malloc(namelen + 200); void *in; SHA_CTX c; + int ret; in = ""; if (size) in = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0); close(fd); - if (!out || (int)(long)in == -1) + if (!out || (int)(long)in == -1) { + free(out); + free(metadata); return -1; + } memset(&stream, 0, sizeof(stream)); deflateInit(&stream, Z_BEST_COMPRESSION); @@ -58,7 +62,12 @@ static int index_fd(const char *path, in SHA1_Update(&c, out, stream.total_out); SHA1_Final(ce->sha1, &c); - return write_sha1_buffer(ce->sha1, out, stream.total_out); + ret = write_sha1_buffer(ce->sha1, out, stream.total_out); + + free(out); + free(metadata); + + return ret; } /* ``` ## Ingo Molnar, 2005-04-14 13:08 Subject: [patch] git: clean up add_file_to_cache() in update-cache.c Message-ID: <20050414130829.GB18703@elte.hu> URL: https://gitlist.dev/e/20050414130829.GB18703%40elte.hu In-Reply-To: <20050414130309.GA18703@elte.hu> ``` this patch cleans up add_file_to_cache() to free up all memory it allocates. This has no significance right now as the only user of add_file_to_cache() die()s immediately in the 'leak' paths - but if the function is ever used without dying then this uncleanliness could lead to a memory leak. Ingo Signed-off-by: Ingo Molnar --- update-cache.c.orig +++ update-cache.c @@ -120,10 +120,17 @@ static int add_file_to_cache(char *path) ce->st_size = st.st_size; ce->namelen = namelen; - if (index_fd(path, namelen, ce, fd, &st) < 0) + if (index_fd(path, namelen, ce, fd, &st) < 0) { + free(ce); return -1; + } - return add_cache_entry(ce, allow_add); + if (add_cache_entry(ce, allow_add)) { + free(ce); + return -1; + } + + return 0; } static int match_data(int fd, void *buffer, unsigned long size) ``` ## Ingo Molnar, 2005-04-14 13:12 Subject: [patch] git: fix memory leak in write-tree.c Message-ID: <20050414131232.GA21543@elte.hu> URL: https://gitlist.dev/e/20050414131232.GA21543%40elte.hu In-Reply-To: <20050414120834.GA14290@elte.hu> ``` this patch fixes a memory leak in write-tree.c's write_tree() function - we didnt free the temporary output buffer. Depending on the size of the tree written out this could leak a significant amount of RAM. Ingo Signed-off-by: Ingo Molnar --- write-tree.c.orig +++ write-tree.c @@ -33,12 +33,12 @@ static int write_tree(struct cache_entry { unsigned char subdir_sha1[20]; unsigned long size, offset; - char *buffer; + char *buffer0, *buffer; int i, nr; /* Guess at some random initial size */ size = 8192; - buffer = malloc(size); + buffer0 = buffer = malloc(size); offset = ORIG_OFFSET; nr = 0; @@ -97,6 +97,8 @@ static int write_tree(struct cache_entry offset -= i; write_sha1_file(buffer, offset, returnsha1); + free(buffer0); + return nr; } ``` ## Ingo Molnar, 2005-04-14 13:25 Subject: Re: [patch] git: fix memory leak #2 in read-cache.c Message-ID: <20050414132550.GA25496@elte.hu> URL: https://gitlist.dev/e/20050414132550.GA25496%40elte.hu In-Reply-To: <20050414123934.GA15420@elte.hu> ``` * Ingo Molnar wrote: > this patch fixes a memory leak in read-cache.c: when there's cache > entry collision we should free the previous one. > + free(active_cache[pos]); > active_cache[pos] = ce; i'm having second thoughs about this one: active_cache entries are not always malloc()-ed - e.g. read_cache() will construct them from the mmap() of the index file. Which must not be free()d! one safe solution would be to malloc() all these entries and copy them over from the index file? Slightly slower but safer and free()-able when update-cache.c notices a collision. The (tested) patch below does this. this would also make Martin Schlemmer's update-cache.c fix safe. (without this second patch, free(active_cache[pos]) might crash, and that crash is would possibly be remote exploitable via a special repository that tricks the index file to look in a certain way.) Ingo Signed-off-by: Ingo Molnar --- read-cache.c.orig +++ read-cache.c @@ -453,10 +453,17 @@ int read_cache(void) offset = sizeof(*hdr); for (i = 0; i < hdr->entries; i++) { - struct cache_entry *ce = map + offset; + struct cache_entry *ce = map + offset, *tmp; offset = offset + ce_size(ce); - active_cache[i] = ce; + + tmp = malloc(ce_size(ce)); + if (!tmp) + return error("malloc failed"); + memcpy(tmp, ce, ce_size(ce)); + active_cache[i] = tmp; } + munmap(map, size); + return active_nr; unmap: ``` ## Ingo Molnar, 2005-04-14 13:31 Subject: [patch] git: fix memory leak #3 in update-cache.c Message-ID: <20050414133134.GB21543@elte.hu> URL: https://gitlist.dev/e/20050414133134.GB21543%40elte.hu In-Reply-To: <20050414130309.GA18703@elte.hu> ``` the patch below fixes a third memory leak in update-cache.c. (the mmap-ed file needs to be unmapped too) Ontop of the previous update-cache.c patches. Ingo Signed-off-by: Ingo Molnar --- update-cache.c.orig +++ update-cache.c @@ -32,6 +32,8 @@ static int index_fd(const char *path, in if (!out || (int)(long)in == -1) { free(out); free(metadata); + if ((int)(long)in != -1 && size) + munmap(in, size); return -1; } @@ -66,6 +68,8 @@ static int index_fd(const char *path, in free(out); free(metadata); + if (size) + munmap(in, size); return ret; } ``` ## Martin Schlemmer, 2005-04-14 14:25 Subject: Re: [patch] git: fix memory leak #2 in read-cache.c Message-ID: <1113488724.23299.106.camel@nosferatu.lan> URL: https://gitlist.dev/e/1113488724.23299.106.camel%40nosferatu.lan In-Reply-To: <20050414132550.GA25496@elte.hu> ``` On Thu, 2005-04-14 at 15:25 +0200, Ingo Molnar wrote: > * Ingo Molnar wrote: > > > this patch fixes a memory leak in read-cache.c: when there's cache > > entry collision we should free the previous one. > > > + free(active_cache[pos]); > > active_cache[pos] = ce; > > i'm having second thoughs about this one: active_cache entries are not > always malloc()-ed - e.g. read_cache() will construct them from the > mmap() of the index file. Which must not be free()d! > > one safe solution would be to malloc() all these entries and copy them > over from the index file? Slightly slower but safer and free()-able when > update-cache.c notices a collision. The (tested) patch below does this. > > this would also make Martin Schlemmer's update-cache.c fix safe. > Ok, bad me it seems - assumed it was malloc'd. -- Martin Schlemmer ``` ## Linus Torvalds, 2005-04-14 15:11 Subject: Re: [patch] git: fix memory leak #2 in read-cache.c Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0504140804480.7211%40ppc970.osdl.org In-Reply-To: <20050414123934.GA15420@elte.hu> ``` On Thu, 14 Apr 2005, Ingo Molnar wrote: > > this patch fixes a memory leak in read-cache.c: when there's cache entry > collision we should free the previous one. As you already noticed "read_cache()" normally just populates the active-cache with pointers to the mmap'ed "active" file. Whether that is a good idea or not, I do not know. But I do know that the active file is the single biggest file we work with (ie 1.6MB in size), so since _most_ tools just read it and modify a very small number of entries, it seemed like a good idea. In other words, if the common case is that we update a couple of entries in the active cache, we actually saved 1.6MB (+ malloc overhead for the 17 _thousand_ allocations) by my approach. And the leak? There's none. We never actually update an existing entry that was allocated with malloc(), unless the user does something stupid. In other words, the only case where there is a "leak" is when the user does something like update-cache file file file file file file .. with the same file listed several times. And dammit, the whole point of doing stuff in user space is that the kernel takes care of business. Unlike kernel work, leaking is ok. You just have to make sure that it is limited enough to to not be a problem. I'm saying that in this case we're _better_ off leaking, because the mmap() trick saves us more memory than the leak can ever leak. (The command line is limited to 128kB or so, which means that the most files you _can_ add with a single update-cache is _less_ than the mmap win). It was _such_ a relief to program in user mode for a change. Not having to care about the small stuff is wonderful. Linus ``` ## Ingo Molnar, 2005-04-14 15:14 Subject: Re: [patch] git: fix memory leak #2 in read-cache.c Message-ID: <20050414151456.GA11385@elte.hu> URL: https://gitlist.dev/e/20050414151456.GA11385%40elte.hu In-Reply-To: ``` * Linus Torvalds wrote: > In other words, if the common case is that we update a couple of > entries in the active cache, we actually saved 1.6MB (+ malloc > overhead for the 17 _thousand_ allocations) by my approach. > > And the leak? There's none. We never actually update an existing entry > that was allocated with malloc(), unless the user does something > stupid. In other words, the only case where there is a "leak" is when > the user does something like > > update-cache file file file file file file .. > > with the same file listed several times. fair enough - as long as this is only used in a scripted environment, and not via some library and not within a repository server, web backend, etc. Ingo ``` ## Petr Baudis, 2005-04-17 23:59 Subject: Re: [patch] git: fix 1-byte overflow in show-files.c Message-ID: <20050417235920.GY1461@pasky.ji.cz> URL: https://gitlist.dev/e/20050417235920.GY1461%40pasky.ji.cz In-Reply-To: <20050414125354.GB15420@elte.hu> ``` Dear diary, on Thu, Apr 14, 2005 at 02:53:54PM CEST, I got a letter where Ingo Molnar told me that... > > this patch fixes a 1-byte overflow in show-files.c (looks narrow is is > probably not exploitable). A specially crafted db object (tree) might > trigger this overflow. > > 'fullname' is an array of 4096+1 bytes, and we do readdir(), which > produces entries that have strings with a length of 0-255 bytes. With a > long enough 'base', it's possible to construct a tree with a name in it > that has directory whose name ends precisely at offset 4095. At that > point this code: > > case DT_DIR: > memcpy(fullname + baselen + len, "/", 2); > > will attempt to append a "/" string to the directory name - resulting in > a 1-byte overflow (a zero byte is written to offset 4097, which is > outside the array). The name ends precisely at offset 4095 with its NUL character: {PATH_MAX} Maximum number of bytes in a pathname, including the terminating null character. [ http://www.opengroup.org/onlinepubs/009695399/basedefs/limits.h.html ] So, if I'm not mistaken, '/' will be written at offset 4095 instead of the NUL and the NUL will be written at 4096. Everything's fine, right? -- Petr "Pasky" Baudis Stuff: http://pasky.or.cz/ C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor ``` ## Ingo Molnar, 2005-04-18 08:28 Subject: Re: [patch] git: fix 1-byte overflow in show-files.c Message-ID: <20050418082827.GA21612@elte.hu> URL: https://gitlist.dev/e/20050418082827.GA21612%40elte.hu In-Reply-To: <20050417235920.GY1461@pasky.ji.cz> ``` * Petr Baudis wrote: > > will attempt to append a "/" string to the directory name - resulting in > > a 1-byte overflow (a zero byte is written to offset 4097, which is > > outside the array). > > The name ends precisely at offset 4095 with its NUL character: > > {PATH_MAX} > Maximum number of bytes in a pathname, including the terminating > null character. > [ http://www.opengroup.org/onlinepubs/009695399/basedefs/limits.h.html ] > > So, if I'm not mistaken, '/' will be written at offset 4095 instead of > the NUL and the NUL will be written at 4096. Everything's fine, right? yeah, you are right - ignore this patch. Ingo ```