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 252763858C83 for ; Wed, 11 Jan 2023 17:21:19 +0000 (GMT) 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 30BHLF5g025308 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Wed, 11 Jan 2023 17:21:16 GMT From: Nick Alcock To: binutils@sourceware.org Cc: Nick Clifton Subject: Re: [PATCH 2.40 v2 3/3] libctf: ctf-link outdated input check faulty References: <20230110130050.366404-1-nick.alcock@oracle.com> <20230110130050.366404-4-nick.alcock@oracle.com> Emacs: more boundary conditions than the Middle East. Date: Wed, 11 Jan 2023 17:21:15 +0000 In-Reply-To: <20230110130050.366404-4-nick.alcock@oracle.com> (Nick Alcock via Binutils's message of "Tue, 10 Jan 2023 13:00:50 +0000") Message-ID: <87r0w1oulw.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--Metrics: loom 1102; Body=2 Fuz1=2 Fuz2=2 X-Spam-Status: No, score=-19.3 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: Is this patch alone OK for 2.40? (After I push this series to master. Tomorrow, not today... all tests passed, crash on i686-pc-linux-gnu fixed, valgrind-clean, asan-clean.) (I plan to backport it all the way back to 2.36.) On 10 Jan 2023, Nick Alcock via Binutils spake thusly: > This check has a pair of faults which, combined, can lead to memory > corruption. Firstly, it assumes that the values of the ctf_link_inputs > hash are ctf_dict_t's: they are not, they are ctf_link_input_t's, a much > shorter structure. So the flags check which is the core of this is > faulty (but happens, by chance, to give the right output on most > architectures, since usually we happen to get a 0 here, so the test that > checks this usually passes). Worse, the warning that is emitted when > the test fails is added to the wrong dict -- it's added to the input > dict, whose warning list is never consumed, rendering the whole check > useless. But the dict it adds to is still the wrong type, so we end up > overwriting something deep in memory (or, much more likely, > dereferencing a garbage pointer and crashing). > > Fixing both reveals another problem: the link input is an *archive* > consisting of multiple members, so we have to consider whether to check > all of them for the outdated-func-info thing we are checking here. > However, no compiler exists that emits a mixture of members with this > flag on and members with it off, and the linker always reserializes (and > upgrades) such things when it sees them: so all members in a given > archive must have the same value of the flag, so we only need to check > one member per input archive. > > libctf/ > PR libctf/29983 > * ctf-link.c (ctf_link_warn_outdated_inputs): Get the types of > members of ctf_link_inputs right, fixing a possible spurious > tesst failure / wild pointer deref / overwrite. Emit the > warning message into the right dict. > --- > libctf/ctf-link.c | 35 +++++++++++++++++++++++++++++------ > 1 file changed, 29 insertions(+), 6 deletions(-) > > diff --git a/libctf/ctf-link.c b/libctf/ctf-link.c > index 2837168b2a6..df8fa3b9d9b 100644 > --- a/libctf/ctf-link.c > +++ b/libctf/ctf-link.c > @@ -1848,19 +1848,42 @@ ctf_link_warn_outdated_inputs (ctf_dict_t *fp) > { > ctf_next_t *i = NULL; > void *name_; > - void *ifp_; > + void *input_; > int err; > > - while ((err = ctf_dynhash_next (fp->ctf_link_inputs, &i, &name_, &ifp_)) == 0) > + while ((err = ctf_dynhash_next (fp->ctf_link_inputs, &i, &name_, &input_)) == 0) > { > const char *name = (const char *) name_; > - ctf_dict_t *ifp = (ctf_dict_t *) ifp_; > + ctf_link_input_t *input = (ctf_link_input_t *) input_; > + ctf_next_t *j = NULL; > + ctf_dict_t *ifp; > + int err; > + > + /* We only care about CTF archives by this point: lazy-opened archives > + have always been opened by this point, and short-circuited entries have > + a matching corresponding archive member. Entries with NULL clin_arc can > + exist, and constitute old entries renamed via a name changer: the > + renamed entries exist elsewhere in the list, so we can just skip > + those. */ > + > + if (!input->clin_arc) > + continue; > + > + /* All entries in the archive will necessarily contain the same > + CTF_F_NEWFUNCINFO flag, so we only need to check the first. We don't > + even need to do that if we can't open it for any reason at all: the > + link will fail later on regardless, since an input can't be opened. */ > + > + ifp = ctf_archive_next (input->clin_arc, &j, NULL, 0, &err); > + if (!ifp) > + continue; > + ctf_next_destroy (j); > > if (!(ifp->ctf_header->cth_flags & CTF_F_NEWFUNCINFO) > && (ifp->ctf_header->cth_varoff - ifp->ctf_header->cth_funcoff) > 0) > - ctf_err_warn (ifp, 1, 0, _("linker input %s has CTF func info but uses " > - "an old, unreleased func info format: " > - "this func info section will be dropped."), > + ctf_err_warn (fp, 1, 0, _("linker input %s has CTF func info but uses " > + "an old, unreleased func info format: " > + "this func info section will be dropped."), > name); > } > if (err != ECTF_NEXT_END) -- NULL && (void)