[patch] git: fix memory leak in checkout-cache.c

[patch] git: fix memory leak in checkout-cache.c

[Ingo Molnar]

this patch fixes a memory leak in checkout-cache.

	Ingo

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- 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)

[patch] git: fix memory leak #2 in checkout-cache.c

[Ingo Molnar]

this patch fixes another (very rare) memory leak in checkout-cache.

	Ingo

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- 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));
 	}

[patch] git: cleanup in ls-tree.c

[Ingo Molnar]

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 <mingo@elte.hu>

--- 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;
 }
 

[patch] git: fix memory leaks in read-tree.c

[Ingo Molnar]

this patch fixes one common, and 4 rare memory leaks in read-tree.c.

	Ingo

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- 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;
 }
 

[patch] git: fix rare memory leak in rev-tree.c

[Ingo Molnar]

this patch fixes a rare memory leak in rev-tree.c.

	Ingo

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- 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);

[patch] git: report parse_commit() errors in rev-tree.c

[Ingo Molnar]

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 <mingo@elte.hu>

--- 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 <cache-file>] <commit-id> [<commit-id>]");
-		parse_commit(sha1[nr]);
+		if (parse_commit(sha1[nr]))
+			fprintf(stderr, "warning: rev-tree: bad commit!\n");
 		nr++;
 	}
 

[patch] git: fix memory leak in show-diff.c

[Ingo Molnar]

this patch fixes a memory leak in show-diff.c.

	Ingo

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- 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)

[patch] git: fix overflow in update-cache.c

[Ingo Molnar]

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 <mingo@elte.hu>

--- 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) {

[patch] cleanup: read_sha1_file() -> malloc_read_sha1_file()

[Ingo Molnar]

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 <mingo@elte.hu>

--- 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] <sha1>");
-	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")) {

[patch] git: fix memory leaks in read-cache.c

[Ingo Molnar]

this patch fixes a common and a rare memory leak in read-cache.c.

	Ingo

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- 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;

[patch] git: fix memory leak #2 in read-cache.c

[Ingo Molnar]

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 <mingo@elte.hu>

--- 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;
 	}

[patch] git: fix 1-byte overflow in show-files.c

[Ingo Molnar]

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 <mingo@elte.hu>

--- 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) {

[patch] git: fix memory leaks in update-cache.c

[Ingo Molnar]

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 <mingo@elte.hu>

--- 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;
 }
 
 /*

[patch] git: clean up add_file_to_cache() in update-cache.c

[Ingo Molnar]

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 <mingo@elte.hu>

--- 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)

[patch] git: fix memory leak in write-tree.c

[Ingo Molnar]

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 <mingo@elte.hu>

--- 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;
 }
 

Re: [patch] git: fix memory leak #2 in read-cache.c

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> 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 <mingo@elte.hu>

--- 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:

[patch] git: fix memory leak #3 in update-cache.c

[Ingo Molnar]

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 <mingo@elte.hu>

--- 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;
 }

Re: [patch] git: fix memory leak #2 in read-cache.c

[Martin Schlemmer]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

On Thu, 2005-04-14 at 15:25 +0200, Ingo Molnar wrote:
> * Ingo Molnar <mingo@elte.hu> 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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch] git: fix memory leak #2 in read-cache.c

[Linus Torvalds]

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

Re: [patch] git: fix memory leak #2 in read-cache.c

[Ingo Molnar]

* Linus Torvalds <torvalds@osdl.org> 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

Re: [patch] git: fix 1-byte overflow in show-files.c

[Petr Baudis]

Dear diary, on Thu, Apr 14, 2005 at 02:53:54PM CEST, I got a letter
where Ingo Molnar <mingo@elte.hu> 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

Re: [patch] git: fix 1-byte overflow in show-files.c

[Ingo Molnar]

* Petr Baudis <pasky@ucw.cz> 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