Re: [PATCH v2] Avoid infinite recursion in find_pc_sect_line

[Kevin Buettner <kevinb at redhat dot com>]

Ping.

On Wed,  4 Dec 2019 14:54:39 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> A patch somewhat like this patch has been in Fedora GDB for well over
> a decade.  The Fedora patch was written by Jan Kratochvil.  The Fedora
> version prints a warning and attempts to continue.  This version will
> error out, fatally.  An earlier version of this patch was more like
> the Fedora version than this one.  Simon Marchi recommended use of an
> assertion to test for the infinite recursion; I decided to use an
> explicit test (with an "if" statement) along with a call to
> internal_error() if the condition is met.  This way, I could include
> a plea to file a bug report.
> 
> It was motivated by a customer reported bug (back in 2006!) which
> showed infinite mutual recursion between find_pc_sect_line and
> find_pc_line.  Here is a portion of the backtrace from the bug report:
> 
>     (gdb) bt
>     #0  0x00000000004450a4 in lookup_minimal_symbol_by_pc_section (
> 	pc=251700325328, section=0x570f500) at gdb/minsyms.c:484
>     #1  0x00000000004bbfb2 in find_pc_sect_line (pc=251700325328,
> 	section=0x570f500, notcurrent=0) at gdb/symtab.c:2057
>     #2  0x00000000004bc480 in find_pc_line (pc=251700325328, notcurrent=0)
> 	at gdb/symtab.c:2232
>     #3  0x00000000004bc1ff in find_pc_sect_line (pc=251700325328,
> 	section=0x570f500, notcurrent=0) at gdb/symtab.c:2081
> 
>     ...   (lots and lots of the same two functions with the same parameters)
> 
>     #1070 0x00000000004bc480 in find_pc_line (pc=251700325328, notcurrent=0)
> 	at gdb/symtab.c:2232
>     #1071 0x00000000004bc1ff in find_pc_sect_line (pc=251700325328,
> 	section=0x570f500, notcurrent=0) at gdb/symtab.c:2081
>     #1072 0x00000000004bc480 in find_pc_line (pc=251700325328, notcurrent=0)
> 	at gdb/symtab.c:2232
>     #1073 0x00000000004bc1ff in find_pc_sect_line (pc=251700325328,
> 	section=0x570f500, notcurrent=0) at gdb/symtab.c:2081
>     #1074 0x00000000004bc480 in find_pc_line (pc=251700325328, notcurrent=0)
> 	at gdb/symtab.c:2232
>     #1075 0x00000000004bc1ff in find_pc_sect_line (pc=251696794399,
> 	section=0x59b0df8, notcurrent=0) at gdb/symtab.c:2081
>     #1076 0x00000000004bc480 in find_pc_line (pc=251696794399, notcurrent=0)
> 	at gdb/symtab.c:2232
>     #1077 0x000000000055550e in find_frame_sal (frame=0xb3f3e0, sal=0x7fff1d1a8200)
> 	at gdb/frame.c:1392
>     #1078 0x00000000004d86fd in set_current_sal_from_frame (frame=0x1648, center=1)
> 	at gdb/stack.c:379
>     #1079 0x00000000004cf137 in normal_stop () at gdb/infrun.c:3147
>     ...
> 
> The test case was a large application.  Attempts were made to make a
> small(er) test case, but those attempts were not successful.
> Therefore, I cannot provide a new test for this patch.
> 
> That said, we ought to guard against recursively calling
> find_pc_sect_line (via find_pc_line) with the identical PC value that
> it had been called with.  Should this happen, infinite recursion (as
> shown in the above backtrace) is the result.  This patch prevents
> that from happening.
> 
> If this should happens, there is a bug somewhere, perhaps in GDB, perhaps
> in some other part of the toolchain or a library.  We error out fatally
> with a message briefly describing the condition along with a plea to file
> a bug report.
> 
> I spent some time looking at the surrounding code and commentary which
> handle the case of PC being in a stub/trampoline.  It first appeared
> in the public GDB repository in April, 1999.  The ChangeLog entry for
> this commit is from 1998-12-31.  The relevant portion is:
> 
> 	(find_pc_sect_line): Return correct information if pc is in import
> 	or export stub (trampoline).
> 
> What's remarkable about the overall ChangeLog entry is that it's over
> 2500+ lines long!  I believe that this was part of the infamous "HP
> merge" (in which insufficient due diligence was given in accepting
> a large batch of changes from an outside source).  In the years that
> followed, much of this code was either significantly revised or
> outright removed.
> 
> For this particular case, I'm grateful that extensive comments were
> provided by "RT".  (I haven't been able to figure out who RT is/was.)
> I've decided against attempting to revise this stub/trampoline handling
> code any further than adding Jan's test which prevents an obvious case
> of infinite recursion.
> 
> I've tested on Fedora 31, x86-64.  I see no regressions.  I've also
> searched the logfile for the new message, but as expected, no message
> was found (which is good).
> 
> gdb/ChangeLog:
> 
> 	* symtab.c (find_pc_sect_line): Add check which prevents infinite
> 	recursion.
> 
> Change-Id: I595470be6ab5f61ca7e4e9e70c61a252c0deaeaa
> ---
>  gdb/symtab.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 5c33fbf9ab..95020d843c 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -3167,7 +3167,17 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent)
>  	  ;
>  	/* fall through */
>  	else
> -	  return find_pc_line (BMSYMBOL_VALUE_ADDRESS (mfunsym), 0);
> +	  {
> +	    /* Detect an obvious case of infinite recursion.  If this
> +	       should occur, we'd like to know about it, so error out,
> +	       fatally.  */
> +	    if (BMSYMBOL_VALUE_ADDRESS (mfunsym) == pc)
> +	      internal_error (__FILE__, __LINE__,
> +	        _("Infinite recursion detected in find_pc_sect_line;"
> +		  "please file a bug report"));
> +
> +	    return find_pc_line (BMSYMBOL_VALUE_ADDRESS (mfunsym), 0);
> +	  }
>        }
>  
>    symtab_and_line val;
> -- 
> 2.23.0
>