Re: [PATCH v2] libelf: {de,}compress: ensure zlib resource cleanup

[Mark Wielaard <mark@klomp.org>]

[-- 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