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

From: Ingo Molnar <mingo@elte.hu>
Date: Apr 14, 2005, 13:25 UTC
Message-ID: <20050414132550.GA25496@elte.hu>
In-Reply-To: <20050414123934.GA15420@elte.hu>

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