Re: [PATCH 0/3] line-log: plug some memory leaks

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 3, 2022, 09:05 UTC
Message-ID: <221103.864jvg2yit.gmgdl@evledraar.gmail.com>
In-Reply-To: <20221102220142.574890-1-szeder.dev@gmail.com>

On Wed, Nov 02 2022, SZEDER Gábor wrote:

> The first patch plugs the reported big memory leak, the second one
> plugs a minor leak, and the little cleanup in the third puts the
> cherry on top.

Looks good as far as it goes.

The "further" part seems a real mess though, e.g. I came up with the below on top as a quick test, i.e. we have other existing users that also loop over the same struct, and free "queue[n]", but do so differently.

I handled the combine-diff.c one (rough WIP, only compiled it), but then diffcore-rename.c has another such case.

I wonder if you looked at that further, and if the free function we're adding now should anticipate that case or not.

And, orthagonally I came up with this rough WIP yesterday:
	
	@@ -6640,9 +6633,7 @@ static void diffcore_apply_filter(struct diff_options *options)
	 {
	 	int i;
	 	struct diff_queue_struct *q = &diff_queued_diff;
	-	struct diff_queue_struct outq;
	-
	-	DIFF_QUEUE_CLEAR(&outq);
	+	struct diff_queue_struct outq = DIFF_QUEUE_STRUCT_INIT;
	 
	 	if (!options->filter)
	 		return;
	@@ -6735,8 +6726,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)
	 {
	 	int i;
	 	struct diff_queue_struct *q = &diff_queued_diff;
	-	struct diff_queue_struct outq;
	-	DIFF_QUEUE_CLEAR(&outq);
	+	struct diff_queue_struct outq = DIFF_QUEUE_STRUCT_INIT;
	 
	 	for (i = 0; i < q->nr; i++) {
	 		struct diff_filepair *p = q->queue[i];
	diff --git a/diffcore.h b/diffcore.h
	index badc2261c20..a0a89568cec 100644
	--- a/diffcore.h
	+++ b/diffcore.h
	@@ -150,12 +150,7 @@ struct diff_queue_struct {
	 	int alloc;
	 	int nr;
	 };
	-
	-#define DIFF_QUEUE_CLEAR(q) \
	-	do { \
	-		(q)->queue = NULL; \
	-		(q)->nr = (q)->alloc = 0; \
	-	} while (0)
	+	#define DIFF_QUEUE_STRUCT_INIT { 0 }

You leave the DIFF_QUEUE_CLEAR in place, but I wonder given that that's the common pattern whether you shouldn't have a *_reset() and *_{free,release}() (one of the two, your current naming is fine) which resets it too, as some callers seen below & in your diff context want that.

diff --git a/combine-diff.c b/combine-diff.c
index b0ece954808..509c58ad556 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1297,12 +1297,6 @@ void show_combined_diff(struct combine_diff_path *p,
 		show_patch_diff(p, num_parent, 1, rev);
 }
 
-static void free_combined_pair(struct diff_filepair *pair)
-{
-	free(pair->two);
-	free(pair);
-}
-
 /*
  * A combine_diff_path expresses N parents on the LHS against 1 merge
  * result. Synthesize a diff_filepair that has N entries on the "one"
@@ -1355,9 +1349,7 @@ static void handle_combined_callback(struct diff_options *opt,
 	for (i = 0, p = paths; p; p = p->next)
 		q.queue[i++] = combined_pair(p, num_parent);
 	opt->format_callback(&q, opt, opt->format_callback_data);
-	for (i = 0; i < num_paths; i++)
-		free_combined_pair(q.queue[i]);
-	free(q.queue);
+	diff_free_queue(&q, 1);
 }
 
 static const char *path_path(void *obj)
diff --git a/diff.c b/diff.c
index 03e6ffb5e4e..b48105c070f 100644
--- a/diff.c
+++ b/diff.c
@@ -5773,10 +5773,20 @@ void diff_free_filepair(struct diff_filepair *p)
 	free(p);
 }
 
-void diff_free_queue(struct diff_queue_struct *q)
+static void free_combined_pair(struct diff_filepair *pair)
 {
-	for (int i = 0; i < q->nr; i++)
-		diff_free_filepair(q->queue[i]);
+	free(pair->two);
+	free(pair);
+}
+
+void diff_free_queue(struct diff_queue_struct *q, int combined)
+{
+	for (int i = 0; i < q->nr; i++) {
+		if (combined)
+			diff_free_filepair(q->queue[i]);
+		else
+			free_combined_pair(q->queue[i]);
+	}
 	free(q->queue);
 }
 
@@ -6339,7 +6349,7 @@ int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int
 	struct diff_queue_struct *q = &diff_queued_diff;
 	int result = diff_get_patch_id(options, oid, diff_header_only);
 
-	diff_free_queue(q);
+	diff_free_queue(q, 0);
 	DIFF_QUEUE_CLEAR(q);
 
 	return result;
@@ -6609,7 +6619,7 @@ void diff_flush(struct diff_options *options)
 		options->format_callback(q, options, options->format_callback_data);
 
 free_queue:
-	diff_free_queue(q);
+	diff_free_queue(q, 0);
 	DIFF_QUEUE_CLEAR(q);
 	diff_free(options);
 
diff --git a/diffcore-rename.c b/diffcore-rename.c
index c0422d9e709..1754411a916 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -1686,6 +1686,7 @@ void diffcore_rename_extended(struct diff_options *options,
 			pair_to_free = p;
 
 		if (pair_to_free)
+			/* ??? */
 			pool_diff_free_filepair(pool, pair_to_free);
 	}
 	diff_debug_queue("done copying original", &outq);
diff --git a/diffcore.h b/diffcore.h
index 9b588a1ee15..6cb74e6eadf 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -162,7 +162,7 @@ struct diff_filepair *diff_queue(struct diff_queue_struct *,
 				 struct diff_filespec *,
 				 struct diff_filespec *);
 void diff_q(struct diff_queue_struct *, struct diff_filepair *);
-void diff_free_queue(struct diff_queue_struct *q);
+void diff_free_queue(struct diff_queue_struct *q, int combined);
 
 /* dir_rename_relevance: the reason we want rename information for a dir */
 enum dir_rename_relevance {
diff --git a/line-log.c b/line-log.c
index a7f3e7f6ce4..88c22b20c0f 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1090,7 +1090,7 @@ static struct diff_filepair *diff_filepair_dup(struct diff_filepair *pair)
 static void free_diffqueues(int n, struct diff_queue_struct *dq)
 {
 	for (int i = 0; i < n; i++)
-		diff_free_queue(&dq[i]);
+		diff_free_queue(&dq[i], 0);
 	free(dq);
 }
 
@@ -1193,7 +1193,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c
 	if (parent)
 		add_line_range(rev, parent, parent_range);
 	free_line_log_data(parent_range);
-	diff_free_queue(&queue);
+	diff_free_queue(&queue, 0);
 	return changed;
 }