Re: [PATCH 1/3] get_sha1_oneline: do not leak or double free

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 13, 2010, 06:19 UTC
Message-ID: <7vvd2y5h63.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7v1v5m6w26.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> I think the following is easier to read and conveys what the code is
> trying to do more clearly.  No?

This time with a proposed log message...

-- >8 --
Subject: [PATCH] get_sha1_oneline: fix lifespan rule of temp_commit_buffer variable

This is trying to free only what we ourselves read (as opposed to what we borrowed from commit->buffer) but do so lazily only to work around the fact that the code has many irregular exit points, and doing it right makes it necessary to call free() from many different places in the loop.

Rewrite the structure of the code inside the loop so that the variable has to live within a single iteration, ever. This should make the logic easier to follow as well.

Also we didn't free a temporary commit list we kept to hold the original set of commits. Free it.

Noticed-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1_name.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 2c3a5fb..2cc7a42 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -693,8 +693,7 @@ static int handle_one_ref(const char *path,
 static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
 {
 	struct commit_list *list = NULL, *backup = NULL, *l;
-	int retval = -1;
-	char *temp_commit_buffer = NULL;
+	int found = 0;
 	regex_t regex;
 
 	if (prefix[0] == '!') {
@@ -710,37 +709,40 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
 	for (l = list; l; l = l->next)
 		commit_list_insert(l->item, &backup);
 	while (list) {
-		char *p;
+		char *p, *to_free = NULL;
 		struct commit *commit;
 		enum object_type type;
 		unsigned long size;
+		int matches;
 
 		commit = pop_most_recent_commit(&list, ONELINE_SEEN);
 		if (!parse_object(commit->object.sha1))
 			continue;
-		free(temp_commit_buffer);
 		if (commit->buffer)
 			p = commit->buffer;
 		else {
 			p = read_sha1_file(commit->object.sha1, &type, &size);
 			if (!p)
 				continue;
-			temp_commit_buffer = p;
+			to_free = p;
 		}
-		if (!(p = strstr(p, "\n\n")))
-			continue;
-		if (!regexec(&regex, p + 2, 0, NULL, 0)) {
+
+		p = strstr(p, "\n\n");
+		matches = p && !regexec(&regex, p + 2, 0, NULL, 0);
+		free(to_free);
+
+		if (matches) {
 			hashcpy(sha1, commit->object.sha1);
-			retval = 0;
+			found = 1;
 			break;
 		}
 	}
 	regfree(&regex);
-	free(temp_commit_buffer);
 	free_commit_list(list);
 	for (l = backup; l; l = l->next)
 		clear_commit_marks(l->item, ONELINE_SEEN);
-	return retval;
+	free_commit_list(backup);
+	return found ? 0 : -1;
 }
 
 struct grab_nth_branch_switch_cbdata {

-- 
1.7.3.3.763.g91c7d