From: Tom de Vries <tdevries@suse.de>
To: Bernd Edlinger <bernd.edlinger@hotmail.de>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [PATCH v3 1/2] [gdb/symtab] Fix an out of bounds array access in find_epilogue_using_linetable
Date: Sat, 6 Apr 2024 10:29:41 +0200 [thread overview]
Message-ID: <544ecc72-916f-436e-b4f2-093bea6882a9@suse.de> (raw)
In-Reply-To: <a33ed623-5814-4ac1-a89b-27ac8c61beb4@hotmail.de>
On 4/6/24 07:03, Bernd Edlinger wrote:
> On 4/5/24 17:10, Tom de Vries wrote:
>> From: Bernd Edlinger <bernd.edlinger@hotmail.de>
>>
>> An out of bounds array access in find_epilogue_using_linetable causes random
>> test failures like these:
>>
>> 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.
>>
>> It also happens 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
>>
>> Fixes: 528b729be1a2 ("gdb/dwarf2: Add support for DW_LNS_set_epilogue_begin in line-table")
>>
>> Co-Authored-By: Tom de Vries <tdevries@suse.de>
>>
>> PR symtab/31268
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31268
>> ---
>> gdb/symtab.c | 85 +++++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 75 insertions(+), 10 deletions(-)
>>
>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index 86603dfebc3..0c126d99cd4 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -4166,10 +4166,14 @@ find_epilogue_using_linetable (CORE_ADDR func_addr)
>> = unrelocated_addr (end_pc - objfile->text_section_offset ());
>>
>> const linetable *linetable = sal.symtab->linetable ();
>> - /* This should find the last linetable entry of the current function.
>> - It is probably where the epilogue begins, but since the DWARF 5
>> - spec doesn't guarantee it, we iterate backwards through the function
>> - until we either find it or are sure that it doesn't exist. */
>> + if (linetable->nitems == 0)
>> + {
>> + /* Empty line table. */
>> + return {};
>> + }
>> +
>> + /* Find the first linetable entry after the current function. Note that
>> + this also may be an end_sequence entry. */
>> auto it = std::lower_bound
>> (linetable->item, linetable->item + linetable->nitems, unrel_end,
>> [] (const linetable_entry <e, unrelocated_addr pc)
>> @@ -4177,13 +4181,74 @@ find_epilogue_using_linetable (CORE_ADDR func_addr)
>> return lte.unrelocated_pc () < pc;
>> });
>>
>> - while (it->unrelocated_pc () >= unrel_start)
>> - {
>> - if (it->epilogue_begin)
>> - return {it->pc (objfile)};
>> - it --;
>> - }
>> + if (it == linetable->item + linetable->nitems)
>> + {
>> + /* We couldn't find either:
>> + - a linetable entry starting the function after the current
>> + function, or
>> + - an end_sequence entry that terminates the current function
>> + at unrel_end.
>> + This can happen when the linetable doesn't describe the full
>> + extent of the function. Even though this is a corner case, which
>> + may not happen other than in dwarf assembly test-cases, let's
>> + handle this.
>> +
>> + Move to the last entry in the linetable, and check that it's an
>> + end_sequence terminating the current function. */
>> + gdb_assert (it != &linetable->item[0]);
>> + it--;
>> + if (!(it->line == 0
>> + && unrel_start <= it->unrelocated_pc ()
>> + && it->unrelocated_pc () < unrel_end))
>> + return {};
>
> Why is this check necessary here, and not also when
> this is not the last function in the line-table?
>
> And why is this check necessary at all?
>
It spells out as much as possible the specific conditions of the
corner-case we're handling.
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.
>> + }
>> + 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 ());
...
Thanks,
- Tom
next prev parent reply other threads:[~2024-04-06 8:29 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-05 15:10 Tom de Vries
2024-04-05 15:10 ` [PATCH v3 2/2] [gdb/testsuite] Add gdb.dwarf2/dw2-epilogue-begin-2.exp Tom de Vries
2024-04-06 5:03 ` [PATCH v3 1/2] [gdb/symtab] Fix an out of bounds array access in find_epilogue_using_linetable Bernd Edlinger
2024-04-06 8:29 ` Tom de Vries [this message]
2024-04-07 8:17 ` Bernd Edlinger
2024-04-08 12:58 ` Tom de Vries
2024-04-08 14:41 ` Bernd Edlinger
2024-04-09 9:37 ` Tom de Vries
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=544ecc72-916f-436e-b4f2-093bea6882a9@suse.de \
--to=tdevries@suse.de \
--cc=bernd.edlinger@hotmail.de \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).