[PATCH v2] libelf: {de,}compress: ensure zlib resource cleanup
Mark Wielaard
mark@klomp.org
Fri Apr 24 23:28:33 GMT 2020
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libelf-Fix-double-free-in-__libelf_compress-on-error.patch
Type: text/x-diff
Size: 1491 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/elfutils-devel/attachments/20200425/ac7e7a40/attachment.bin>
More information about the Elfutils-devel
mailing list