public inbox for elfutils@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] libelf: decompress: ensure zlib resource cleanup
@ 2020-03-15 22:03 Matthias Maennich
  2020-03-15 23:10 ` Mark Wielaard
  2020-03-20 11:17 ` [PATCH v2] libelf: {de,}compress: " Matthias Maennich
  0 siblings, 2 replies; 9+ messages in thread
From: Matthias Maennich @ 2020-03-15 22:03 UTC (permalink / raw)
  To: elfutils-devel; +Cc: kernel-team, maennich

__libelf_decompress would only cleanup zlib resources via inflateEnd()
in case inflating was successful, but would leak memory if not. Fix this
by calling inflateEnd() in the error case as well.

Fixes: 272018bba1f2 ("libelf: Add elf_compress and elf_compress_gnu.")
Signed-off-by: Matthias Maennich <maennich@google.com>
---
 libelf/elf_compress.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
index 244467b5e3ae..beb1834bbbd7 100644
--- a/libelf/elf_compress.c
+++ b/libelf/elf_compress.c
@@ -257,6 +257,7 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t size_out)
   if (unlikely (zrc != Z_OK) || unlikely (z.avail_out != 0))
     {
       free (buf_out);
+      inflateEnd(&z);
       __libelf_seterrno (ELF_E_DECOMPRESS_ERROR);
       return NULL;
     }
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] libelf: decompress: ensure zlib resource cleanup
  2020-03-15 22:03 [PATCH] libelf: decompress: ensure zlib resource cleanup Matthias Maennich
@ 2020-03-15 23:10 ` Mark Wielaard
  2020-03-15 23:42   ` Matthias Maennich
  2020-03-20 11:17 ` [PATCH v2] libelf: {de,}compress: " Matthias Maennich
  1 sibling, 1 reply; 9+ messages in thread
From: Mark Wielaard @ 2020-03-15 23:10 UTC (permalink / raw)
  To: Matthias Maennich, elfutils-devel; +Cc: kernel-team

Hi Matthias,

On Sun, 2020-03-15 at 23:03 +0100, Matthias Maennich via Elfutils-devel wrote:
> __libelf_decompress would only cleanup zlib resources via inflateEnd()
> in case inflating was successful, but would leak memory if not. Fix this
> by calling inflateEnd() in the error case as well.
> 
> Fixes: 272018bba1f2 ("libelf: Add elf_compress and elf_compress_gnu.")
> Signed-off-by: Matthias Maennich <maennich@google.com>
> ---
>  libelf/elf_compress.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
> index 244467b5e3ae..beb1834bbbd7 100644
> --- a/libelf/elf_compress.c
> +++ b/libelf/elf_compress.c
> @@ -257,6 +257,7 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t size_out)
>    if (unlikely (zrc != Z_OK) || unlikely (z.avail_out != 0))
>      {
>        free (buf_out);
> +      inflateEnd(&z);
>        __libelf_seterrno (ELF_E_DECOMPRESS_ERROR);
>        return NULL;
>      }

This looks correct at first sight, but...

Just before this hunk we do:

  if (likely (zrc == Z_OK))
    zrc = inflateEnd (&z);

So, zrc can be !Z_OK because the earlier inflateEnd() failed, which
might cause it to call inflateEnd() twice (which might be fine, I
dunno). Should we maybe ignore the error if inflateEnd() and just call
it unconditionally before (ignoring its return code)?

So, replace:
  if (... Z_OK) zrc = inflateEnd (&z);
with unconditionally ending the stream:
  (void)inflateEnd(&z);

What do you think?

Cheers,

Mark

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] libelf: decompress: ensure zlib resource cleanup
  2020-03-15 23:10 ` Mark Wielaard
@ 2020-03-15 23:42   ` Matthias Maennich
  2020-03-16 12:05     ` Mark Wielaard
  0 siblings, 1 reply; 9+ messages in thread
From: Matthias Maennich @ 2020-03-15 23:42 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: elfutils-devel, kernel-team

