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

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

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

LGTM.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>
> PR symtab/31268
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31268
> ---
>  gdb/symtab.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 84 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 86603dfebc3..e032178aaa6 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4156,6 +4156,9 @@ find_epilogue_using_linetable (CORE_ADDR func_addr)
>    if (!find_pc_partial_function (func_addr, nullptr, &start_pc, &end_pc))
>      return {};
>  
> +  /* While the standard allows for multiple points marked with epilogue_begin
> +     in the same function, for performance reasons, this function will only
> +     find the last address that sets this flag for a given block.  */
>    const struct symtab_and_line sal = find_pc_line (start_pc, 0);
>    if (sal.symtab != nullptr && sal.symtab->language () != language_asm)
>      {
> @@ -4166,24 +4169,95 @@ 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 == nullptr || 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 &lte, unrelocated_addr pc)
>  	 {
>  	   return lte.unrelocated_pc () < pc;
>  	 });
> +      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.  This can be triggered with:
> +	     - compiler-generated debug info, in the cornercase that the pc
> +	       with which we call find_pc_line resides in a different file
> +	       than unrel_end, or
> +	     - invalid dwarf assembly debug info.
> +	     In the former case, there's no point in iterating further, simply
> +	     return "not found".  In the latter case, there's no current
> +	     incentive to attempt to support this, so handle this
> +	     conservatively and do the same.  */
> +	  return {};
> +	}
>  
> -      while (it->unrelocated_pc () >= unrel_start)
> -      {
> -	if (it->epilogue_begin)
> -	  return {it->pc (objfile)};
> -	it --;
> -      }
> +      if (unrel_end < it->unrelocated_pc ())
> +	{
> +	  /* We found a line entry that starts past the end of the
> +	     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.  */
> +	  return {};
> +	}
> +      gdb_assert (unrel_end == it->unrelocated_pc ());
> +
> +      /* Move to the last linetable entry of the current function.  */
> +      if (it == &linetable->item[0])
> +	{
> +	  /* Doing it-- would introduce undefined behaviour, avoid it by
> +	     explicitly handling this case.  */
> +	  return {};
> +	}
> +      it--;
> +      if (it->unrelocated_pc () < unrel_start)
> +	{
> +	  /* Not in the current function.  */
> +	  return {};
> +	}
> +      gdb_assert (it->unrelocated_pc () < unrel_end);
> +
> +      /* We're at the the last linetable entry of the current function.  This
> +	 is probably where the epilogue begins, but since the DWARF 5 spec
> +	 doesn't guarantee it, we iterate backwards through the current
> +	 function until we either find the epilogue beginning, or are sure
> +	 that it doesn't exist.  */
> +      for (; it >= &linetable->item[0]; it--)
> +	{
> +	  if (it->unrelocated_pc () < unrel_start)
> +	    {
> +	      /* No longer in the current function.  */
> +	      break;
> +	    }
> +
> +	  if (it->epilogue_begin)
> +	    {
> +	      /* Found the beginning of the epilogue.  */
> +	      return {it->pc (objfile)};
> +	    }
> +
> +	  if (it == &linetable->item[0])
> +	    {
> +	      /* No more entries in the current function.
> +		 Doing it-- would introduce undefined behaviour, avoid it by
> +		 explicitly handling this case.  */
> +	      break;
> +	    }
> +	}
>      }
> +
>    return {};
>  }
>  
>
> base-commit: 9132c8152b899a1683bc886f8ba76bedadb48aa1
> -- 
> 2.35.3