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

Bert Wesarg [off-list ref] writes:

On Tue, Feb 23, 2010 at 22:26, Johannes Schindelin

quoted

quoted

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.