Hi Mark!

Thanks for the quick response!

On Mon, Mar 16, 2020 at 12:10:51AM +0100, Mark Wielaard wrote:
>Hi Matthias,
>
>On Sun, 2020-03-15 at 23:03 +0100, Matthias Maennich via Elfutils-devel wrote:
>> __libelf_decompress would only cleanup zlib resources via inflateEnd()
>> in case inflating was successful, but would leak memory if not. Fix this
>> by calling inflateEnd() in the error case as well.
>>
>> Fixes: 272018bba1f2 ("libelf: Add elf_compress and elf_compress_gnu.")
>> Signed-off-by: Matthias Maennich <maennich@google.com>
>> ---
>>  libelf/elf_compress.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
>> index 244467b5e3ae..beb1834bbbd7 100644
>> --- a/libelf/elf_compress.c
>> +++ b/libelf/elf_compress.c
>> @@ -257,6 +257,7 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t size_out)
>>    if (unlikely (zrc != Z_OK) || unlikely (z.avail_out != 0))
>>      {
>>        free (buf_out);
>> +      inflateEnd(&z);
>>        __libelf_seterrno (ELF_E_DECOMPRESS_ERROR);
>>        return NULL;
>>      }
>
>This looks correct at first sight, but...
>
>Just before this hunk we do:
>
>  if (likely (zrc == Z_OK))
>    zrc = inflateEnd (&z);
>
>So, zrc can be !Z_OK because the earlier inflateEnd() failed, which
>might cause it to call inflateEnd() twice (which might be fine, I
>dunno). Should we maybe ignore the error if inflateEnd() and just call
>it unconditionally before (ignoring its return code)?
>
>So, replace:
>  if (... Z_OK) zrc = inflateEnd (&z);
>with unconditionally ending the stream:
>  (void)inflateEnd(&z);
>

I prefer your variant (and it was my first version of the patch)
independently from what comes below.

Having said that: I looked up what inflateEnd() does and the worst that
could happen is returning an error that we anyway ignore. So, duplicate
calls are not an issue. Also, for the compression part we call
deflateEnd() via a macro in the same duplicate fashion. Hence I
consistently used the same pattern for inflateEnd(). And last, I wanted
to keep that existing error handling. OTOH, projects (including the
example code of zlib [1]) usually just unconditionally call inflateEnd()
ignoring any error codes. So, your call :-)

Cheers,
Matthias

[1] https://zlib.net/zlib_how.html

>What do you think?
>
>Cheers,
>
>Mark

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] libelf: decompress: ensure zlib resource cleanup
  2020-03-15 23:42   ` Matthias Maennich
@ 2020-03-16 12:05     ` Mark Wielaard
  2020-03-16 12:18       ` Mark Wielaard
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Wielaard @ 2020-03-16 12:05 UTC (permalink / raw)
  To: Matthias Maennich; +Cc: elfutils-devel, kernel-team

Hi Matthias,

On Mon, Mar 16, 2020 at 12:42:22AM +0100, Matthias Maennich wrote:
> > Just before this hunk we do:
> > 
> >  if (likely (zrc == Z_OK))
> >    zrc = inflateEnd (&z);
> > 
> > So, zrc can be !Z_OK because the earlier inflateEnd() failed, which
> > might cause it to call inflateEnd() twice (which might be fine, I
> > dunno). Should we maybe ignore the error if inflateEnd() and just call
> > it unconditionally before (ignoring its return code)?
> > 
> > So, replace:
> >  if (... Z_OK) zrc = inflateEnd (&z);
> > with unconditionally ending the stream:
> >  (void)inflateEnd(&z);
> > 
> 
> I prefer your variant (and it was my first version of the patch)
> independently from what comes below.
> 
> Having said that: I looked up what inflateEnd() does and the worst that
> could happen is returning an error that we anyway ignore. So, duplicate
> calls are not an issue. Also, for the compression part we call
> deflateEnd() via a macro in the same duplicate fashion. Hence I
> consistently used the same pattern for inflateEnd(). And last, I wanted
> to keep that existing error handling. OTOH, projects (including the
> example code of zlib [1]) usually just unconditionally call inflateEnd()
> ignoring any error codes. So, your call :-)

Thanks for looking into this so closely. I think it is best to just do
as the example code does. Always call inflateEnd (), ignoring the
return code and not care whether (in an error case) it might be called
twice. And to make it consistent in this file. So:

diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
index 244467b5..32422edc 100644
--- a/libelf/elf_compress.c
+++ b/libelf/elf_compress.c
@@ -197,7 +197,7 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data,
     }
   while (flush != Z_FINISH); /* More data blocks.  */
 
-  zrc = deflateEnd (&z);
+  deflateEnd (&z);
   if (zrc != Z_OK)
     {
       __libelf_seterrno (ELF_E_COMPRESS_ERROR);
@@ -251,8 +251,7 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t size_out)
        }
       zrc = inflateReset (&z);
     }
-  if (likely (zrc == Z_OK))
-    zrc = inflateEnd (&z);
+  inflateEnd (&z);
 
   if (unlikely (zrc != Z_OK) || unlikely (z.avail_out != 0))
     {

Thanks,

Mark

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] libelf: decompress: ensure zlib resource cleanup
  2020-03-16 12:05     ` Mark Wielaard
