Re: [patch] git: fix memory leak #2 in read-cache.c
* Linus Torvalds <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] git: fix memory leak #2 in read-cache.c
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 - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] git: fix memory leak #2 in read-cache.c
On Thu, 2005-04-14 at 15:25 +0200, Ingo Molnar wrote: > * Ingo Molnar <[EMAIL PROTECTED]> 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 signature.asc Description: This is a digitally signed message part
Re: [patch] git: fix memory leak #2 in read-cache.c
* Ingo Molnar <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
--- 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:
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] git: fix memory leak #2 in read-cache.c
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 <[EMAIL PROTECTED]>
--- 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;
}
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
