Thread (1 message) 1 message, 1 author, 2016-06-15

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:50:14
Subsystem: the rest · Maintainer: Linus Torvalds

Possibly related (same subject, not in this thread)

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

Double free can happen when commit->buffer == NULL in the first
iteration, then != NULL in the next two iterations.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 sha1_name.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Thanks.  First the later hunk:
quoted hunk
diff --git a/sha1_name.c b/sha1_name.c
index 2c3a5fb..13ee6f5 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -740,6 +740,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
 	free_commit_list(list);
 	for (l = backup; l; l = l->next)
 		clear_commit_marks(l->item, ONELINE_SEEN);
+	free_commit_list(backup);
 	return retval;
 }
This is necessary, but is unrelated to the topic, no?
quoted hunk
@@ -718,13 +718,13 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
 		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;
+			free(temp_commit_buffer);
 			temp_commit_buffer = p;
 		}
 		if (!(p = strstr(p, "\n\n")))
This looks very convoluted.

I think the "temp-commit-buffer with a lifetime one iteration more than
the loop body itself" is merely a misguided attempt to avoid sprinkling
many free() calls inside the loop that has irregular exit points with
continue and break.

If you rewrite the loop to have more regular structure, there is no reason
to have such a temporary variable with tricky lifespan.

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

 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 {
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help