From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2a07:de40:b251:101:10:150:64:2]) by sourceware.org (Postfix) with ESMTPS id 37B3D3858C24 for ; Fri, 5 Apr 2024 15:11:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 37B3D3858C24 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 37B3D3858C24 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a07:de40:b251:101:10:150:64:2 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712329862; cv=none; b=NKtqii+FVZIwcQKPE/V4np3A4+zdsS3CcvfNjGEMYeKbtfKpqbZPwdJgdHnWjlxNhjBRqWGrD3GkYwUp2xyEDIveKNhiKWP/RpSyusKlmYL8ZeqfXzq1JGPMT+0jeQsZwKquFtqtwiTD8IFqJVj9zrLF5BR13/HWxXS66lrTIfQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712329862; c=relaxed/simple; bh=ykUwDDiJHDa6rlyDclxqePoESxVJxVEQVROCpNteqA8=; h=DKIM-Signature:DKIM-Signature:DKIM-Signature:DKIM-Signature: Message-ID:Date:MIME-Version:Subject:To:From; b=WZPjLYWjOI+f3w5zX0tMPDdclf9IO+icujT4lQtWtNfMPDl1Q1fyNRxM4/m5Em0UYLGQtH8B1pwvof5Zas6n8awlyEyETGuGphbkkKg+UR8I9yt6HZ/vpYNdXC3L8dJrAK9ZIpyav3+UVSDTVQ/OrNquPew0DDsQjSBafC79nX8= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from imap2.dmz-prg2.suse.org (imap2.dmz-prg2.suse.org [IPv6:2a07:de40:b281:104:10:150:64:98]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 3A73C1F7DD; Fri, 5 Apr 2024 15:10:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1712329859; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=RGvEuxkxFPYTNPLifZbF7W+Lhq6GibM3ESB7BSqMQD4=; b=Kio7ZS0w3atTN22M4XjNyxwnB/5QdkmT3z9wvezcxz6xqN4ikcBLgqU+4wq58nSclXBhXW nfRRdvjhEHx6JP92dJ4xoCwQVqp856tQ/SIx6r0USdWHNdGP1R9YhH3qBPlDVU7AJsefWI 4WYhlnvpbzRDhI7VSJZ8gF+U5i80Snc= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1712329859; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=RGvEuxkxFPYTNPLifZbF7W+Lhq6GibM3ESB7BSqMQD4=; b=q5Syn0q4mmYeeUYNOrOljKA4fyGXT9vebKuQx/j8ij5LC66fC7X/LpkMev5DWVMC6/8aL7 1VJmo2VR1JQHkgDA== Authentication-Results: smtp-out2.suse.de; dkim=pass header.d=suse.de header.s=susede2_rsa header.b=dWfZp+bt; dkim=pass header.d=suse.de header.s=susede2_ed25519 header.b=W8mXWJJb DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1712329858; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=RGvEuxkxFPYTNPLifZbF7W+Lhq6GibM3ESB7BSqMQD4=; b=dWfZp+btksKB5ht7r5qNYHBnUJpK3I8H/VNzkYHx4IFq29cMhY+96Rs8RT0b+zQXjpyFcI C5bwo/iP+KK3utwdmfRHO8MxHaifId12HRhbro48kdAv3FIyOPRQu76F+7u3NRQmMzwvZ7 wpLvrRmJyRqxMOqe0YrOcfjjvexkl2M= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1712329858; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=RGvEuxkxFPYTNPLifZbF7W+Lhq6GibM3ESB7BSqMQD4=; b=W8mXWJJbxQDgwqeX9bt6HEwjdb6cNZJyjd3Q1gthHj4sIKRYBW+QQR+fZi3VMuN5gE2NOt 35QZgMmezc6m7wBA== Received: from imap2.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap2.dmz-prg2.suse.org (Postfix) with ESMTPS id 13822139F1; Fri, 5 Apr 2024 15:10:58 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap2.dmz-prg2.suse.org with ESMTPSA id fJZcA4IUEGZSUwAAn2gu4w (envelope-from ); Fri, 05 Apr 2024 15:10:58 +0000 Message-ID: Date: Fri, 5 Apr 2024 17:11:04 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] Fix an out of bounds array access in, find_epilogue_using_linetable To: Bernd Edlinger , "gdb-patches@sourceware.org" Cc: Guinevere Larsen References: <0e153f6d-1f99-49c3-94a4-cf273ef94f93@suse.de> Content-Language: en-US From: Tom de Vries In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Level: X-Spamd-Result: default: False [-4.50 / 50.00]; BAYES_HAM(-3.00)[100.00%]; NEURAL_HAM_LONG(-1.00)[-1.000]; R_DKIM_ALLOW(-0.20)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; NEURAL_HAM_SHORT(-0.20)[-1.000]; MIME_GOOD(-0.10)[text/plain]; XM_UA_NO_VERSION(0.01)[]; MX_GOOD(-0.01)[]; FREEMAIL_TO(0.00)[hotmail.de,sourceware.org]; TO_DN_SOME(0.00)[]; TO_DN_EQ_ADDR_SOME(0.00)[]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; MIME_TRACE(0.00)[0:+]; FUZZY_BLOCKED(0.00)[rspamd.com]; ARC_NA(0.00)[]; FREEMAIL_ENVRCPT(0.00)[hotmail.de]; SPAMHAUS_XBL(0.00)[2a07:de40:b281:104:10:150:64:98:from]; RCVD_COUNT_TWO(0.00)[2]; FROM_EQ_ENVFROM(0.00)[]; FROM_HAS_DN(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; RCPT_COUNT_THREE(0.00)[3]; RCVD_VIA_SMTP_AUTH(0.00)[]; RCVD_TLS_ALL(0.00)[]; DKIM_TRACE(0.00)[suse.de:+]; DBL_BLOCKED_OPENRESOLVER(0.00)[suse.de:dkim,imap2.dmz-prg2.suse.org:helo,imap2.dmz-prg2.suse.org:rdns] X-Rspamd-Action: no action X-Rspamd-Queue-Id: 3A73C1F7DD X-Rspamd-Server: rspamd1.dmz-prg2.suse.org X-Spam-Score: -4.50 X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,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 4/1/24 18:41, Bernd Edlinger wrote: > On 4/1/24 11:07, Tom de Vries wrote: >> On 3/31/24 12:45, Bernd Edlinger wrote: >>> This causes random test failures like these: >>> >> >> Hi Bernd, >> >> thanks for the analysis and the patch. >> >> I think with "this" you mean $subject, which was not immediately clear to me when reading it on the mailing list where there's a whole bunch of text inbetween, so please consider spelling it out or using $subject or some such. >> > > ok will do. > >>> FAIL: gdb.base/unwind-on-each-insn-amd64.exp: foo: instruction 6: $fba_value == $fn_fba >>> FAIL: gdb.base/unwind-on-each-insn-amd64.exp: foo: instruction 6: check frame-id matches >>> FAIL: gdb.base/unwind-on-each-insn-amd64.exp: foo: instruction 6: bt 2 >>> FAIL: gdb.base/unwind-on-each-insn-amd64.exp: foo: instruction 6: up >>> FAIL: gdb.base/unwind-on-each-insn-amd64.exp: foo: instruction 6: $sp_value == $::main_sp >>> FAIL: gdb.base/unwind-on-each-insn-amd64.exp: foo: instruction 6: $fba_value == $::main_fba >>> FAIL: gdb.base/unwind-on-each-insn-amd64.exp: foo: instruction 6: [string equal $fid $::main_fid] >>> >>> Here the read happens below the first element of the line >>> table, and the test failure depends on the value that is >>> read from there. >>> >> >> Using this description, I managed to minimally rewrite the loop into a form where I could easily add an assert, and then add an assert that reliably detects the problem: >> ... >>       while (1) >>         { >>           gdb_assert (it >= &linetable->item[0]); >>           if (it->unrelocated_pc () >= unrel_start) >>             ; >>           else >>             break; >>           if (it->epilogue_begin) >>             return {it->pc (objfile)}; >>           it --; >>         } >> ... >> >>> Theoretically it is also possible that std::lower_bound >>> returns a pointer exactly at the upper bound of the line table, >>> also here the read value is undefined, that happens in this test: >>> >>> FAIL: gdb.dwarf2/dw2-epilogue-begin.exp: confirm watchpoint doesn't trigger >>> >> >> Ack, thanks for finding and reporting that. >> >>> Fixes: 528b729be1a2 ("gdb/dwarf2: Add support for DW_LNS_set_epilogue_begin in line-table") >>> --- >>>   gdb/symtab.c | 3 +-- >>>   1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/gdb/symtab.c b/gdb/symtab.c >>> index 86603dfebc3..2fc8e819932 100644 >>> --- a/gdb/symtab.c >>> +++ b/gdb/symtab.c >>> @@ -4177,11 +4177,10 @@ find_epilogue_using_linetable (CORE_ADDR func_addr) >>>          return lte.unrelocated_pc () < pc; >>>        }); >>>   -      while (it->unrelocated_pc () >= unrel_start) >>> +      while (it > linetable->item && (--it)->unrelocated_pc () >= unrel_start) >>>         { >>>       if (it->epilogue_begin) >>>         return {it->pc (objfile)}; >>> -    it --; >>>         } >>>       } >>>     return {}; >> >> I think this makes the while condition fairly convoluted. >> >> Also, AFAIU the problem you mention related to "pointer exactly at the upper bound of the line table", may or may not happen, and the new --it placement that seems intended to address that is executed unconditionally, so wouldn't this sometimes skip an item? >> > > No, because the upper limit is exactly where the next function starts, > so it will evaluate the first line table entry of the following function, > probably harmless, since that should not be the epilogue of the function. > Looking at the "pointer exactly at the upper bound of the line table" problem a bit more, I think it can happen that it->epilogue_begin is evaluated for an end_sequence entry. My guess is that this is harmless, but it's better to avoid it. Atm it's hard to reason about what happens because we try to handle several initial scenarios in the loop. It's best to untangle this: - handle exceptional situations before the loop, and - reserve the loop to handle the steady state: iterating over proper line-table entries of the current function. >> I think things can be solved simpler by: >> - using a trivial for loop that expresses the boundary of the line >>   table, and >> - moving the while condition into the loop, while >> - handling the exceptional case explictly, outside the loop: >> ... >>       if (it == &linetable->item[linetable->nitems]) >>         it--; >> > > no, that is also wrong if linetable->nitems == 0, `it` is > &linetable->item[-1], and the comparing that value to > &linetable->item[0] yields an undefined boolean value. I see, I forgot about that, thanks for pointing that out. It's good to make explicit in the implementation that and where we're avoid this. Btw, if "linetable->nitems == 0" indeed can happen, it's best handled using an early-out. > And as I said above looking at the line table entry > that belongs to the next function is asking for trouble. > >>       for (; it >= &linetable->item[0]; it--) >>         { >>           if (it->unrelocated_pc () < unrel_start) >>             break; >>           if (it->epilogue_begin) >>             return {it->pc (objfile)}; >>         } >> ... >> >> WDYT? >> > > Even decrementing `it` below the first element of an array > is undefined behavior, avoiding that is the main reason > why I wrote the if condition in this way. > > I would agree to add a comment here which explains how the > loop works. > > OK? > I've written a version that avoids the undefined behaviour decrement, clearly advertises this, handles cornercases explicitly and adds and reorganizes comments, as well as adds some asserts. I've submitted it as a v3, which also adds an additional test-case. Thanks, - Tom