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 <maenn...@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
>From 0b2fc95c46dabf85d053b2f0c6aab217b9c5a9b8 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <m...@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 <m...@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 <m...@klomp.org>
+
+ * elf_compress.c (__libelf_compress): Remove free (out_buf).
+
2020-03-18 Omar Sandoval <osan...@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
