[v2] Avoid infinite recursion in find_pc_sect_line

Message ID 20191204215439.1748221-1-kevinb@redhat.com
State New, archived
Headers

Commit Message

Kevin Buettner Dec. 4, 2019, 9:54 p.m. UTC
  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(-)
  

Comments

Kevin Buettner Jan. 6, 2020, 3:55 p.m. UTC | #1
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
>
  
Kevin Buettner Jan. 31, 2020, 12:32 p.m. UTC | #2
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
>
  
Kevin Buettner Feb. 13, 2020, 1:01 p.m. UTC | #3
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
>
  
Kevin Buettner March 1, 2020, 2:27 a.m. UTC | #4
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
>
  
Simon Marchi March 3, 2020, 5:23 a.m. UTC | #5
On 2019-12-04 4:54 p.m., Kevin Buettner 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
> 

Hi Kevin,

Sorry for taking time to get back to this.  This is fine with me.  Hopefully, we don't
ever hear from this again, but if it happens, then we have a chance to catch it.

Simon
  

Patch

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;