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