Re: [PATCH v3 1/2] [gdb/symtab] Fix an out of bounds array access in find_epilogue_using_linetable

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 4/6/24 10:29, Tom de Vries wrote:
> On 4/6/24 07:03, Bernd Edlinger wrote:
>> On 4/5/24 17:10, Tom de Vries wrote:
>>>
>>> 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 {};
>>> +    }
>>> +

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);

>>> +      /* 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 &lte, 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 ());
> ...
> 

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.

If you look at skip_prologue_using_linetable
you see that it does stop the search immediately, when
it->unrelocated_pc() reaches unrel_end, or when the
end of the linetable is reached:

      for (;
           (it < linetable->item + linetable->nitems
            && it->unrelocated_pc () < unrel_end);
           it++)
        if (it->prologue_end)
          return {it->pc (objfile)};

Therefore I would like find_epilogue_using_linetable
to use the same algorithm just in reverse direction.
Which means always do `it--` first before using `it`.

After all this is just a partial function range,
it can end with a jump or a return, and in both
cases the linetable entry at unrel_end can belong
to a completely different function, and it is not
guaranteed to be an end_sequence entry.

BTW: I am not sure what happens if the function
has multiple line tables, e.g. because of inline
functions, or #include to pull in parts of the function
body.  In that case I would expect that the line
table found by find_pc_line (start_pc, 0);
may be covering the prologue area, while the epilogue
may be missing.
Maybe find_pc_line (end_pc - 1, 0); would be better
candidate for a line table covering the epilogue area?


Thanks.
Bernd.

> Thanks,
> - Tom