@ 2020-03-16 12:18       ` Mark Wielaard
  0 siblings, 0 replies; 9+ messages in thread
From: Mark Wielaard @ 2020-03-16 12:18 UTC (permalink / raw)
  To: Matthias Maennich; +Cc: kernel-team, elfutils-devel

Hi,

On Mon, Mar 16, 2020 at 01:05:26PM +0100, Mark Wielaard wrote:
> Thanks for looking into this so closely. I think it is best to just do
> as the example code does. Always call inflateEnd (), ignoring the
> return code and not care whether (in an error case) it might be called
> twice. And to make it consistent in this file. So:
> 
> diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
> index 244467b5..32422edc 100644
> --- a/libelf/elf_compress.c
> +++ b/libelf/elf_compress.c
> @@ -197,7 +197,7 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data,
>      }
>    while (flush != Z_FINISH); /* More data blocks.  */
>  
> -  zrc = deflateEnd (&z);
> +  deflateEnd (&z);
>    if (zrc != Z_OK)
>      {
>        __libelf_seterrno (ELF_E_COMPRESS_ERROR);
> @@ -251,8 +251,7 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t size_out)
>         }
>        zrc = inflateReset (&z);
>      }
> -  if (likely (zrc == Z_OK))
> -    zrc = inflateEnd (&z);
> +  inflateEnd (&z);
>  
>    if (unlikely (zrc != Z_OK) || unlikely (z.avail_out != 0))
>      {

Obviously, I didn't look that closely, because when you apply the
above it causes multiple testcase failures. Oops. Sorry, back to the
drawing board. I suspect the zrz = deflateEnd in the compress case is
different from inflateEnd in the decompress case...

Cheers,

Mark

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] libelf: {de,}compress: ensure zlib resource cleanup
  2020-03-15 22:03 [PATCH] libelf: decompress: ensure zlib resource cleanup Matthias Maennich
  2020-03-15 23:10 ` Mark Wielaard
