Re: [PATCH] libelf: decompress: ensure zlib resource cleanup

[Matthias Maennich <maennich@google.com>]

Hi Mark!

Thanks for the quick response!

On Mon, Mar 16, 2020 at 12:10:51AM +0100, Mark Wielaard wrote:
>Hi Matthias,
>
>On Sun, 2020-03-15 at 23:03 +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() in the error case as well.
>>
>> Fixes: 272018bba1f2 ("libelf: Add elf_compress and elf_compress_gnu.")
>> Signed-off-by: Matthias Maennich <maennich@google.com>
>> ---
>>  libelf/elf_compress.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
>> index 244467b5e3ae..beb1834bbbd7 100644
>> --- a/libelf/elf_compress.c
>> +++ b/libelf/elf_compress.c
>> @@ -257,6 +257,7 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t size_out)
>>    if (unlikely (zrc != Z_OK) || unlikely (z.avail_out != 0))
>>      {
>>        free (buf_out);
>> +      inflateEnd(&z);
>>        __libelf_seterrno (ELF_E_DECOMPRESS_ERROR);
>>        return NULL;
>>      }
>
>This looks correct at first sight, but...
>
>Just before this hunk we do:
>
>  if (likely (zrc == Z_OK))
>    zrc = inflateEnd (&z);
>
>So, zrc can be !Z_OK because the earlier inflateEnd() failed, which
>might cause it to call inflateEnd() twice (which might be fine, I
>dunno). Should we maybe ignore the error if inflateEnd() and just call
>it unconditionally before (ignoring its return code)?
>
>So, replace:
>  if (... Z_OK) zrc = inflateEnd (&z);
>with unconditionally ending the stream:
>  (void)inflateEnd(&z);
>

I prefer your variant (and it was my first version of the patch)
independently from what comes below.

Having said that: I looked up what inflateEnd() does and the worst that
could happen is returning an error that we anyway ignore. So, duplicate
calls are not an issue. Also, for the compression part we call
deflateEnd() via a macro in the same duplicate fashion. Hence I
consistently used the same pattern for inflateEnd(). And last, I wanted
to keep that existing error handling. OTOH, projects (including the
example code of zlib [1]) usually just unconditionally call inflateEnd()
ignoring any error codes. So, your call :-)

Cheers,
Matthias

[1] https://zlib.net/zlib_how.html

>What do you think?
>
>Cheers,
>
>Mark