Re: [PATCH 1/1] cat-file: fix a memory leak in cat_one_file

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 27, 2014, 00:24 UTC
Message-ID: <20141127002417.GT6527@google.com>
In-Reply-To: <CANCZXo7CKz2bOM=6ehed-VLUGtyThuBbwtd_QaBMm2KKhEo=Bw@mail.gmail.com>

Hi,

Alexander Kuleshov wrote:

> [Subject: cat-file: fix a memory leak in cat_one_file]

Can you explain further? How did you run into this? Is it an unbounded leak or a small one-time leak? What is the benefit of applying this patch?

"By code inspection" is a fine answer to the question of how the problem was discovered. I'm just trying to understand the impact of the change.

Likewise, if the problem was discovered using valgrind, "in order to have cleaner valgrind output so other problems are easier to find" might be an okay answer to the question of what the benefit of the patch is. (Or it might not --- it depends on the maintainability impact and how much noise is being suppressed.)

[...]

Show 7 quoted lines

> +++ b/builtin/cat-file.c
> @@ -18,6 +18,7 @@ static int cat_one_file(int opt, const char
> *exp_type, const char *obj_name)
>      unsigned char sha1[20];
>      enum object_type type;
>      char *buf;
> +    int buf_must_free = 0;

The usual idiom is to initialize a pointer the current function is responsible for to NULL so it can call free(ptr) unconditionally before returning.

[...]

Show 7 quoted lines

> @@ -68,9 +69,14 @@ static int cat_one_file(int opt, const char
> *exp_type, const char *obj_name)
> 
>          if (type == OBJ_BLOB)
>              return stream_blob_to_fd(1, sha1, NULL, 0);
> +
>          buf = read_sha1_file(sha1, &type, &size);

Unrelated change?

Show 6 quoted lines

> +        buf_must_free = 1;
> +
> -        if (!buf)
> +        if (!buf) {
> +            free(buf);
>              die("Cannot read object %s", obj_name);

Is this free() needed? The program die()s right afterward, which would free all memory automatically as part of exiting. Tools like valgrind would understand that nothing is wrong since buf is still reachable through the stack.

[...]

Show 10 quoted lines

> @@ -110,6 +117,10 @@ static int cat_one_file(int opt, const char
> *exp_type, const char *obj_name)
>          die("git cat-file %s: bad file", obj_name);
> 
>      write_or_die(1, buf, size);
> +
> +    if (buf_must_free)
> +        free(buf);
> +
>      return 0;

Is this free() needed? The only caller is cmd_cat_file, which exits right away.

However, cmd_cat_file (and more importantly, cat_one_file) uses 'return' to exit instead of calling exit(), so it does create some valgrind noise. Is that what this is about fixing?

On the whole, I'm not too excited --- this patch seems to add complication for no obvious benefit.

Hope that helps, Jonathan