[PATCH 0/6] Memory leaks once again

[PATCH 0/6] Memory leaks once again

[Stefan Beller]

Here comes another bunch of memory leaks fixed.
patches 1-4 are safe bets, but 5 and 6 are not so.

In patch 5 I wonder if we need to fix more aggressively and
in patch 6 I just know there is a leak but I have no idea how to
actually fix it.

Stefan Beller (6):
  shallow: fix a memleak
  line-log.c: fix a memleak
  line-log.c: fix a memleak
  wt-status.c: fix a memleak
  pack-bitmap: fix a memleak
  WIP/RFC/entry.c: fix a memleak

 entry.c       |  4 +++-
 line-log.c    |  4 ++++
 pack-bitmap.c | 27 ++++++++++++++++++---------
 shallow.c     |  4 ++--
 wt-status.c   |  2 ++
 5 files changed, 29 insertions(+), 12 deletions(-)

-- 
2.3.0.81.gc37f363

[PATCH 1/6] pack-bitmap: fix a memleak

[Stefan Beller]

`recent_bitmaps` is allocated in the function load_bitmap_entries_v1
and it is not passed into any function, so it's safe to free it before
leaving that function.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    I wonder however if we need to free the actual bitmaps
    stored in the recent_bitmaps as well.

 pack-bitmap.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 365f9d9..34823e9 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -213,7 +213,7 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
 {
 	static const size_t MAX_XOR_OFFSET = 160;
 
-	uint32_t i;
+	uint32_t i, ret = 0;
 	struct stored_bitmap **recent_bitmaps;
 
 	recent_bitmaps = xcalloc(MAX_XOR_OFFSET, sizeof(struct stored_bitmap));
@@ -232,24 +232,31 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
 		sha1 = nth_packed_object_sha1(index->pack, commit_idx_pos);
 
 		bitmap = read_bitmap_1(index);
-		if (!bitmap)
-			return -1;
+		if (!bitmap) {
+			ret = -1;
+			goto out;
+		}
 
-		if (xor_offset > MAX_XOR_OFFSET || xor_offset > i)
-			return error("Corrupted bitmap pack index");
+		if (xor_offset > MAX_XOR_OFFSET || xor_offset > i) {
+			ret = error("Corrupted bitmap pack index");
+			goto out;
+		}
 
 		if (xor_offset > 0) {
 			xor_bitmap = recent_bitmaps[(i - xor_offset) % MAX_XOR_OFFSET];
 
-			if (xor_bitmap == NULL)
-				return error("Invalid XOR offset in bitmap pack index");
+			if (xor_bitmap == NULL) {
+				ret = error("Invalid XOR offset in bitmap pack index");
+				goto out;
+			}
 		}
 
 		recent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(
 			index, bitmap, sha1, xor_bitmap, flags);
 	}
-
-	return 0;
+out:
+	free(recent_bitmaps);
+	return ret;
 }
 
 static char *pack_bitmap_filename(struct packed_git *p)
@@ -986,6 +993,8 @@ void test_bitmap_walk(struct rev_info *revs)
 		fprintf(stderr, "OK!\n");
 	else
 		fprintf(stderr, "Mismatch!\n");
+
+	free(result);
 }
 
 static int rebuild_bitmap(uint32_t *reposition,
-- 
2.3.0.81.gc37f363

[PATCH 1/6] shallow: fix a memleak

[Stefan Beller]

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

diff --git a/shallow.c b/shallow.c
index d8bf40a..f8b0458 100644
--- a/shallow.c
+++ b/shallow.c
@@ -416,7 +416,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
 	uint32_t *bitmap = paint_alloc(info);
 	struct commit *c = lookup_commit_reference_gently(sha1, 1);
 	if (!c)
-		return;
+		goto out;
 	memset(bitmap, 0, bitmap_size);
 	bitmap[id / 32] |= (1 << (id % 32));
 	commit_list_insert(c, &head);
@@ -471,7 +471,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
 		if (o && o->type == OBJ_COMMIT)
 			o->flags &= ~SEEN;
 	}
-
+out:
 	free(tmp);
 }
 
-- 
2.3.0.81.gc37f363

[PATCH 2/6] line-log.c: fix a memleak

[Stefan Beller]

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

diff --git a/line-log.c b/line-log.c
index a490efe..b43ac58 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1099,6 +1099,7 @@ static int process_all_files(struct line_log_data **range_out,
 			rg->pair = diff_filepair_dup(queue->queue[i]);
 			memcpy(&rg->diff, pairdiff, sizeof(struct diff_ranges));
 		}
+		free(pairdiff);
 	}
 
 	return changed;
-- 
2.3.0.81.gc37f363

[PATCH 2/6] shallow: fix a memleak

[Stefan Beller]

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

diff --git a/shallow.c b/shallow.c
index d8bf40a..f8b0458 100644
--- a/shallow.c
+++ b/shallow.c
@@ -416,7 +416,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
 	uint32_t *bitmap = paint_alloc(info);
 	struct commit *c = lookup_commit_reference_gently(sha1, 1);
 	if (!c)
-		return;
+		goto out;
 	memset(bitmap, 0, bitmap_size);
 	bitmap[id / 32] |= (1 << (id % 32));
 	commit_list_insert(c, &head);
