threads / patch / 38054

patchcat-file: fix a memory leak in cat_one_file

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

## tl;dr

3 messages between Nov 26, 2014 and Nov 27, 2014. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Alexander Kuleshov· Nov 26, 2014, 14:17 UTC · lore

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(-)
Show changes to builtin/cat-file.c +12 −1
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
Jonathan Nieder· Nov 27, 2014, 00:24 UTC · re: Alexander Kuleshov · lore

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

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

Eric Sunshine· Nov 27, 2014, 06:18 UTC · re: Jonathan Nieder · lore

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

On Wed, Nov 26, 2014 at 7:24 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 15 quoted lines
> 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.

← back to recent threads