[PATCH] rerere: fix memory leak if rerere images can't be read

[PATCH] rerere: fix memory leak if rerere images can't be read

[Bert Wesarg]

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
 rerere.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/rerere.c b/rerere.c
index d1d3e75..9ca4cb8 100644
--- a/rerere.c
+++ b/rerere.c
@@ -364,16 +364,17 @@ static int find_conflict(struct string_list *conflict)
 static int merge(const char *name, const char *path)
 {
 	int ret;
-	mmfile_t cur, base, other;
+	mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};
 	mmbuffer_t result = {NULL, 0};
 
 	if (handle_file(path, NULL, rerere_path(name, "thisimage")) < 0)
 		return 1;
 
+	ret = 1;
 	if (read_mmfile(&cur, rerere_path(name, "thisimage")) ||
 			read_mmfile(&base, rerere_path(name, "preimage")) ||
 			read_mmfile(&other, rerere_path(name, "postimage")))
-		return 1;
+		goto out;
 	ret = ll_merge(&result, path, &base, &cur, "", &other, "", 0);
 	if (!ret) {
 		FILE *f = fopen(path, "w");
@@ -387,6 +388,7 @@ static int merge(const char *name, const char *path)
 				     strerror(errno));
 	}
 
+out:
 	free(cur.ptr);
 	free(base.ptr);
 	free(other.ptr);
-- 
1.7.0.500.ge51cb

Re: [PATCH] rerere: fix memory leak if rerere images can't be read

[Johannes Schindelin]

Hi,

On Tue, 23 Feb 2010, Bert Wesarg wrote:

> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> ---
>  rerere.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)

Looks good to me, except...

> diff --git a/rerere.c b/rerere.c
> index d1d3e75..9ca4cb8 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -364,16 +364,17 @@ static int find_conflict(struct string_list *conflict)
>  static int merge(const char *name, const char *path)
>  {
>  	int ret;
> -	mmfile_t cur, base, other;
> +	mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};
>  	mmbuffer_t result = {NULL, 0};
>  
>  	if (handle_file(path, NULL, rerere_path(name, "thisimage")) < 0)
>  		return 1;
>  
> +	ret = 1;

This initialization can come earlier, at declaration time.

>  	if (read_mmfile(&cur, rerere_path(name, "thisimage")) ||
>  			read_mmfile(&base, rerere_path(name, "preimage")) ||
>  			read_mmfile(&other, rerere_path(name, "postimage")))
> -		return 1;
> +		goto out;
>  	ret = ll_merge(&result, path, &base, &cur, "", &other, "", 0);
>  	if (!ret) {
>  		FILE *f = fopen(path, "w");

Ciao,
Dscho

Re: [PATCH] rerere: fix memory leak if rerere images can't be read

[Bert Wesarg]

On Tue, Feb 23, 2010 at 22:26, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 23 Feb 2010, Bert Wesarg wrote:
>
>> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>> ---
>>  rerere.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> Looks good to me, except...
>
>> diff --git a/rerere.c b/rerere.c
>> index d1d3e75..9ca4cb8 100644
>> --- a/rerere.c
>> +++ b/rerere.c
>> @@ -364,16 +364,17 @@ static int find_conflict(struct string_list *conflict)
>>  static int merge(const char *name, const char *path)
>>  {
>>       int ret;
>> -     mmfile_t cur, base, other;
>> +     mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};
>>       mmbuffer_t result = {NULL, 0};
>>
>>       if (handle_file(path, NULL, rerere_path(name, "thisimage")) < 0)
>>               return 1;
>>
>> +     ret = 1;
>
> This initialization can come earlier, at declaration time.
I thought about it, but I think it is clearer to put just in front of
the condition which may fail.

Bye,
Bert

Re: [PATCH] rerere: fix memory leak if rerere images can't be read

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1338 bytes --]

Hi,

On Tue, 23 Feb 2010, Bert Wesarg wrote:

