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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 23, 2010, 22:53 UTC
Message-ID: <7vd3zv7ee3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <36ca99e91002231356u189b80ebka7ae8caf89ab9c10@mail.gmail.com>

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

Show 19 quoted lines

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