From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.esperi.org.uk (icebox.esperi.org.uk [81.187.191.129]) by sourceware.org (Postfix) with ESMTPS id 5F10D381EC91 for ; Thu, 15 Dec 2022 15:27:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5F10D381EC91 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=oracle.com Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=oracle.com Received: from loom (nix@sidle.srvr.nix [192.168.14.8]) by mail.esperi.org.uk (8.16.1/8.16.1) with ESMTPS id 2BFFQw3Q016357 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Thu, 15 Dec 2022 15:26:58 GMT From: Nick Alcock To: Felix Willgerodt , Alan Modra Cc: binutils@sourceware.org Subject: Re: [PATCH 1/1] libctf: Fix double free in ctf_link_add_cu_mapping. References: <20221207141137.1527113-1-felix.willgerodt@intel.com> Emacs: a Lisp interpreter masquerading as ... a Lisp interpreter! Date: Thu, 15 Dec 2022 15:26:58 +0000 In-Reply-To: (Alan Modra via Binutils's message of "Thu, 8 Dec 2022 11:52:53 +1030") Message-ID: <87ilicelh9.fsf@esperi.org.uk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.1.91 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-DCC-wuwien-Metrics: loom 1290; Body=3 Fuz1=3 Fuz2=3 X-Spam-Status: No, score=-21.6 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KHOP_HELO_FCRDNS,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 8 Dec 2022, Alan Modra via Binutils stated: > On Wed, Dec 07, 2022 at 03:11:37PM +0100, Felix Willgerodt via Binutils wrote: >> This fixes a potential double free which can occur if we jump to >> oom_noerrno after the first free. >> >> I am not very familiar with libctf, so any comments are welcome. >> I am wondering if the right solution wouldn't be to free both t and f before >> the "return 0". But I didn't fully understand the code and saw that other >> users of ctf_dynhash_insert() also don't free the key manually. > > No, they can't be freed if successfully inserted into the hash table, > but "t" should indeed be freed if already inserted. I'm applying the > following fix. > > * ctf-link.c (ctf_link_add_cu_mapping): Set t NULL after free. > > diff --git a/libctf/ctf-link.c b/libctf/ctf-link.c > index 702f2b4d5fe..902b4408cd6 100644 > --- a/libctf/ctf-link.c > +++ b/libctf/ctf-link.c > @@ -431,7 +431,10 @@ ctf_link_add_cu_mapping (ctf_dict_t *fp, const char *from, const char *to) > } > } > else > - free (t); > + { > + free (t); > + t = NULL; > + } > > if (ctf_dynhash_insert (one_out, f, NULL) < 0) > { Looks good! Sorry about that. This function has been subject to more OOM-related memory allocation problems than everything else in libctf combined :/ -- NULL && (void)