Re: [PATCH V2 6/6] WIP/RFC/entry.c: fix a memleak

On Fri, Mar 27, 2015 at 08:14:28PM -0400, Eric Sunshine wrote:

On Friday, March 27, 2015, Eric Sunshine [off-list ref] wrote:

quoted

On Fri, Mar 27, 2015 at 6:32 PM, Stefan Beller [off-list ref] wrote:

quoted

I  observe that filter is going out of scope, but the
implementation proposed in this patch produces just a
crash instead of any helpful fix.

Signed-off-by: Stefan Beller <redacted>
---

diff --git a/entry.c b/entry.c
index 1eda8e9..5383001 100644
--- a/entry.c
+++ b/entry.c

@@ -152,8 +152,10 @@ static int write_entry(struct cache_entry *ce,
                if (filter &&
                    !streaming_write_entry(ce, path, filter,
                                           state, to_tempfile,
-                                          &fstat_done, &st))
+                                          &fstat_done, &st)) {
+                       free_stream_filter(filter);

Aside from the crash you are seeing, this is a bogus fix anyway.
You're only freeing 'filter' if it was allocated _and_ if
streaming_write_entry() returned 0. I would guess your intention was
to free 'filter' regardless of the result of streaming_write_entry().

Unless streaming_write_entry() is freeing the filter for you -- there
is a free_stream_filter() call in close_method_decl() in streaming.c
-- in which case your new free_stream_filter() call would attempt to
free the already-freed filter.

Yes, I think the correct fix for this leak is to make
stream_blob_to_fd() always free the filter, since there's only one path
out that doesn't at the moment and there's no way for the caller to
figure out whether or not the filter has been freed:

-- >8 --

diff --git a/streaming.c b/streaming.c
index 2ff036a..811fcc2 100644
--- a/streaming.c
+++ b/streaming.c

@@ -507,8 +507,11 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f
 	int result = -1;
 
 	st = open_istream(sha1, &type, &sz, filter);
-	if (!st)
+	if (!st) {
+		if (filter)
+			free_stream_filter(filter);
 		return result;
+	}
 	if (type != OBJ_BLOB)
 		goto close_and_exit;
 	for (;;) {