> On Tue, Feb 23, 2010 at 22:26, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Tue, 23 Feb 2010, Bert Wesarg wrote:
> >
> >> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> >> ---
> >>  rerere.c |    6 ++++--
> >>  1 files changed, 4 insertions(+), 2 deletions(-)
> >
> > Looks good to me, except...
> >
> >> diff --git a/rerere.c b/rerere.c
> >> index d1d3e75..9ca4cb8 100644
> >> --- a/rerere.c
> >> +++ b/rerere.c
> >> @@ -364,16 +364,17 @@ static int find_conflict(struct string_list *conflict)
> >>  static int merge(const char *name, const char *path)
> >>  {
> >>       int ret;
> >> -     mmfile_t cur, base, other;
> >> +     mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};
> >>       mmbuffer_t result = {NULL, 0};
> >>
> >>       if (handle_file(path, NULL, rerere_path(name, "thisimage")) < 0)
> >>               return 1;
> >>
> >> +     ret = 1;
> >
> > This initialization can come earlier, at declaration time.
> I thought about it, but I think it is clearer to put just in front of
> the condition which may fail.

Well, to _this_ developer, it is clearer when a variable has been 
initialized in any case. No need to think about how it could be used 
uninitialized. But if you insist...

Ciao,
Dscho

Re: [PATCH] rerere: fix memory leak if rerere images can't be read

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> On Tue, Feb 23, 2010 at 22:26, Johannes Schindelin
>>> diff --git a/rerere.c b/rerere.c
>>> index d1d3e75..9ca4cb8 100644
>>> --- a/rerere.c
>>> +++ b/rerere.c
>>> @@ -364,16 +364,17 @@ static int find_conflict(struct string_list *conflict)
>>>  static int merge(const char *name, const char *path)
>>>  {
>>>       int ret;
>>> -     mmfile_t cur, base, other;
>>> +     mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};
>>>       mmbuffer_t result = {NULL, 0};
>>>
>>>       if (handle_file(path, NULL, rerere_path(name, "thisimage")) < 0)
>>>               return 1;
>>>
>>> +     ret = 1;
>>
>> This initialization can come earlier, at declaration time.

Yes it can, but I do not think it makes it any easier to read.

> I thought about it, but I think it is clearer to put just in front of
> the condition which may fail.

And I do not think yours is much easier to follow, either.

The function looks like:

	static int merge()
        {
        	int ret; /* uninitialized */
                ...

		if (something)
                	return 1; /* error */

		if (some thing
                    that does allocation and is
                    fairly large)
			return 1; /* error */
		ret = operation_that_return_status();
                free resources
                return ret;
	}

If you initialize ret early, it might make "ret" always defined, but it
also makes the first "return 1" give you "huh, why not use ret?".

	static int merge()
        {
        	int ret = 1; /* assume bad things will happen */
                ...

		if (something)
                	return 1; /* error */

		if (some thing
                    that does allocation and is
                    fairly large)
			goto out; /* error */
		ret = operation_that_return_status();
	out:
                free resources
                return ret;
	}

Also it makes clearing of assumed error harder to spot.

Bert's version is not much better.  That "set ret to positive before going
to the exit codepath" logically belongs to the error case.  IOW:

	static int merge()
        {
        	int ret; /* uninitialized */
                ...

		if (something)
                	return 1; /* error */

		if (some thing
                    that does allocation and is
                    fairly large) {
			ret = 1;
			goto out; /* error */
		}
		ret = operation_that_return_status();
	out:
                free resources
                return ret;
	}

We could initialize ret to 0 at the beginning, to signal the people who
might be tempted to touch the code later that you are supposed to flip it
to non-zero when you find an error and jump to out.  An immediate follow
up to such a change would be to do something like:

	static int merge()
        {
        	int ret = 0; /* no error yet */
                ...

		if (something) {
                	ret = 1;
                        goto out; /* error */
		}

		if (some thing
                    that does allocation and is
                    fairly large) {
			ret = 1;
			goto out; /* error */
		}
		ret = operation_that_return_status();
	out:
                free resources
                return ret;
	}

but the first condition hasn't even allocated anything to be freed, so
there isn't much point doing this either.