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

[Junio C Hamano <gitster@xxxxxxxxx>]

Nguyán ThÃi Ngác Duy  <pclouds@xxxxxxxxx> 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 <pclouds@xxxxxxxxx>
> ---
>  sha1_name.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

Thanks.  First the later 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?

> @@ -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 {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html