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

[Tom de Vries <tdevries@suse.de>]

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 <stdio.h>
> 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 <http://gnu.org/licenses/gpl.html>
> 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:
> <https://www.gnu.org/software/gdb/bugs/>.
> Find the GDB manual and other documentation resources online at:
>      <http://www.gnu.org/software/gdb/documentation/>.
> 
> 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.