From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.smtpout.orange.fr (smtp-25.smtpout.orange.fr [80.12.242.25]) by sourceware.org (Postfix) with ESMTPS id DA6203858C74 for ; Thu, 13 Jul 2023 06:01:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DA6203858C74 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=jacob.remcomp.fr Authentication-Results: sourceware.org; spf=none smtp.mailfrom=jacob.remcomp.fr Received: from smtpclient.apple ([90.22.252.13]) by smtp.orange.fr with ESMTP id JpNqqkpXKF3kiJpNqqQWFI; Thu, 13 Jul 2023 08:00:59 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=wanadoo.fr; s=t20230301; t=1689228059; bh=m0YYTcHYPAi0uApg1W5CHwpH0epMXZKpLB/gdHd+S60=; h=From:Subject:Date:In-Reply-To:Cc:To:References; b=Du5J5uIs7Uzr0+pggjQb9D12FxGt3taPXOHszDWRwbDaHcgqvHTPoeAyVpM/4O3Gm /5/fVDPhHgImx2eFM3cv5mk/OfL6Nq2PV7yjohc5sGTvO2MlGL9C5gHhFYmxGhaLbW b78LPTflHuu0sgAjIKve4EHQ3Vl6gXvvvA/o3D/sy0ubiAcrct2lrWPOm/uLK82fNJ xsT4VRs4E28nk8gqvxugcJqHDYm4Trh7oxUurWVx+b/wWp4QOHKDKLUry3sgCz5exW qtFnG57MLbee7nA4Xhx78rcZrgUDzscE+TpiJSde6T26Nt1P6KixkP+M+qiKQGsXt9 /lbaa+dOuUKjw== X-ME-Helo: smtpclient.apple X-ME-Date: Thu, 13 Jul 2023 08:00:59 +0200 X-ME-IP: 90.22.252.13 From: jacob navia Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_5BBAEA7E-B34F-4F7A-9A01-F3B92BC7F5A9" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.600.7\)) Subject: Re: SUSPICIOUS CODE Date: Thu, 13 Jul 2023 08:00:48 +0200 In-Reply-To: Cc: binutils@sourceware.org To: Alan Modra References: X-Mailer: Apple Mail (2.3731.600.7) X-Spam-Status: No, score=-0.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,FORGED_SPF_HELO,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_NONE,SUBJ_ALL_CAPS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --Apple-Mail=_5BBAEA7E-B34F-4F7A-9A01-F3B92BC7F5A9 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi Mr Modra First, thanks for replying. Good thing you are available and can explain th= ings. Second, I tried to run the test suite but=E2=80=A6 no Makefile, no document= ation on how to run these tests=E2=80=A6 Impossible to continue unless you = would point me to some documentation about the tests. Alternatively you cou= ld just tell me which test fails and WHY this change is needed. Third, basically when looking if an address is between two borders we inclu= de the lower limit and exclude the upper limit. If you want to know if a is= between Addrx and Addrx+6 you will use Addrx , Addrx+1 until Addrx+5, excl= uding Addrx+6. This is because we use zero based array indices in C. This i= s obvious to you of course.=20 Fourth, the first two loops of =C2=AB get_frag_from_reloc =C2=BB use this p= rinciple. What bothers me is that with no justification the valid address r= ange is extended by one in the third. These relocations (stored in =C2=AB reloc_list =C2=BB ) are created by =C2= =AB create_note_reloc =C2=BB . The two calls to that function are in =C2=AB= maybe_generate_build_notes =C2=BB The address of the note is composed of t= he addition of two numbers, note_offset and desc2_offset, and in those call= s to =C2=AB create_note_reloc =C2=BB I see a division by 2 that *COULD* exp= lain the behavior you programmed since it is an integer division that round= s towards zero. But if that is the case, why not adding 1 to the division instead of runnin= g an additional loop with <=3D ??? In any case it would be nice if we added some comments to the code to expla= in why it is there so people reading the code do not pose unwanted question= s! :-) Jacob > Le 13 juil. 2023 =C3=A0 00:33, Alan Modra a =C3=A9crit= : >=20 > On Wed, Jul 12, 2023 at 05:12:13PM +0200, jacob navia wrote: >> Consider this code: >>=20 >> 1202 static fragS * get_frag_for_reloc (fragS *last_frag, >> 1203 const segment_info_type *seginfo, >> 1204 const struct reloc_list *r) >> 1205 {=20=20=20 >> 1206 fragS *f; >> 1207=20=20=20 >> 1208 for (f =3D last_frag; f !=3D NULL; f =3D f->fr_next) >> 1209 if (f->fr_address <=3D r->u.b.r.address >> 1210 && r->u.b.r.address < f->fr_address + f->fr_fix) >> 1211 return f; >> 1212=20 >> 1213 for (f =3D seginfo->frchainP->frch_root; f !=3D NULL; f =3D f->fr= _next) >> 1214 if (f->fr_address <=3D r->u.b.r.address >> 1215 && r->u.b.r.address < f->fr_address + f->fr_fix) >> 1216 return f; >> 1217=20=20=20 >> 1218 for (f =3D seginfo->frchainP->frch_root; f !=3D NULL; f =3D f->fr= _next) >> 1219 if (f->fr_address <=3D r->u.b.r.address >> 1220 && r->u.b.r.address <=3D f->fr_address + f->fr_fix) >> 1221 return f; >> 1222=20 >> 1223 as_bad_where (r->file, r->line, >> 1224 _("reloc not within (fixed part of) section")); >> 1225 return NULL; >> 1226 } >>=20 >> This function consists of 3 loops: 1208-1211, 1213 to 1216 and 1218 to 1= 221.=20 >>=20 >> Lines 1213 - 1216 are ALMOST identical to lines 1218 to 1221. The ONLY d= ifference that I can see is that the less in line 1215 is replaced by a les= s equal in line 1220. >>=20 >> But=E2=80=A6 why? >>=20 >> This code is searching the fragment that contains a given address in bet= ween the start and end addresses of the frags in question, either in the fr= agment list given by last_frag or in the list given by seginfo. >>=20 >> To know if a fragment is OK you should start with the given address and = stop one memory address BEFORE the limit given by fr_address + f->fr_fix. T= hat is what the first two loops are doing. The third loop repeats the secon= d one and changes the less to less equal, so if fr_address+fr_fix is one MO= RE than the address it will still pass. >>=20 >> Why it is doing that?=20 >=20 > Take out the third loop and run the testsuite. Does anything regress? >=20 >> If that code is correct, it is obvious that we could merge the second an= d third loops and put a <=3D in t he second one and erase the third one=E2= =80=A6 UNLESS priority should be given to matches that are less and not les= s equal, >=20 > That is exactly what needs to happen. >=20 >> what seems incomprehensible =E2=80=A6 to me. >>=20 >> This change was introduced on Aug 18th 2011 by Mr Alan Modra with the ra= ther terse comment: "(get_frag_for_reloc): New function. =C2=BB. There are = no further comments in the code at all. >=20 > Yes, I'm responsible for lots of suspicious code, but this isn't the > full history of get_frag_for_reloc. >=20 >> This code is run after all relocations are fixed just before the softwar= e writes them out. The code is in file =C2=AB write.c =C2=BB in the gas dir= ectory. Note that this code runs through ALL relocations lists each time fo= r EACH relocation, so it is quite expensive. In general the list data struc= ture is not really optimal here but that is another story. >=20 > The code does not run through all relocations, just those created with > the .reloc directive. >=20 >> Thanks in advance for your help. >>=20 >> Jacob >=20 > --=20 > Alan Modra > Australia Development Lab, IBM --Apple-Mail=_5BBAEA7E-B34F-4F7A-9A01-F3B92BC7F5A9--