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:12 UTC
Message-ID: <7v1v5m6w26.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1292209275-17451-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 7 quoted lines

> 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 <pclouds@gmail.com>
> ---
>  sha1_name.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

Thanks.  First the later hunk:

Show 11 quoted lines

> 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?

Show 15 quoted lines

> @@ -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 {