[PATCH 0/2] fix for the stabs debug parser

[PATCH 0/2] fix for the stabs debug parser

[Andrew Burgess]

Fixes bug:

  https://sourceware.org/bugzilla/show_bug.cgi?id=28900

Andrew

Andrew Burgess (2):
  gdb: make use of std::string in dbxread.c
  gdb: avoid nullptr access in dbxread.c from read_dbx_symtab

 gdb/dbxread.c | 155 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 92 insertions(+), 63 deletions(-)

-- 
2.25.4

[PATCH 1/2] gdb: make use of std::string in dbxread.c

[Andrew Burgess]

While taking a look through dbxread.c I spotted a couple of places
where making use of std::string would remove the need for manual
memory allocation and memcpy.

There should be no user visible changes after this commit.
---
 gdb/dbxread.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index 911d518b7e9..ddda5df4ee1 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -1603,13 +1603,8 @@ read_dbx_symtab (minimal_symbol_reader &reader,
 	    case 'f':
 	      if (! pst)
 		{
-		  int name_len = p - namestring;
-		  char *name = (char *) xmalloc (name_len + 1);
-
-		  memcpy (name, namestring, name_len);
-		  name[name_len] = '\0';
-		  function_outside_compilation_unit_complaint (name);
-		  xfree (name);
+		  std::string name (namestring, (p - namestring));
+		  function_outside_compilation_unit_complaint (name.c_str ());
 		}
 	      /* Kludges for ELF/STABS with Sun ACC.  */
 	      last_function_name = namestring;
@@ -1663,13 +1658,8 @@ read_dbx_symtab (minimal_symbol_reader &reader,
 	    case 'F':
 	      if (! pst)
 		{
-		  int name_len = p - namestring;
-		  char *name = (char *) xmalloc (name_len + 1);
-
-		  memcpy (name, namestring, name_len);
-		  name[name_len] = '\0';
-		  function_outside_compilation_unit_complaint (name);
-		  xfree (name);
+		  std::string name (namestring, (p - namestring));
+		  function_outside_compilation_unit_complaint (name.c_str ());
 		}
 	      /* Kludges for ELF/STABS with Sun ACC.  */
 	      last_function_name = namestring;
-- 
2.25.4

Re: [PATCH 1/2] gdb: make use of std::string in dbxread.c

[Simon Marchi]

On 2022-02-19 14:51, Andrew Burgess via Gdb-patches wrote:
> While taking a look through dbxread.c I spotted a couple of places
> where making use of std::string would remove the need for manual
> memory allocation and memcpy.
> 
> There should be no user visible changes after this commit.

LGTM.

It's not really related, but there is the exact same code that could be
changed in the exact same way in xcoffread.c, it would be nice to change
it as well.

Simon

Re: [PATCH 1/2] gdb: make use of std::string in dbxread.c

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> On 2022-02-19 14:51, Andrew Burgess via Gdb-patches wrote:
>> While taking a look through dbxread.c I spotted a couple of places
>> where making use of std::string would remove the need for manual
>> memory allocation and memcpy.
>> 
>> There should be no user visible changes after this commit.
>
> LGTM.
>
> It's not really related, but there is the exact same code that could be
> changed in the exact same way in xcoffread.c, it would be nice to change
> it as well.

Thanks, I pushed the patch below that also fixes xcoffread.c.

Andrew

---

commit 9c6c44713f31f7b27bfe6921de378fa69127a048
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sat Feb 19 13:08:32 2022 +0000

    gdb: make use of std::string in dbxread.c and xcoffread.c
    
    While taking a look through dbxread.c I spotted a couple of places
    where making use of std::string would remove the need for manual
    memory allocation and memcpy.
    
    During review Simon pointed out that the same code exists in
    xcoffread.c, so I've applied the same fix there too.
    
    There should be no user visible changes after this commit.

diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index 911d518b7e9..ddda5df4ee1 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -1603,13 +1603,8 @@ read_dbx_symtab (minimal_symbol_reader &reader,
 	    case 'f':
 	      if (! pst)
 		{
-		  int name_len = p - namestring;
-		  char *name = (char *) xmalloc (name_len + 1);
-
-		  memcpy (name, namestring, name_len);
-		  name[name_len] = '\0';
-		  function_outside_compilation_unit_complaint (name);
-		  xfree (name);
+		  std::string name (namestring, (p - namestring));
+		  function_outside_compilation_unit_complaint (name.c_str ());
 		}
 	      /* Kludges for ELF/STABS with Sun ACC.  */
 	      last_function_name = namestring;
@@ -1663,13 +1658,8 @@ read_dbx_symtab (minimal_symbol_reader &reader,
 	    case 'F':
 	      if (! pst)
 		{
-		  int name_len = p - namestring;
-		  char *name = (char *) xmalloc (name_len + 1);
-
-		  memcpy (name, namestring, name_len);
-		  name[name_len] = '\0';
-		  function_outside_compilation_unit_complaint (name);
-		  xfree (name);
+		  std::string name (namestring, (p - namestring));
+		  function_outside_compilation_unit_complaint (name.c_str ());
 		}
 	      /* Kludges for ELF/STABS with Sun ACC.  */
 	      last_function_name = namestring;
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index d3e9ade72cb..f6db7f9ff7e 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -2734,13 +2734,8 @@ scan_xcoff_symtab (minimal_symbol_reader &reader,
 	      case 'f':
 		if (! pst)
 		  {
-		    int name_len = p - namestring;
-		    char *name = (char *) xmalloc (name_len + 1);
-
-		    memcpy (name, namestring, name_len);
-		    name[name_len] = '\0';
-		    function_outside_compilation_unit_complaint (name);
-		    xfree (name);
+		    std::string name (namestring, (p - namestring));
+		    function_outside_compilation_unit_complaint (name.c_str ());
 		  }
 		pst->add_psymbol (gdb::string_view (namestring,
 						    p - namestring),
@@ -2758,13 +2753,8 @@ scan_xcoff_symtab (minimal_symbol_reader &reader,
 	      case 'F':
 		if (! pst)
 		  {
-		    int name_len = p - namestring;
-		    char *name = (char *) xmalloc (name_len + 1);
-
-		    memcpy (name, namestring, name_len);
-		    name[name_len] = '\0';
-		    function_outside_compilation_unit_complaint (name);
-		    xfree (name);
+		    std::string name (namestring, (p - namestring));
+		    function_outside_compilation_unit_complaint (name.c_str ());
 		  }
 
 		/* We need only the minimal symbols for these

[PATCH 2/2] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab

[Andrew Burgess]

This fixes a GDB crash reported in bug pr/28900, related to reading in
some stabs debug information.

In this commit my goal is to stop GDB crashing.  I am not trying to
ensure that GDB makes the best possible use of the available stabs
debug information.  At this point I consider stabs a legacy debug
format, with only limited support in GDB.

So, the problem appears to be that, when reading in the stabs data, we
need to find a N_SO entry, this is the entry that defines the start of
a compilation unit (or at least the location of a corresponding source
file).

It is while handling an N_SO that GDB creates a psymtab to hold the
incoming debug information (symbols, etc).

The problem we hit in the bug is that we encounter some symbol
information (an N_PC entry) outside of an N_SO entry - that is we find
some symbol information that is not associated with any source file.

We already have some protection for this case, look (in
read_dbx_symtab) at the handling of N_PC entries of type 'F' and 'f',
if we have no psymtab (the pst variable is nullptr) then we issue a
complaint.  However, for whatever reason, in both 'f' and 'F'
handling, there is one place where we assume that the pst
variable (the psymtab) is not nullptr.  This is a mistake.

In this commit, I guard these two locations (in 'f' and 'F' handling)
so we no longer assume pst is not nullptr.

While I was at it, I audited all the other uses of pst in
read_dbx_symtab, and in every potentially dangerous case I added a
nullptr check, and issue a suitable complaint if pst is found to be
nullptr.

It might well be true that we could/should do something smarter if we
see a debug symbol outside of an N_SO entry, and if anyone wanted to
do that work, they're welcome too.  But this commit is just about
preventing the nullptr access, and the subsequent GDB crash.

I don't have any tests for this change, I have no idea how to generate
weird stabs data for testing.  The original binary from the bug report
now loads just fine without GDB crashing.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28900
---
 gdb/dbxread.c | 137 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 88 insertions(+), 49 deletions(-)

diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index ddda5df4ee1..165040dd7ce 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -1461,23 +1461,33 @@ read_dbx_symtab (minimal_symbol_reader &reader,
 	  switch (p[1])
 	    {
 	    case 'S':
-	      pst->add_psymbol (gdb::string_view (sym_name, sym_len), true,
-				VAR_DOMAIN, LOC_STATIC,
-				data_sect_index,
-				psymbol_placement::STATIC,
-				nlist.n_value, psymtab_language,
-				partial_symtabs, objfile);
+	      if (pst != nullptr)
+		pst->add_psymbol (gdb::string_view (sym_name, sym_len), true,
+				  VAR_DOMAIN, LOC_STATIC,
+				  data_sect_index,
+				  psymbol_placement::STATIC,
+				  nlist.n_value, psymtab_language,
+				  partial_symtabs, objfile);
+	      else
+		complaint (_("static `%*s' appears to be defined "
+			     "outside of all compilation units"),
+			   sym_len, sym_name);
 	      continue;
 
 	    case 'G':
 	      /* The addresses in these entries are reported to be
 		 wrong.  See the code that reads 'G's for symtabs.  */
-	      pst->add_psymbol (gdb::string_view (sym_name, sym_len), true,
-				VAR_DOMAIN, LOC_STATIC,
-				data_sect_index,
-				psymbol_placement::GLOBAL,
-				nlist.n_value, psymtab_language,
-				partial_symtabs, objfile);
+	      if (pst != nullptr)
+		pst->add_psymbol (gdb::string_view (sym_name, sym_len), true,
+				  VAR_DOMAIN, LOC_STATIC,
+				  data_sect_index,
+				  psymbol_placement::GLOBAL,
+				  nlist.n_value, psymtab_language,
+				  partial_symtabs, objfile);
+	      else
+		complaint (_("global `%*s' appears to be defined "
+			     "outside of all compilation units"),
+			   sym_len, sym_name);
 	      continue;
 
 	    case 'T':
@@ -1491,19 +1501,30 @@ read_dbx_symtab (minimal_symbol_reader &reader,
 		  || (p == namestring + 1
 		      && namestring[0] != ' '))
 		{
-		  pst->add_psymbol (gdb::string_view (sym_name, sym_len),
-				    true, STRUCT_DOMAIN, LOC_TYPEDEF, -1,
-				    psymbol_placement::STATIC,
-				    0, psymtab_language,
-				    partial_symtabs, objfile);
+		  if (pst != nullptr)
+		    pst->add_psymbol (gdb::string_view (sym_name, sym_len),
+				      true, STRUCT_DOMAIN, LOC_TYPEDEF, -1,
+				      psymbol_placement::STATIC,
+				      0, psymtab_language,
+				      partial_symtabs, objfile);
+		  else
+		    complaint (_("enum, struct, or union `%*s' appears "
+				 "to be defined outside of all "
+				 "compilation units"),
+			       sym_len, sym_name);
 		  if (p[2] == 't')
 		    {
 		      /* Also a typedef with the same name.  */
-		      pst->add_psymbol (gdb::string_view (sym_name, sym_len),
-					true, VAR_DOMAIN, LOC_TYPEDEF, -1,
-					psymbol_placement::STATIC,
-					0, psymtab_language,
-					partial_symtabs, objfile);
+		      if (pst != nullptr)
+			pst->add_psymbol (gdb::string_view (sym_name, sym_len),
+					  true, VAR_DOMAIN, LOC_TYPEDEF, -1,
+					  psymbol_placement::STATIC,
+					  0, psymtab_language,
+					  partial_symtabs, objfile);
+		      else
+			complaint (_("typedef `%*s' appears to be defined "
+				     "outside of all compilation units"),
+				   sym_len, sym_name);
 		      p += 1;
 		    }
 		}
@@ -1512,11 +1533,16 @@ read_dbx_symtab (minimal_symbol_reader &reader,
 	    case 't':
 	      if (p != namestring)	/* a name is there, not just :T...  */
 		{
-		  pst->add_psymbol (gdb::string_view (sym_name, sym_len),
-				    true, VAR_DOMAIN, LOC_TYPEDEF, -1,
-				    psymbol_placement::STATIC,
-				    0, psymtab_language,
-				    partial_symtabs, objfile);
+		  if (pst != nullptr)
+		    pst->add_psymbol (gdb::string_view (sym_name, sym_len),
+				      true, VAR_DOMAIN, LOC_TYPEDEF, -1,
+				      psymbol_placement::STATIC,
+				      0, psymtab_language,
+				      partial_symtabs, objfile);
+		  else
+		    complaint (_("typename `%*s' appears to be defined "
+				 "outside of all compilation units"),
+			       sym_len, sym_name);
 		}
 	    check_enum:
 	      /* If this is an enumerated type, we need to
@@ -1574,11 +1600,16 @@ read_dbx_symtab (minimal_symbol_reader &reader,
 			;
 		      /* Note that the value doesn't matter for
 			 enum constants in psymtabs, just in symtabs.  */
-		      pst->add_psymbol (gdb::string_view (p, q - p), true,
-					VAR_DOMAIN, LOC_CONST, -1,
-					psymbol_placement::STATIC, 0,
-					psymtab_language,
-					partial_symtabs, objfile);
+		      if (pst != nullptr)
+			pst->add_psymbol (gdb::string_view (p, q - p), true,
+					  VAR_DOMAIN, LOC_CONST, -1,
+					  psymbol_placement::STATIC, 0,
+					  psymtab_language,
+					  partial_symtabs, objfile);
+		      else
+			complaint (_("enum constant `%*s' appears to be defined "
+				     "outside of all compilation units"),
+				   ((int) (q - p)), p);
 		      /* Point past the name.  */
 		      p = q;
 		      /* Skip over the value.  */
@@ -1593,11 +1624,17 @@ read_dbx_symtab (minimal_symbol_reader &reader,
 
 	    case 'c':
 	      /* Constant, e.g. from "const" in Pascal.  */
-	      pst->add_psymbol (gdb::string_view (sym_name, sym_len), true,
-				VAR_DOMAIN, LOC_CONST, -1,
-				psymbol_placement::STATIC, 0,
-				psymtab_language,
-				partial_symtabs, objfile);
+	      if (pst != nullptr)
+		pst->add_psymbol (gdb::string_view (sym_name, sym_len), true,
+				  VAR_DOMAIN, LOC_CONST, -1,
+				  psymbol_placement::STATIC, 0,
+				  psymtab_language,
+				  partial_symtabs, objfile);
+	      else
+		complaint (_("constant `%*s' appears to be defined "
+			     "outside of all compilation units"),
+			   sym_len, sym_name);
+
 	      continue;
 
 	    case 'f':
@@ -1644,12 +1681,13 @@ read_dbx_symtab (minimal_symbol_reader &reader,
 		  pst->set_text_low (nlist.n_value);
 		  textlow_not_set = 0;
 		}
-	      pst->add_psymbol (gdb::string_view (sym_name, sym_len), true,
-				VAR_DOMAIN, LOC_BLOCK,
-				SECT_OFF_TEXT (objfile),
-				psymbol_placement::STATIC,
-				nlist.n_value, psymtab_language,
-				partial_symtabs, objfile);
+	      if (pst != nullptr)
+		pst->add_psymbol (gdb::string_view (sym_name, sym_len), true,
+				  VAR_DOMAIN, LOC_BLOCK,
+				  SECT_OFF_TEXT (objfile),
+				  psymbol_placement::STATIC,
+				  nlist.n_value, psymtab_language,
+				  partial_symtabs, objfile);
 	      continue;
 
 	      /* Global functions were ignored here, but now they
@@ -1699,12 +1737,13 @@ read_dbx_symtab (minimal_symbol_reader &reader,
 		  pst->set_text_low (nlist.n_value);
 		  textlow_not_set = 0;
 		}
-	      pst->add_psymbol (gdb::string_view (sym_name, sym_len), true,
-				VAR_DOMAIN, LOC_BLOCK,
-				SECT_OFF_TEXT (objfile),
-				psymbol_placement::GLOBAL,
-				nlist.n_value, psymtab_language,
-				partial_symtabs, objfile);
+	      if (pst != nullptr)
+		pst->add_psymbol (gdb::string_view (sym_name, sym_len), true,
+				  VAR_DOMAIN, LOC_BLOCK,
+				  SECT_OFF_TEXT (objfile),
+				  psymbol_placement::GLOBAL,
+				  nlist.n_value, psymtab_language,
+				  partial_symtabs, objfile);
 	      continue;
 
 	      /* Two things show up here (hopefully); static symbols of
-- 
2.25.4

Re: [PATCH 2/2] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab

[Simon Marchi]

On 2022-02-19 14:51, Andrew Burgess via Gdb-patches wrote:
> This fixes a GDB crash reported in bug pr/28900, related to reading in
> some stabs debug information.
> 
> In this commit my goal is to stop GDB crashing.  I am not trying to
> ensure that GDB makes the best possible use of the available stabs
> debug information.  At this point I consider stabs a legacy debug
> format, with only limited support in GDB.
> 
> So, the problem appears to be that, when reading in the stabs data, we
> need to find a N_SO entry, this is the entry that defines the start of
> a compilation unit (or at least the location of a corresponding source
> file).
> 
> It is while handling an N_SO that GDB creates a psymtab to hold the
> incoming debug information (symbols, etc).
> 
> The problem we hit in the bug is that we encounter some symbol
> information (an N_PC entry) outside of an N_SO entry - that is we find
> some symbol information that is not associated with any source file.
> 
> We already have some protection for this case, look (in
> read_dbx_symtab) at the handling of N_PC entries of type 'F' and 'f',
> if we have no psymtab (the pst variable is nullptr) then we issue a
> complaint.  However, for whatever reason, in both 'f' and 'F'
> handling, there is one place where we assume that the pst
> variable (the psymtab) is not nullptr.  This is a mistake.
> 
> In this commit, I guard these two locations (in 'f' and 'F' handling)
> so we no longer assume pst is not nullptr.
> 
> While I was at it, I audited all the other uses of pst in
> read_dbx_symtab, and in every potentially dangerous case I added a
> nullptr check, and issue a suitable complaint if pst is found to be
> nullptr.
> 
> It might well be true that we could/should do something smarter if we
> see a debug symbol outside of an N_SO entry, and if anyone wanted to
> do that work, they're welcome too.  But this commit is just about
> preventing the nullptr access, and the subsequent GDB crash.
> 
> I don't have any tests for this change, I have no idea how to generate
> weird stabs data for testing.  The original binary from the bug report
> now loads just fine without GDB crashing.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28900

For all I know, this looks correct.  Thanks for doing this.

Simon

Re: [PATCH 2/2] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> On 2022-02-19 14:51, Andrew Burgess via Gdb-patches wrote:
>> This fixes a GDB crash reported in bug pr/28900, related to reading in
>> some stabs debug information.
>> 
>> In this commit my goal is to stop GDB crashing.  I am not trying to
>> ensure that GDB makes the best possible use of the available stabs
>> debug information.  At this point I consider stabs a legacy debug
>> format, with only limited support in GDB.
>> 
>> So, the problem appears to be that, when reading in the stabs data, we
>> need to find a N_SO entry, this is the entry that defines the start of
>> a compilation unit (or at least the location of a corresponding source
>> file).
>> 
>> It is while handling an N_SO that GDB creates a psymtab to hold the
>> incoming debug information (symbols, etc).
>> 
>> The problem we hit in the bug is that we encounter some symbol
>> information (an N_PC entry) outside of an N_SO entry - that is we find
>> some symbol information that is not associated with any source file.
>> 
>> We already have some protection for this case, look (in
>> read_dbx_symtab) at the handling of N_PC entries of type 'F' and 'f',
>> if we have no psymtab (the pst variable is nullptr) then we issue a
>> complaint.  However, for whatever reason, in both 'f' and 'F'
>> handling, there is one place where we assume that the pst
>> variable (the psymtab) is not nullptr.  This is a mistake.
>> 
>> In this commit, I guard these two locations (in 'f' and 'F' handling)
>> so we no longer assume pst is not nullptr.
>> 
>> While I was at it, I audited all the other uses of pst in
>> read_dbx_symtab, and in every potentially dangerous case I added a
>> nullptr check, and issue a suitable complaint if pst is found to be
>> nullptr.
>> 
>> It might well be true that we could/should do something smarter if we
>> see a debug symbol outside of an N_SO entry, and if anyone wanted to
>> do that work, they're welcome too.  But this commit is just about
>> preventing the nullptr access, and the subsequent GDB crash.
>> 
>> I don't have any tests for this change, I have no idea how to generate
>> weird stabs data for testing.  The original binary from the bug report
>> now loads just fine without GDB crashing.
>> 
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28900
>
> For all I know, this looks correct.  Thanks for doing this.

Thanks, I pushed this patch.

Andrew