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

[Jonathan Nieder <jrnieder@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.)

[...]
> +++ 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.

[...]
> @@ -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?

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

[...]
> @@ -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