[binutils-gdb] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab

[binutils-gdb] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=336125713fcf9b43960a57724fa39a319036ba38

commit 336125713fcf9b43960a57724fa39a319036ba38
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sat Feb 19 13:09:34 2022 +0000

    gdb: avoid nullptr access in dbxread.c from read_dbx_symtab
    
    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

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