[PATCH v2 4/4] reflog-walk: include all fields when freeing complete_reflogs

From: Jeff King <peff@peff.net>
Date: Jul 7, 2017, 08:43 UTC
Message-ID: <20170707084316.jetdonpj75ricara@sigill.intra.peff.net>
In-Reply-To: <20170707083636.kjsr5ry3237paeiv@sigill.intra.peff.net>

When we encounter an error adding reflogs for a walk, we try to free any logs we have read. But we didn't free all fields, meaning that we could in theory leak all of the "items" array (which would consitute the bulk of the allocated memory).

This patch adds a helper which frees all of the entries and uses it as appropriate.

As it turns out, the leak seems impossible to trigger with the current code. Of the three error paths that free the complete_reflogs struct, two only kick in when the items array is empty, and the third was removed entirely in the previous commit.

So this patch should be a noop in terms of behavior, but it fixes a potential maintenance headache should anybody add a new error path and copy the partial-free code. Which is what happened in 5026b47175 (add_reflog_for_walk: avoid memory leak, 2017-05-04), though its leaky call was the third one that was recently removed.

Signed-off-by: Jeff King <peff@peff.net>
---
 reflog-walk.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index 2a43537326..ba72020fc3 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -38,6 +38,22 @@ static int read_one_reflog(struct object_id *ooid, struct object_id *noid,
 	return 0;
 }
 
+static void free_complete_reflog(struct complete_reflogs *array)
+{
+	int i;
+
+	if (!array)
+		return;
+
+	for (i = 0; i < array->nr; i++) {
+		free(array->items[i].email);
+		free(array->items[i].message);
+	}
+	free(array->items);
+	free(array->ref);
+	free(array);
+}
+
 static struct complete_reflogs *read_complete_reflog(const char *ref)
 {
 	struct complete_reflogs *reflogs =
@@ -189,20 +205,14 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 			if (ret > 1)
 				free(b);
 			else if (ret == 1) {
-				if (reflogs) {
-					free(reflogs->ref);
-					free(reflogs);
-				}
+				free_complete_reflog(reflogs);
 				free(branch);
 				branch = b;
 				reflogs = read_complete_reflog(branch);
 			}
 		}
 		if (!reflogs || reflogs->nr == 0) {
-			if (reflogs) {
-				free(reflogs->ref);
-				free(reflogs);
-			}
+			free_complete_reflog(reflogs);
 			free(branch);
 			return -1;
 		}

-- 
2.13.2.1000.g8590c1af5d