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>]

Hi Tom,

we are making good progress.

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

So this debug info which is not invalid at all,
triggers your error conditions:

$ ..../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.