@@ -471,7 +471,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
 		if (o && o->type == OBJ_COMMIT)
 			o->flags &= ~SEEN;
 	}
-
+out:
 	free(tmp);
 }
 
-- 
2.3.0.81.gc37f363

[PATCH 3/6] line-log.c: fix a memleak

[Stefan Beller]

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

diff --git a/line-log.c b/line-log.c
index b43ac58..db6e58d 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1129,6 +1129,9 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c
 	changed = process_all_files(&parent_range, rev, &queue, range);
 	if (parent)
 		add_line_range(rev, parent, parent_range);
+	else
+		free_line_log_data(parent_range);
+
 	return changed;
 }
 
-- 
2.3.0.81.gc37f363

[PATCH 4/6] line-log.c: fix a memleak

[Stefan Beller]

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

diff --git a/line-log.c b/line-log.c
index b43ac58..db6e58d 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1129,6 +1129,9 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c
 	changed = process_all_files(&parent_range, rev, &queue, range);
 	if (parent)
 		add_line_range(rev, parent, parent_range);
+	else
+		free_line_log_data(parent_range);
+
 	return changed;
 }
 
-- 
2.3.0.81.gc37f363

[PATCH 4/6] wt-status.c: fix a memleak

[Stefan Beller]

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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/wt-status.c b/wt-status.c
index 853419f..9d2a349 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1576,6 +1576,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 
 	color_fprintf(s->fp, header_color, "]");
 	fputc(s->null_termination ? '\0' : '\n', s->fp);
+
+	free((char *)base);
 }
 
 void wt_shortstatus_print(struct wt_status *s)
-- 
2.3.0.81.gc37f363

[PATCH 5/6] pack-bitmap: fix a memleak

[Stefan Beller]

`recent_bitmaps` is allocated in the function load_bitmap_entries_v1
and it is not passed into any function, so it's safe to free it before
leaving that function.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    I wonder however if we need to free the actual bitmaps
    stored in the recent_bitmaps as well.

 pack-bitmap.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 365f9d9..34823e9 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -213,7 +213,7 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
 {
 	static const size_t MAX_XOR_OFFSET = 160;
 
-	uint32_t i;
+	uint32_t i, ret = 0;
 	struct stored_bitmap **recent_bitmaps;
 
 	recent_bitmaps = xcalloc(MAX_XOR_OFFSET, sizeof(struct stored_bitmap));
@@ -232,24 +232,31 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
 		sha1 = nth_packed_object_sha1(index->pack, commit_idx_pos);
 
 		bitmap = read_bitmap_1(index);
-		if (!bitmap)
-			return -1;
+		if (!bitmap) {
+			ret = -1;
+			goto out;
+		}
 
-		if (xor_offset > MAX_XOR_OFFSET || xor_offset > i)
-			return error("Corrupted bitmap pack index");
+		if (xor_offset > MAX_XOR_OFFSET || xor_offset > i) {
+			ret = error("Corrupted bitmap pack index");
+			goto out;
+		}
 
 		if (xor_offset > 0) {
 			xor_bitmap = recent_bitmaps[(i - xor_offset) % MAX_XOR_OFFSET];
 
-			if (xor_bitmap == NULL)
-				return error("Invalid XOR offset in bitmap pack index");
+			if (xor_bitmap == NULL) {
+				ret = error("Invalid XOR offset in bitmap pack index");
+				goto out;
+			}
 		}
 
 		recent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(
 			index, bitmap, sha1, xor_bitmap, flags);
 	}
-
-	return 0;
+out:
+	free(recent_bitmaps);
+	return ret;
 }
 
 static char *pack_bitmap_filename(struct packed_git *p)
@@ -986,6 +993,8 @@ void test_bitmap_walk(struct rev_info *revs)
 		fprintf(stderr, "OK!\n");
 	else
 		fprintf(stderr, "Mismatch!\n");
+
+	free(result);
 }
 
 static int rebuild_bitmap(uint32_t *reposition,
-- 
2.3.0.81.gc37f363

[PATCH 5/6] wt-status.c: fix a memleak

[Stefan Beller]

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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/wt-status.c b/wt-status.c
index 853419f..9d2a349 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1576,6 +1576,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 
 	color_fprintf(s->fp, header_color, "]");
 	fputc(s->null_termination ? '\0' : '\n', s->fp);
+
+	free((char *)base);
 }
 
 void wt_shortstatus_print(struct wt_status *s)
-- 
2.3.0.81.gc37f363

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

[Stefan Beller]

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 <sbeller@google.com>
---
 entry.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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);
 			goto finish;
+		}
 	}
 
 	switch (ce_mode_s_ifmt) {
-- 
2.3.0.81.gc37f363

Re: [PATCH 0/6] Memory leaks once again

[Stefan Beller]

On Fri, Mar 27, 2015 at 3:09 PM, Stefan Beller <sbeller@google.com> wrote:
> Here comes another bunch of memory leaks fixed.
> patches 1-4 are safe bets, but 5 and 6 are not so.
>
> In patch 5 I wonder if we need to fix more aggressively and
> in patch 6 I just know there is a leak but I have no idea how to
> actually fix it.
>

And I sent 12 instead of 6 patches because I needed to fine tune the
order of the patch set and I forgot to delete the un-tuned version.