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

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

[Alexander Kuleshov]

Sometimes we should free output buffer in cat_one_file, but not always.
It must to be freed only if it was allocated during git cat-file -t,
commit,tag and tree types handling.

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 builtin/cat-file.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f8d8129..f5dad60 100644
--- a/builtin/cat-file.c
+++ 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;
     unsigned long size;
     struct object_context obj_context;

@@ -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);
-        if (!buf)
+        buf_must_free = 1;
+
+        if (!buf) {
+            free(buf);
             die("Cannot read object %s", obj_name);
+        }

         /* otherwise just spit out the data */
         break;
@@ -100,6 +106,7 @@ static int cat_one_file(int opt, const char
*exp_type, const char *obj_name)
              */
         }
         buf = read_object_with_reference(sha1, exp_type, &size, NULL);
+        buf_must_free = 1;
         break;

     default:
@@ -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;
 }

-- 
2.2.0.rc3.224.gb2e243b

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

[Jonathan Nieder]

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

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

[Eric Sunshine]

On Wed, Nov 26, 2014 at 7:24 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Alexander Kuleshov wrote:
> [...]
>> +++ b/builtin/cat-file.c
>> @@ -68,9 +69,14 @@ static int cat_one_file(int opt, const char
>> +        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.

Moreover, 'buf' can only be NULL inside the conditional, so the free()
is pointless.