From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2a07:de40:b251:101:10:150:64:1]) by sourceware.org (Postfix) with ESMTPS id DED3B3858C39 for ; Tue, 9 Apr 2024 09:36:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DED3B3858C39 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 DED3B3858C39 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a07:de40:b251:101:10:150:64:1 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712655421; cv=none; b=Ln/lJJispFQjKAoNAGsRi7Kn7dDCAWlBWeRZTqz+V7H4j64Z4ati1QJSxhcoc9v3lDCWw+aoTaixP0VYLQUxhZ7FsvbODrjKAv7RrH8GpNEAPIBPkHDFx6QXzkPsblqkVNLAOEy7Bw2hzDjtmR7UX+u42+UwdlCcKpEc7hfyBNk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712655421; c=relaxed/simple; bh=4aVrT4Bb5qJroTVofM0tQJuwR5BqPLgMzDWGmCZdUzA=; h=DKIM-Signature:DKIM-Signature:DKIM-Signature:DKIM-Signature: Message-ID:Date:MIME-Version:Subject:To:From; b=rfI0t5a520OFuWtorhOlm38rPNskXd88laHyXXN5+T8ba/0OXhqwhPHqOZfPRfFMp2OgGiGVN4Q6rLgeELGplO/LQMenUBy3uyO+0lJFrHTwmSV4xHxnsxpg64RYZ4AWGCjWzNxHsbV3C6bd+plWpQrCYquQ4tolE2RCCeuKKpg= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from imap1.dmz-prg2.suse.org (imap1.dmz-prg2.suse.org [IPv6:2a07:de40:b281:104:10:150:64:97]) (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-out1.suse.de (Postfix) with ESMTPS id CC8E9338F8; Tue, 9 Apr 2024 09:36:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1712655417; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=PhTMYpFqsBWH4kD+lgUO/ZuvN+fx+XWlNL7akFPD3cE=; b=RjZ0+qIiouRyAmOX5RdgTVx5q3raLpl1F5M7rcGSD+NPXxYhBU8BgVCyeQBM6nw5M/3y2S l6NV2NlhdkJBhwBSjeySclp696vf4qoVWckTFkuyc1fu6Rgyb5MKP78SYdBDoKfxuNX+6K tfkRHCQzoPQTgEJ3HOVLVp7kpyq1GXc= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1712655417; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=PhTMYpFqsBWH4kD+lgUO/ZuvN+fx+XWlNL7akFPD3cE=; b=uefWb1IV7vdzbaaeyTaO1FM7Mq9Z3eisKKnm1ztxAI2wvhJfGW8TtjtEAB2dfppoFcMEay FvFREmAi3h58R5Ag== Authentication-Results: smtp-out1.suse.de; dkim=pass header.d=suse.de header.s=susede2_rsa header.b="Bsrh/Ll/"; dkim=pass header.d=suse.de header.s=susede2_ed25519 header.b=3fl9w+7g DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1712655416; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=PhTMYpFqsBWH4kD+lgUO/ZuvN+fx+XWlNL7akFPD3cE=; b=Bsrh/Ll/dBXyCfSrYC/Og7+/bK2MY2vpuNzk2O7u5Te2ZcT4mMtc0xaFghYembKl4rJwco NPO+7K2lhlTRyMqj2Bn/CZBn1VDnOFt6wDnexfwjZ1CDYLR2ry709wajXIisBUT32F6jaL 4NtWOxMP8ocJ/uhzi/af79Hm7BunSPg= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1712655416; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=PhTMYpFqsBWH4kD+lgUO/ZuvN+fx+XWlNL7akFPD3cE=; b=3fl9w+7gjxlYJBzU5plSbhPRgjTVZ16S3zswBUNm0EBzDu0LncNouf7E5VfGXmyK9LpXiE W3gUX3+yHcasWmDA== Received: from imap1.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 imap1.dmz-prg2.suse.org (Postfix) with ESMTPS id B4382136D9; Tue, 9 Apr 2024 09:36:56 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap1.dmz-prg2.suse.org with ESMTPSA id aC9DKjgMFWZPXgAAD6G6ig (envelope-from ); Tue, 09 Apr 2024 09:36:56 +0000 Message-ID: <74723da4-ce6e-4eae-a764-c47e80f9f7bc@suse.de> Date: Tue, 9 Apr 2024 11:37:12 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 1/2] [gdb/symtab] Fix an out of bounds array access in find_epilogue_using_linetable To: Bernd Edlinger , "gdb-patches@sourceware.org" References: <20240405151012.14763-1-tdevries@suse.de> <544ecc72-916f-436e-b4f2-093bea6882a9@suse.de> <1c28bfe4-8841-4363-8460-d2d3bd5e529d@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]; NEURAL_HAM_SHORT(-0.20)[-1.000]; R_DKIM_ALLOW(-0.20)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; MIME_GOOD(-0.10)[text/plain]; MX_GOOD(-0.01)[]; XM_UA_NO_VERSION(0.01)[]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; RCPT_COUNT_TWO(0.00)[2]; FREEMAIL_TO(0.00)[hotmail.de,sourceware.org]; FUZZY_BLOCKED(0.00)[rspamd.com]; ARC_NA(0.00)[]; TO_DN_SOME(0.00)[]; RBL_SPAMHAUS_BLOCKED_OPENRESOLVER(0.00)[2a07:de40:b281:104:10:150:64:97:from]; MIME_TRACE(0.00)[0:+]; TO_DN_EQ_ADDR_SOME(0.00)[]; FREEMAIL_ENVRCPT(0.00)[hotmail.de]; TO_MATCH_ENVRCPT_ALL(0.00)[]; RCVD_TLS_ALL(0.00)[]; RCVD_COUNT_TWO(0.00)[2]; FROM_EQ_ENVFROM(0.00)[]; FROM_HAS_DN(0.00)[]; SPAMHAUS_XBL(0.00)[2a07:de40:b281:104:10:150:64:97:from]; MID_RHS_MATCH_FROM(0.00)[]; RECEIVED_SPAMHAUS_BLOCKED_OPENRESOLVER(0.00)[2a07:de40:b281:106:10:150:64:167:received]; DWL_DNSWL_BLOCKED(0.00)[suse.de:dkim]; DKIM_TRACE(0.00)[suse.de:+]; RCVD_VIA_SMTP_AUTH(0.00)[]; DBL_BLOCKED_OPENRESOLVER(0.00)[gnu.org:url,imap1.dmz-prg2.suse.org:helo,imap1.dmz-prg2.suse.org:rdns] X-Rspamd-Action: no action X-Rspamd-Queue-Id: CC8E9338F8 X-Rspamd-Server: rspamd1.dmz-prg2.suse.org X-Spam-Score: -4.50 X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,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/8/24 16:41, Bernd Edlinger wrote: > Hi Tom, > > we are making good progress. > Agreed :) > On 4/8/24 14:58, Tom de Vries wrote: >> On 4/7/24 10:17, Bernd Edlinger wrote: >>> >>> Hmm, this can be an assertion, because >>> the line table was found by find_pc_line (start_pc, 0); >>> so the linetable is guaranteed to be non-empty. >>> BTW: empty linetables are usually NULL pointers, >>> so that probably the assertion should >>> also assert like >>> gdb_assert (linetable != NULL && linetable->nitems > 0); >>> >> >> I've updated the if condition to include the nullptr check, but didn't turn it into an assert.  By doing so we gain nothing, and add a potential user inconvenience. >> > > ok. > >>>> >>>> We could also just simply handle the cornercase by returning {}, I went forth and back a bit on that, and decided to support it on the basis that at least currently we have dwarf assembly test-cases in the testsuite that trigger this path, though I've submitted a series to clean that up. >>>> >>>> But I'm still on the fence about this, if you prefer a "return {}" I'm fine with that. >>>> >> >> I've thought about this a bit more over the weekend, and I managed to convince myself that a simple "return {}" is the proper solution.  The rationale is that an incorrectly written dwarf assembly test-case is not a good reason to support a corner-case.  If we do stumble one day into a compiler that generates the incorrect debug info, we can always opt to add a workaround for this (which we then can test extensively).  But adding a workaround without such an incentive is pointless. >> >>>>>> +    } >>>>>> +      else >>>>>> +    gdb_assert (unrel_end <= it->unrelocated_pc ()); >>>>> >>>>> Why do you not check that 'it' points to an end_sequence >>>>> at exactly unrel_end? >>>>> It could be anything at an address much higher PC than unrel_end. >>>> >>>> This assert spells out the post-condition of the call to std::lower_bound, in case it found an entry. >>>> >>>> If there's debug info where one line entry straddles two functions, the call returns the entry after it, which doesn't have address unrel_end. >>>> >>>> Having said that, we can unsupport such a scenario by doing: >>>> ... >>>>        else >>>>          { >>>>            if (unrel_end < it->unrelocated_pc ()) >>>>              return {}; >>>>            gdb_assert (unrel_end == it->unrelocated_pc ()); >>>> ... >>>> >>> >>> I think we should not look at the `it` in any case. >>> If there is an inconsistency in the debug info, a >>> debug message that can be enabled in maintainer mode >>> would be good enough. >>> But even in this case, I would prefer a best effort, >>> and continue whenever possible. >>> >> >> IMO, a best effort only makes sense in case there's a compiler release generating the incorrect debug info. >> > > Consider this counter example: > The debug info is correct, but the internal representation > is not what you probably expect. > > $ cat hello.c > #include > int main() > { > printf("hello "); > #include "world.inc" > /*** End of hello.c ***/ > > $ cat world.inc > printf("world\n"); > return 0; > } > /*** End of world.inc ***/ > > gcc -g -o hello hello.c > > The line table starting at main ends between > the two printf statements. > > I demonstrate the effect this little mod over > your v4 patch version: > > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c > index 0bb7a24cbd0..35c1fb85409 100644 > --- a/gdb/amd64-tdep.c > +++ b/gdb/amd64-tdep.c > @@ -2908,6 +2908,7 @@ amd64_epilogue_frame_sniffer_1 (const struct frame_unwind *self, > /* We're not in the inner frame, so assume we're not in an epilogue. */ > return 0; > > +#if 0 > bool unwind_valid_p > = compunit_epilogue_unwind_valid (find_pc_compunit_symtab (pc)); > if (override_p) > @@ -2924,6 +2925,7 @@ amd64_epilogue_frame_sniffer_1 (const struct frame_unwind *self, > "amd64 epilogue". */ > return 0; > } > +#endif > > /* Check whether we're in an epilogue. */ > return amd64_stack_frame_destroyed_p_1 (gdbarch, pc); > diff --git a/gdb/symtab.c b/gdb/symtab.c > index 427d7b9f8b2..bb3c1bba70b 100644 > --- a/gdb/symtab.c > +++ b/gdb/symtab.c > @@ -4192,6 +4192,7 @@ find_epilogue_using_linetable (CORE_ADDR func_addr) > extent of the function, which shouldn't happen with > compiler-generated debug info. Handle the corner case > conservatively. */ > + printf("at linetable end nitems=%d\n", linetable->nitems); > return {}; > } > else > @@ -4202,6 +4203,7 @@ find_epilogue_using_linetable (CORE_ADDR func_addr) > function. This can happen if the previous entry straddles > two functions, which shouldn't happen with compiler-generated > debug info. Handle the corner case conservatively. */ > + printf("unrel_end = %lx < it->unrelocated_pc = %lx\n", (long)unrel_end, (long)it->unrelocated_pc()); > return {}; > } > gdb_assert (unrel_end == it->unrelocated_pc ()); > Nice. I've filed this as a PR ( https://sourceware.org/bugzilla/show_bug.cgi?id=31622 ) since it's an orthogonal issue, and added your suggested fix with a test-case in a separate patch in v5. > So this debug info which is not invalid at all, > triggers your error conditions: > IIUC, one of them. Anyway, I've updated the comments to reflect this situation, and also added a comment that was part of the commit message of the initial commit adding find_epilogue_using_linetable. V5 submitted here ( https://sourceware.org/pipermail/gdb-patches/2024-April/207959.html ). Thanks, - Tom > $ ..../gdb-build/gdb/gdb hello > GNU gdb (GDB) 15.0.50.20240406-git > Copyright (C) 2024 Free Software Foundation, Inc. > License GPLv3+: GNU GPL version 3 or later > This is free software: you are free to change and redistribute it. > There is NO WARRANTY, to the extent permitted by law. > Type "show copying" and "show warranty" for details. > This GDB was configured as "x86_64-pc-linux-gnu". > Type "show configuration" for configuration details. > For bug reporting instructions, please see: > . > Find the GDB manual and other documentation resources online at: > . > > For help, type "help". > Type "apropos word" to search for commands related to "word"... > Reading symbols from hello... > (gdb) b main > Breakpoint 1 at 0x114d: file hello.c, line 4. > (gdb) r > Starting program: /home/.../hello > [Thread debugging using libthread_db enabled] > Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". > at linetable end nitems=3 > > Breakpoint 1, main () at hello.c:4 > 4 printf("hello "); > (gdb) n > at linetable end nitems=3 > at linetable end nitems=3 > 1 printf("world\n"); > (gdb) n > hello world > at linetable end nitems=3 > at linetable end nitems=3 > 2 return 0; > (gdb) maint info line-table > objfile: .../hello ((struct objfile *) 0x5641508bf6b0) > compunit_symtab: hello.c ((struct compunit_symtab *) 0x5641508bfbc0) > symtab: .../hello.c ((struct symtab *) 0x5641508bfc40) > linetable: ((struct linetable *) 0x564150920ce0): > INDEX LINE REL-ADDRESS UNREL-ADDRESS IS-STMT PROLOGUE-END EPILOGUE-BEGIN > 0 3 0x0000555555555149 0x0000000000001149 Y > 1 4 0x000055555555514d 0x000000000000114d Y > 2 END 0x0000555555555161 0x0000000000001161 Y > > objfile: .../hello ((struct objfile *) 0x5641508bf6b0) > compunit_symtab: hello.c ((struct compunit_symtab *) 0x5641508bfbc0) > symtab: .../world.inc ((struct symtab *) 0x5641508bfc80) > linetable: ((struct linetable *) 0x564150920c80): > INDEX LINE REL-ADDRESS UNREL-ADDRESS IS-STMT PROLOGUE-END EPILOGUE-BEGIN > 0 1 0x0000555555555161 0x0000000000001161 Y > 1 2 0x0000555555555170 0x0000000000001170 Y > 2 3 0x0000555555555175 0x0000000000001175 Y > 3 END 0x0000555555555177 0x0000000000001177 Y > > objfile: .../hello ((struct objfile *) 0x5641508bf6b0) > compunit_symtab: hello.c ((struct compunit_symtab *) 0x5641508bfbc0) > symtab: /usr/include/stdio.h ((struct symtab *) 0x5641508bfcc0) > linetable: ((struct linetable *) 0x0): > No line table. > > > And this is also why I would suggest to look into using > the line-table that has addresses near the end of the range > like this: > > --- a/gdb/symtab.c > +++ b/gdb/symtab.c > @@ -4156,7 +4156,7 @@ find_epilogue_using_linetable (CORE_ADDR func_addr) > if (!find_pc_partial_function (func_addr, nullptr, &start_pc, &end_pc)) > return {}; > > - const struct symtab_and_line sal = find_pc_line (start_pc, 0); > + const struct symtab_and_line sal = find_pc_line (end_pc - 1, 0); > if (sal.symtab != nullptr && sal.symtab->language () != language_asm) > { > struct objfile *objfile = sal.symtab->compunit ()->objfile (); > > > > Thanks > Bernd.