From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (wildebeest.demon.nl [212.238.236.112]) by sourceware.org (Postfix) with ESMTPS id 1AEAA3877003 for ; Mon, 16 Mar 2020 12:05:47 +0000 (GMT) Received: from librem (deer0x15.wildebeest.org [172.31.17.151]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id DD7243027748; Mon, 16 Mar 2020 13:05:45 +0100 (CET) Received: by librem (Postfix, from userid 1000) id 43207C38AD; Mon, 16 Mar 2020 13:05:26 +0100 (CET) Date: Mon, 16 Mar 2020 13:05:26 +0100 From: Mark Wielaard To: Matthias Maennich Cc: elfutils-devel@sourceware.org, kernel-team@android.com Subject: Re: [PATCH] libelf: decompress: ensure zlib resource cleanup Message-ID: <20200316120526.GB39901@wildebeest.org> References: <20200315220329.82201-1-maennich@google.com> <20200315234222.GF21487@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200315234222.GF21487@google.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-24.5 required=5.0 tests=GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Mar 2020 12:05:48 -0000 Hi Matthias, On Mon, Mar 16, 2020 at 12:42:22AM +0100, Matthias Maennich wrote: > > 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 :-) Thanks for looking into this so closely. I think it is best to just do as the example code does. Always call inflateEnd (), ignoring the return code and not care whether (in an error case) it might be called twice. And to make it consistent in this file. So: diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c index 244467b5..32422edc 100644 --- a/libelf/elf_compress.c +++ b/libelf/elf_compress.c @@ -197,7 +197,7 @@ __libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data, } while (flush != Z_FINISH); /* More data blocks. */ - zrc = deflateEnd (&z); + deflateEnd (&z); if (zrc != Z_OK) { __libelf_seterrno (ELF_E_COMPRESS_ERROR); @@ -251,8 +251,7 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t size_out) } zrc = inflateReset (&z); } - if (likely (zrc == Z_OK)) - zrc = inflateEnd (&z); + inflateEnd (&z); if (unlikely (zrc != Z_OK) || unlikely (z.avail_out != 0)) { Thanks, Mark