@ 2020-03-20 11:17 ` Matthias Maennich
  2020-03-20 17:10   ` Mark Wielaard
  2020-04-24 23:28   ` Mark Wielaard
  1 sibling, 2 replies; 9+ messages in thread
From: Matthias Maennich @ 2020-03-20 11:17 UTC (permalink / raw)
  To: elfutils-devel; +Cc: kernel-team, maennich

__libelf_decompress would only cleanup zlib resources via inflateEnd()
in case inflating was successful, but would leak memory if not. Fix this
by calling inflateEnd() unconditionally.

__libelf_decompress did this all the time already, but called
deflateEnd() twice. That is not a (known) issue, but can be cleaned up
by ensuring all error paths use 'return deflate_cleanup' and the success
path calls deflateEnd() only once. Note, the deflate() needs to return
Z_STREAM_END to indicate we are done. Hence change the condition.

Fixes: 272018bba1f2 ("libelf: Add elf_compress and elf_compress_gnu.")
Signed-off-by: Matthias Maennich <maennich@google.com>
---
 libelf/elf_compress.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
index 244467b5e3ae..b1b896890ff7 100644
--- a/libelf/elf_compress.c
+++ b/libelf/elf_compress.c
@@ -115,7 +115,7 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data,
     {
       free (out_buf);
       __libelf_seterrno (ELF_E_COMPRESS_ERROR);
-      return NULL;
+      return deflate_cleanup(NULL, NULL);
     }
 
   Elf_Data cdata;
@@ -197,13 +197,13 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data,
     }
   while (flush != Z_FINISH); /* More data blocks.  */
 
-  zrc = deflateEnd (&z);
-  if (zrc != Z_OK)
+  if (zrc != Z_STREAM_END)
     {
       __libelf_seterrno (ELF_E_COMPRESS_ERROR);
       return deflate_cleanup (NULL, NULL);
     }
 
+  deflateEnd (&z);
   *new_size = used;
   return out_buf;
 }
@@ -251,16 +251,15 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t size_out)
 	}
       zrc = inflateReset (&z);
     }
-  if (likely (zrc == Z_OK))
-    zrc = inflateEnd (&z);
 
   if (unlikely (zrc != Z_OK) || unlikely (z.avail_out != 0))
     {
       free (buf_out);
+      buf_out = NULL;
       __libelf_seterrno (ELF_E_DECOMPRESS_ERROR);
-      return NULL;
     }
 
+  inflateEnd(&z);
   return buf_out;
 }
 
-- 
2.25.1.696.g5e7596f4ac-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] libelf: {de,}compress: ensure zlib resource cleanup
  2020-03-20 11:17 ` [PATCH v2] libelf: {de,}compress: " Matthias Maennich
@ 2020-03-20 17:10   ` Mark Wielaard
  2020-04-24 23:28   ` Mark Wielaard
  1 sibling, 0 replies; 9+ messages in thread
From: Mark Wielaard @ 2020-03-20 17:10 UTC (permalink / raw)
  To: Matthias Maennich, elfutils-devel; +Cc: kernel-team

Hi Matthias,

On Fri, 2020-03-20 at 12:17 +0100, Matthias Maennich via Elfutils-devel wrote:
> __libelf_decompress would only cleanup zlib resources via inflateEnd()
> in case inflating was successful, but would leak memory if not. Fix this
> by calling inflateEnd() unconditionally.
> 
> __libelf_decompress did this all the time already, but called
> deflateEnd() twice. That is not a (known) issue, but can be cleaned up
> by ensuring all error paths use 'return deflate_cleanup' and the success
> path calls deflateEnd() only once. Note, the deflate() needs to return
> Z_STREAM_END to indicate we are done. Hence change the condition.

It was that last condition I had missed.
Added a ChangeLog entry and pushed to master.

Thanks,

Mark

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] libelf: {de,}compress: ensure zlib resource cleanup
  2020-03-20 11:17 ` [PATCH v2] libelf: {de,}compress: " Matthias Maennich
  2020-03-20 17:10   ` Mark Wielaard
@ 2020-04-24 23:28   ` Mark Wielaard
  2020-04-27  8:14     ` Matthias Maennich
  1 sibling, 1 reply; 9+ messages in thread
From: Mark Wielaard @ 2020-04-24 23:28 UTC (permalink / raw)
  To: Matthias Maennich; +Cc: elfutils-devel, kernel-team

