[PATCHv3 6/6] entry.c: fix a memleak

I changed the patches based on Erics feedback, all of them look sane to me now.

I still retain 2 patches which I need to convince myself are reasonable to send to the list though.

John Keeping (1):
  entry.c: fix a memleak

Stefan Beller (5):
  shallow: fix a memleak
  line-log.c: fix a memleak
  line-log.c: fix a memleak
  wt-status.c: fix a memleak
  pack-bitmap.c: fix a memleak

 line-log.c    | 4 ++++
 pack-bitmap.c | 2 ++
 shallow.c     | 5 +++--
 streaming.c   | 5 ++++-
 wt-status.c   | 1 +
 5 files changed, 14 insertions(+), 3 deletions(-)

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 shallow.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

On Mon, Mar 30, 2015 at 9:22 PM, Stefan Beller <sbeller@google.com> wrote:

You've made two unrelated changes in this patch, only one of which is a fix for the memory leak mentioned by the commit message.

The 'tmp' change fixes the memory leak. The 'bitmap' change, however, merely avoids doing work unnecessarily in the "early return" case. Since 'bitmap' gets released by the caller of paint_down() when the paint_info structure is freed, it is misleading to claim that that particular change is a "memleak" fix.

Either split the two changes into separate patches or augment the commit message to say something along the lines of:

    While here, also avoid unnecessarily allocating 'bitmap' within
    paint_info in the early-return case.

The `filepair` is assigned new memory with any iteration via process_diff_filepair, so free it before the current iteration ends.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 line-log.c | 1 +
 1 file changed, 1 insertion(+)

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 line-log.c | 3 +++
 1 file changed, 3 insertions(+)

On 03/31/2015 03:22 AM, Stefan Beller wrote:

Would it make sense to join 2/6 and 3/6 into a single patch ?

Both fix the memleaks in the same file, and the header can be "fix memleaks".

Stefan Beller <sbeller@google.com> writes:

Should this be in the else clause? From a cursory scan add-line-range feeds it to either line-log-data-merge or line-log-data-copy to create "new" that is a separate thing from the parent_range we are feeding it, and that copy is used by add-decoration. If add-line-range does not take ownership of parent_range from us, shouldn't we be freeing it whether we called it or not???

On 30.03.2015 22:06, Junio C Hamano wrote:

In my understanding it does take ownership of it. (According to my code review on friday)

Stefan Beller <stefanbeller@gmail.com> writes:

Hmmm.

add_line_range() is given the "parent_range" as its third parameter, i.e. "range".

If there is nothing for the commit yet, it calls line_log_data_copy() on it to make a deep copy and stores that in new, which is then used as the decoration for the commit---the copy is what is retained.

I am not quite sure what the lifetime rule is for the original of this deep copy, i.e. "range", aka "parent_range" of the caller. Who holds pointers into it after this function is done?

In any code path of shorten_unambiguous_ref the return value is a xstrdup(some string), so it is safe to free the variable `base` in any codepath.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 wt-status.c | 1 +
 1 file changed, 1 insertion(+)

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 pack-bitmap.c | 2 ++
 1 file changed, 2 insertions(+)

On Mon, Mar 30, 2015 at 06:22:10PM -0700, Stefan Beller wrote:

I think this one is fine. I see you dropped the other bits from load_bitmap_entries_v1. There are definitely some leaks there in the error case, and I left some notes in the other thread.

I don't think those leaks are all that critical (we only load the bitmaps one per process, and the memory persists if we succeed, so the worst case is that we spend the memory but do not get to use the bitmaps). But if your goal is making us valgrind-clean, they would certainly count.

-Peff

From: John Keeping <john@keeping.me.uk>

stream_blob_to_fd() always frees the filter now, so there is no memory leak in entry.c:152 just before the `goto finish`.

Signed-off-by: John Keeping <john@keeping.me.uk>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 
 I added Johns signoff here tentatively, John can you confirm the sign off?
 
 streaming.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Stefan Beller <sbeller@google.com> writes:

I think this one is fine. I see you moved the callsite to free this thing from entry.c down to streaming (stream_blob_to_fd()), which means you would need to adjust the title of the change, too.

It somewhat feels dirty that we free what the caller gave us, but given that closing a filtered stream frees the filter at the end, I think this is the right place to discard the filter. In essence, the overall use of the API in this code is equivalent to

	filter = ... prepare a filter ...
        if (open_istream(... filter) == OK) {
		... use filter ...
                close_istream(); /* which frees the filter */
	} else {
           	/* we failed to free filter here without the patch */
		free_stream_filter(filter);
	}

so the patch makes sense.

We may want to teach free_stream_filter() that a request to free a NULL filter is OK, though. That would be a separate clean-up topic.

On Mon, Mar 30, 2015 at 06:22:11PM -0700, Stefan Beller wrote:

Signed-off-by: John Keeping <john@keeping.me.uk>

John Keeping <john@keeping.me.uk> writes:

Thanks. I'll queue with a retitle, as this no longer is about entry.c at all.