From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 75610 invoked by alias); 25 Mar 2019 19:54:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 75232 invoked by uid 89); 25 Mar 2019 19:54:11 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_STOCKGEN,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.1 spammy=HContent-Transfer-Encoding:8bit X-HELO: mailsec111.isp.belgacom.be Received: from mailsec111.isp.belgacom.be (HELO mailsec111.isp.belgacom.be) (195.238.20.107) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 25 Mar 2019 19:54:08 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1553543648; x=1585079648; h=message-id:subject:from:to:cc:date:in-reply-to: references:mime-version:content-transfer-encoding; bh=ckW7Bom8j+vEwkYzpjfjcRuVssd7iitWNsFYu2ej+G8=; b=E93lgYh18ih2i56nryS2rg7WbmPAIXLp05xdoVMrg/rFbM1s3rjTn9L/ fliuiANWyf9mpHxvwtE3y89HUUagoA==; Received: from 147.122-130-109.adsl-dyn.isp.belgacom.be (HELO md) ([109.130.122.147]) by relay.skynet.be with ESMTP/TLS/AES256-GCM-SHA384; 25 Mar 2019 20:54:06 +0100 Message-ID: <1553543645.1504.7.camel@skynet.be> Subject: Re: [RFAv2] Fix buffer overflow regression due to minsym malloc-ed instead of obstack-ed. From: Philippe Waroquiers To: Tom Tromey Cc: gdb-patches@sourceware.org Date: Mon, 25 Mar 2019 19:54:00 -0000 In-Reply-To: <87h8brj7ie.fsf@tromey.com> References: <20190324091856.2529-1-philippe.waroquiers@skynet.be> <87h8brj7ie.fsf@tromey.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-03/txt/msg00566.txt.bz2 On Mon, 2019-03-25 at 09:31 -0600, Tom Tromey wrote: > Philippe> + int n_after_msymbol = minsym.objfile->per_bfd->minimal_symbol_count > Philippe> + - (msymbol - minsym.objfile->per_bfd->msymbols.get ()) > Philippe> + - 1; > > What do you think of the appended instead? > The idea is to make the last element more explicit. Yes, that looks better, 2 minor comments below. Thanks Philippe > > Tom > > diff --git a/gdb/minsyms.c b/gdb/minsyms.c > index b95e9ef6e8b..03743e3062b 100644 > --- a/gdb/minsyms.c > +++ b/gdb/minsyms.c > @@ -1480,11 +1480,10 @@ find_solib_trampoline_target (struct frame_info *frame, CORE_ADDR pc) > CORE_ADDR > minimal_symbol_upper_bound (struct bound_minimal_symbol minsym) > { > - int i; > short section; > struct obj_section *obj_section; > CORE_ADDR result; > - struct minimal_symbol *msymbol; > + struct minimal_symbol *iter, *msymbol; > > gdb_assert (minsym.minsym != NULL); > > @@ -1499,21 +1498,24 @@ minimal_symbol_upper_bound (struct bound_minimal_symbol minsym) > other sections, to find the next symbol in this section with a > different address. */ > > + struct minimal_symbol *last > + = (minsym.objfile->per_bfd->msymbols.get () > + + minsym.objfile->per_bfd->minimal_symbol_count); Are the parenthesis needed here ? Also, I find the name 'last' a little bit confusing, as in the loop below, last is not handled. Maybe last could be the 'real' last i.e. as: minsym.objfile->per_bfd->msymbols.get () +        + minsym.objfile->per_bfd->minimal_symbol_count - 1; and have the '< last' conditions below then be '<= last'. That makes more clear for me that we handle the last element of the array. > msymbol = minsym.minsym; > section = MSYMBOL_SECTION (msymbol); > - for (i = 1; MSYMBOL_LINKAGE_NAME (msymbol + i) != NULL; i++) > + for (iter = msymbol + 1; iter < last; ++iter) > { > - if ((MSYMBOL_VALUE_RAW_ADDRESS (msymbol + i) > + if ((MSYMBOL_VALUE_RAW_ADDRESS (iter) > != MSYMBOL_VALUE_RAW_ADDRESS (msymbol)) > - && MSYMBOL_SECTION (msymbol + i) == section) > + && MSYMBOL_SECTION (iter) == section) > break; > } > > obj_section = MSYMBOL_OBJ_SECTION (minsym.objfile, minsym.minsym); > - if (MSYMBOL_LINKAGE_NAME (msymbol + i) != NULL > - && (MSYMBOL_VALUE_ADDRESS (minsym.objfile, msymbol + i) > + if (iter < last > + && (MSYMBOL_VALUE_ADDRESS (minsym.objfile, iter) > < obj_section_endaddr (obj_section))) > - result = MSYMBOL_VALUE_ADDRESS (minsym.objfile, msymbol + i); > + result = MSYMBOL_VALUE_ADDRESS (minsym.objfile, iter); > else > /* We got the start address from the last msymbol in the objfile. > So the end address is the end of the section. */