[-- Attachment #1: Type: text/plain, Size: 3155 bytes --]

Hi,

On Fri, Mar 20, 2020 at 12:17:55PM +0100, Matthias Maennich via Elfutils-devel wrote:
> __libelf_decompress would only cleanup zlib resources via inflateEnd()
> in case inflating was successful, but would leak memory if not. Fix this
> by calling inflateEnd() unconditionally.
> 
> __libelf_decompress did this all the time already, but called
> deflateEnd() twice. That is not a (known) issue, but can be cleaned up
> by ensuring all error paths use 'return deflate_cleanup' and the success
> path calls deflateEnd() only once. Note, the deflate() needs to return
> Z_STREAM_END to indicate we are done. Hence change the condition.
> 
> Fixes: 272018bba1f2 ("libelf: Add elf_compress and elf_compress_gnu.")
> Signed-off-by: Matthias Maennich <maennich@google.com>
> ---
>  libelf/elf_compress.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
> index 244467b5e3ae..b1b896890ff7 100644
> --- a/libelf/elf_compress.c
> +++ b/libelf/elf_compress.c
> @@ -115,7 +115,7 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data,
>      {
>        free (out_buf);
>        __libelf_seterrno (ELF_E_COMPRESS_ERROR);
> -      return NULL;
> +      return deflate_cleanup(NULL, NULL);
>      }

I was sure this was correct. But we both missed that deflate_cleanup
is a macro that passes out_buf and frees it. So now it is freed
twice... Oops.

GCC10 (not released yet, but already in Fedora 32 beta) has a new
-fanalyzer option which does catch this:

elf_compress.c: In function ‘__libelf_compress’:
elf_compress.c:50:3: error: double-‘free’ of ‘out_buf’ [CWE-415] [-Werror=analyzer-double-free]
   50 |   free (out_buf);
      |   ^~~~~~~~~~~~~~
  ‘__libelf_compress’: events 1-10
    |
    |   50 |   free (out_buf);
    |      |   ~~~~~~~~~~~~~~
    |      |   |
    |      |   (10) second ‘free’ here; first ‘free’ was at (9)
    |......
    |   79 |   if (data == NULL)
    |      |      ^
    |      |      |
    |      |      (1) following ‘false’ branch (when ‘data’ is non-NULL)...
    |......
    |   86 |   Elf_Data *next_data = elf_getdata (scn, data);
    |      |   ~~~~~~~~
    |      |   |
    |      |   (2) ...to here
    |......
    |   91 |   *orig_addralign = data->d_align;
    |      |   ~   
    |      |   |
    |      |   (3) allocated here
    |......
    |  100 |   if (out_buf == NULL)
    |      |      ~
    |      |      |
    |      |      (4) assuming ‘out_buf’ is non-NULL
    |      |      (5) following ‘false’ branch (when ‘out_buf’ is non-NULL)...
    |......
    |  107 |   size_t used = hsize;
    |      |   ~~~~~~
    |      |   |
    |      |   (6) ...to here
    |......
    |  114 |   if (zrc != Z_OK)
    |      |      ~
    |      |      |
    |      |      (7) following ‘true’ branch (when ‘zrc != 0’)...
    |  115 |     {
    |  116 |       free (out_buf);
    |      |       ~~~~
    |      |       |
    |      |       (8) ...to here
    |      |       (9) first ‘free’ here
    |

Fixed by removing the free (out_buf) on line 116 as attached.

Cheers,

Mark

[-- Attachment #2: 0001-libelf-Fix-double-free-in-__libelf_compress-on-error.patch --]
[-- Type: text/x-diff, Size: 1490 bytes --]

From 0b2fc95c46dabf85d053b2f0c6aab217b9c5a9b8 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Sat, 25 Apr 2020 01:21:12 +0200
Subject: [PATCH] libelf: Fix double free in __libelf_compress on error path.

In commit 2092865a7e589ff805caa47e69ac9630f34d4f2a
"libelf: {de,}compress: ensure zlib resource cleanup" we added a
call to deflate_cleanup to make sure all resources were freed.
As GCC10 -fanalyzer points out that could cause a double free
of out_buf. Fix by removing the free (out_buf) in __libelf_compress.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libelf/ChangeLog      | 4 ++++
 libelf/elf_compress.c | 1 -
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 8f79a625..56f5354c 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,7 @@
+2020-04-25  Mark Wielaard  <mark@klomp.org>
+
+	* elf_compress.c (__libelf_compress): Remove free (out_buf).
+
 2020-03-18  Omar Sandoval  <osandov@fb.com>
 
 	* elf_getphdrnum.c (__elf_getphdrnum_rdlock): Call
diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
index b1b89689..e5d3d2e0 100644
--- a/libelf/elf_compress.c
+++ b/libelf/elf_compress.c
@@ -113,7 +113,6 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data,
   int zrc = deflateInit (&z, Z_BEST_COMPRESSION);
   if (zrc != Z_OK)
     {
-      free (out_buf);
       __libelf_seterrno (ELF_E_COMPRESS_ERROR);
       return deflate_cleanup(NULL, NULL);
     }
-- 
2.26.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] libelf: {de,}compress: ensure zlib resource cleanup
  2020-04-24 23:28   ` Mark Wielaard
@ 2020-04-27  8:14     ` Matthias Maennich
  0 siblings, 0 replies; 9+ messages in thread
From: Matthias Maennich @ 2020-04-27  8:14 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: elfutils-devel, kernel-team

On Sat, Apr 25, 2020 at 01:28:33AM +0200, Mark Wielaard wrote:
>Hi,
>
>On Fri, Mar 20, 2020 at 12:17:55PM +0100, Matthias Maennich via Elfutils-devel wrote:
>> __libelf_decompress would only cleanup zlib resources via inflateEnd()
>> in case inflating was successful, but would leak memory if not. Fix this
>> by calling inflateEnd() unconditionally.
>>
>> __libelf_decompress did this all the time already, but called
>> deflateEnd() twice. That is not a (known) issue, but can be cleaned up
>> by ensuring all error paths use 'return deflate_cleanup' and the success
>> path calls deflateEnd() only once. Note, the deflate() needs to return
>> Z_STREAM_END to indicate we are done. Hence change the condition.
>>
>> Fixes: 272018bba1f2 ("libelf: Add elf_compress and elf_compress_gnu.")
>> Signed-off-by: Matthias Maennich <maennich@google.com>
>> ---
>>  libelf/elf_compress.c | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
>> index 244467b5e3ae..b1b896890ff7 100644
>> --- a/libelf/elf_compress.c
>> +++ b/libelf/elf_compress.c
>> @@ -115,7 +115,7 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data,
>>      {
>>        free (out_buf);
>>        __libelf_seterrno (ELF_E_COMPRESS_ERROR);
>> -      return NULL;
>> +      return deflate_cleanup(NULL, NULL);
>>      }
>
>I was sure this was correct. But we both missed that deflate_cleanup
>is a macro that passes out_buf and frees it. So now it is freed
>twice... Oops.
>
>GCC10 (not released yet, but already in Fedora 32 beta) has a new
>-fanalyzer option which does catch this:
>
>elf_compress.c: In function ‘__libelf_compress’:
>elf_compress.c:50:3: error: double-‘free’ of ‘out_buf’ [CWE-415] [-Werror=analyzer-double-free]
>   50 |   free (out_buf);
>      |   ^~~~~~~~~~~~~~
>  ‘__libelf_compress’: events 1-10
>    |
>    |   50 |   free (out_buf);
>    |      |   ~~~~~~~~~~~~~~
>    |      |   |
>    |      |   (10) second ‘free’ here; first ‘free’ was at (9)
>    |......
>    |   79 |   if (data == NULL)
>    |      |      ^
>    |      |      |
>    |      |      (1) following ‘false’ branch (when ‘data’ is non-NULL)...
>    |......
>    |   86 |   Elf_Data *next_data = elf_getdata (scn, data);
>    |      |   ~~~~~~~~
>    |      |   |
>    |      |   (2) ...to here
>    |......
>    |   91 |   *orig_addralign = data->d_align;
>    |      |   ~
>    |      |   |
>    |      |   (3) allocated here
>    |......
>    |  100 |   if (out_buf == NULL)
>    |      |      ~
>    |      |      |
>    |      |      (4) assuming ‘out_buf’ is non-NULL
>    |      |      (5) following ‘false’ branch (when ‘out_buf’ is non-NULL)...
>    |......
>    |  107 |   size_t used = hsize;
>    |      |   ~~~~~~
>    |      |   |
>    |      |   (6) ...to here
>    |......
>    |  114 |   if (zrc != Z_OK)
>    |      |      ~
>    |      |      |
>    |      |      (7) following ‘true’ branch (when ‘zrc != 0’)...
>    |  115 |     {
>    |  116 |       free (out_buf);
>    |      |       ~~~~
>    |      |       |
>    |      |       (8) ...to here
>    |      |       (9) first ‘free’ here
>    |
>
>Fixed by removing the free (out_buf) on line 116 as attached.

Hi Mark!

Thanks for catching and fixing that!

>
>Cheers,
>
>Mark

From 0b2fc95c46dabf85d053b2f0c6aab217b9c5a9b8 Mon Sep 17 00:00:00 2001
>From: Mark Wielaard <mark@klomp.org>
>Date: Sat, 25 Apr 2020 01:21:12 +0200
>Subject: [PATCH] libelf: Fix double free in __libelf_compress on error path.
>
>In commit 2092865a7e589ff805caa47e69ac9630f34d4f2a
>"libelf: {de,}compress: ensure zlib resource cleanup" we added a
>call to deflate_cleanup to make sure all resources were freed.
>As GCC10 -fanalyzer points out that could cause a double free
>of out_buf. Fix by removing the free (out_buf) in __libelf_compress.
>
>Signed-off-by: Mark Wielaard <mark@klomp.org>
>---
> libelf/ChangeLog      | 4 ++++
> libelf/elf_compress.c | 1 -
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/libelf/ChangeLog b/libelf/ChangeLog
>index 8f79a625..56f5354c 100644
>--- a/libelf/ChangeLog
>+++ b/libelf/ChangeLog
>@@ -1,3 +1,7 @@
>+2020-04-25  Mark Wielaard  <mark@klomp.org>
>+
>+	* elf_compress.c (__libelf_compress): Remove free (out_buf).
>+
> 2020-03-18  Omar Sandoval  <osandov@fb.com>
> 
> 	* elf_getphdrnum.c (__elf_getphdrnum_rdlock): Call
>diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
>index b1b89689..e5d3d2e0 100644
>--- a/libelf/elf_compress.c
>+++ b/libelf/elf_compress.c
>@@ -113,7 +113,6 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data,
>   int zrc = deflateInit (&z, Z_BEST_COMPRESSION);
>   if (zrc != Z_OK)
>     {
>-      free (out_buf);

Maybe add a comment to the deflate_cleanup macro call then?

Cheers,
Matthias

>       __libelf_seterrno (ELF_E_COMPRESS_ERROR);
>       return deflate_cleanup(NULL, NULL);
>     }
>-- 
>2.26.0
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-04-27  8:14 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-15 22:03 [PATCH] libelf: decompress: ensure zlib resource cleanup Matthias Maennich
2020-03-15 23:10 ` Mark Wielaard
2020-03-15 23:42   ` Matthias Maennich
2020-03-16 12:05     ` Mark Wielaard
2020-03-16 12:18       ` Mark Wielaard
2020-03-20 11:17 ` [PATCH v2] libelf: {de,}compress: " Matthias Maennich
2020-03-20 17:10   ` Mark Wielaard
2020-04-24 23:28   ` Mark Wielaard
2020-04-27  8:14     ` Matthias Maennich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).