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

[Matthias Maennich]

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
>