[PATCH] gdb: Handle missing .debug_str section

[PATCH] gdb: Handle missing .debug_str section

[Andrew Burgess]

While messing with the Dwarf assembler (gdb/testsuite/lib/dwarf.exp) I
managed to create an ELF which made use of DW_FORM_strp, but didn't
include a .debug_str section.

When I started GDB on this ELF, GDB crashed.  I would have expected to
get an error instead.

I tracked this down to an unfortunate design choice in
dwarf2_section_info, a class which wraps around a bfd section, and is
used for reading in debug information.  GBB creates many
dwarf2_section_info objects, one for each debug section that might
need to be read, then as we find the input bfd sections we associate
them with the corresponding dwarf2_section_info.

If no matching input bfd section is found then the dwarf2_section_info
is left in an unassociated state, its internal bfd section pointer is
null.

If later GDB tries to read content from the dwarf2_section_info, for
example, which trying to read the string associated with DW_FORM_strp,
we spot that there is no associated bfd section and issue an error
message.

To make the users life easier, the error message includes the section
name being looked for, and the bfd from which the section was
obtained.

However, we get the section name by calling bfd_section_name on the
associated section, and we get the bfd filename by calling
bfd_get_filename on the owner of the associated section.

Of course, if there is no associated section then both the calls
bfd_section_name and dwarf2_section_info::get_bfd_owner will result in
undefined behaviour (e.g. a crash).

The solution I propose in this patch is, I know, not ideal.  I simply
spot the case where there is no associated section, and print a
simpler error message, leaving out the section name and filename.

A better solution would involve redesigning dwarf2_section_info, we
could associate each dwarf2_section_info with the initial bfd being
parsed.  We would then display this filename if there's nothing better
to display (e.g. if we find a section in a dwo/dwp split dwarf file
then we would probably use that filename in preference).

Each dwarf2_section_info could also have the concept of the default
section name that would be read for that section, for example, string
data might appear in ".debug_str" or ".zdebug_str", but if neither is
found, then it would probably be OK to just say ".debug_str" is
missing.

Anyway, I didn't do any of that redesign, I just wanted to stop GDB
crashing for now, so instead we get this:

  Dwarf Error: DW_FORM_strp used without required section

Which isn't the best, but in context, isn't too bad:

  Reading symbols from /path/to/executable...
  Dwarf Error: DW_FORM_strp used without required section
  (No debugging symbols found in /path/to/executable)

I also added some asserts into dwarf2_section_info which should
trigger before GDB crashes in future, if we trigger any other bad
paths through this code.

And there's a test for the specific issue I hit.

gdb/ChangeLog:

	* dwarf2/section.c (dwarf2_section_info::get_bfd_owner): Add an
	assert.
	(dwarf2_section_info::get_file_name): Add an assert.
	(dwarf2_section_info::read_string): Display a minimal, sane error
	when the dwarf2_section_info is not associated with a bfd section.

gdb/testsuite/ChangeLog:

	* gdb.dwarf2/dw2-using-debug-str.exp: Add an additional test.
---
 gdb/ChangeLog                                 |  8 ++++
 gdb/dwarf2/section.c                          | 12 +++++-
 gdb/testsuite/ChangeLog                       |  4 ++
 .../gdb.dwarf2/dw2-using-debug-str.exp        | 41 +++++++++++++++++++
 4 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2/section.c b/gdb/dwarf2/section.c
index a839c47ecca..8e1c0197c85 100644
--- a/gdb/dwarf2/section.c
+++ b/gdb/dwarf2/section.c
@@ -54,6 +54,7 @@ dwarf2_section_info::get_bfd_owner () const
       section = get_containing_section ();
       gdb_assert (!section->is_virtual);
     }
+  gdb_assert (section->s.section != nullptr);
   return section->s.section->owner;
 }
 
@@ -83,6 +84,7 @@ dwarf2_section_info::get_file_name () const
 {
   bfd *abfd = get_bfd_owner ();
 
+  gdb_assert (abfd != nullptr);
   return bfd_get_filename (abfd);
 }
 
@@ -194,8 +196,14 @@ dwarf2_section_info::read_string (struct objfile *objfile, LONGEST str_offset,
 {
   read (objfile);
   if (buffer == NULL)
-    error (_("%s used without %s section [in module %s]"),
-	   form_name, get_name (), get_file_name ());
+    {
+      if (get_bfd_section () == nullptr)
+	error (_("Dwarf Error: %s used without required section"),
+	       form_name);
+      else
+	error (_("Dwarf Error: %s used without %s section [in module %s]"),
+	       form_name, get_name (), get_file_name ());
+    }
   if (str_offset >= size)
     error (_("%s pointing outside of %s section [in module %s]"),
 	   form_name, get_name (), get_file_name ());
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
index 3184f9078cb..62febdb0a99 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
@@ -99,3 +99,44 @@ if ![runto_main] {
 # fictional, it only exists in the DWARF, but it contains lots of nice
 # field names, all of which are stored in the .debug_str section.
 gdb_test "p global_var" " = \\{aa = 0, bb = 0, cc = 0\\}"
+
+# Now copy the executable, and remove the .debug_str section.  This
+# creates an executable with an invalid DWARF configuration.  GDB
+# should give an error when trying to read the debug information from
+# this executable.
+set binfile_no_debug_str "${binfile}-no-debug-str"
+set args "--remove-section .debug_str $binfile ${binfile_no_debug_str}"
+if {[run_on_host "objcopy" [gdb_find_objcopy] "$args"]} {
+    perror "failed to run objcopy"
+    return -1
+}
+
+# Restart GDB, but don't load an executable.  When we do load the
+# executable we're going to get an error, which we check for below.
+clean_restart
+
+# This pattern is hit when GDB does not use -readnow (i.e. the default
+# behaviour).
+set pattern1 \
+    [multi_line \
+        "Reading symbols from \[^\r\n\]+" \
+        "Dwarf Error: DW_FORM_strp used without required section" \
+        "\\(No debugging symbols \[^\r\n\]+\\)"]
+
+# This pattern is hit when GDB does use -readnow (e.g. running with
+# --target_board=readnow).
+set pattern2 \
+    [multi_line \
+        "Reading symbols from \[^\r\n\]+" \
+        "Expanding full symbols from \[^\r\n\]+" \
+        "Dwarf Error: DW_FORM_strp used without required section"]
+
+# Load the executable, we expect an error from the DWARF parser.
+gdb_test_multiple "file $binfile_no_debug_str" "file $testfile" {
+    -wrap -re $pattern1 {
+       pass $gdb_test_name
+    }
+    -re -wrap "$pattern2" {
+       pass $gdb_test_name
+    }
+}
-- 
2.25.4

Re: [PATCH] gdb: Handle missing .debug_str section

[Andrew Burgess]

Ping!

* Andrew Burgess <andrew.burgess@embecosm.com> [2021-03-22 17:43:34 +0000]:

> While messing with the Dwarf assembler (gdb/testsuite/lib/dwarf.exp) I
> managed to create an ELF which made use of DW_FORM_strp, but didn't
> include a .debug_str section.
> 
> When I started GDB on this ELF, GDB crashed.  I would have expected to
> get an error instead.
> 
> I tracked this down to an unfortunate design choice in
> dwarf2_section_info, a class which wraps around a bfd section, and is
> used for reading in debug information.  GBB creates many
> dwarf2_section_info objects, one for each debug section that might
> need to be read, then as we find the input bfd sections we associate
> them with the corresponding dwarf2_section_info.
> 
> If no matching input bfd section is found then the dwarf2_section_info
> is left in an unassociated state, its internal bfd section pointer is
> null.
> 
> If later GDB tries to read content from the dwarf2_section_info, for
> example, which trying to read the string associated with DW_FORM_strp,
> we spot that there is no associated bfd section and issue an error
> message.
> 
> To make the users life easier, the error message includes the section
> name being looked for, and the bfd from which the section was
> obtained.
> 
> However, we get the section name by calling bfd_section_name on the
> associated section, and we get the bfd filename by calling
> bfd_get_filename on the owner of the associated section.
> 
> Of course, if there is no associated section then both the calls
> bfd_section_name and dwarf2_section_info::get_bfd_owner will result in
> undefined behaviour (e.g. a crash).
> 
> The solution I propose in this patch is, I know, not ideal.  I simply
> spot the case where there is no associated section, and print a
> simpler error message, leaving out the section name and filename.
> 
> A better solution would involve redesigning dwarf2_section_info, we
> could associate each dwarf2_section_info with the initial bfd being
> parsed.  We would then display this filename if there's nothing better
> to display (e.g. if we find a section in a dwo/dwp split dwarf file
> then we would probably use that filename in preference).
> 
> Each dwarf2_section_info could also have the concept of the default
> section name that would be read for that section, for example, string
> data might appear in ".debug_str" or ".zdebug_str", but if neither is
> found, then it would probably be OK to just say ".debug_str" is
> missing.
> 
> Anyway, I didn't do any of that redesign, I just wanted to stop GDB
> crashing for now, so instead we get this:
> 
>   Dwarf Error: DW_FORM_strp used without required section
> 
> Which isn't the best, but in context, isn't too bad:
> 
>   Reading symbols from /path/to/executable...
>   Dwarf Error: DW_FORM_strp used without required section
>   (No debugging symbols found in /path/to/executable)
> 
> I also added some asserts into dwarf2_section_info which should
> trigger before GDB crashes in future, if we trigger any other bad
> paths through this code.
> 
> And there's a test for the specific issue I hit.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2/section.c (dwarf2_section_info::get_bfd_owner): Add an
> 	assert.
> 	(dwarf2_section_info::get_file_name): Add an assert.
> 	(dwarf2_section_info::read_string): Display a minimal, sane error
> 	when the dwarf2_section_info is not associated with a bfd section.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.dwarf2/dw2-using-debug-str.exp: Add an additional test.
> ---
>  gdb/ChangeLog                                 |  8 ++++
>  gdb/dwarf2/section.c                          | 12 +++++-
>  gdb/testsuite/ChangeLog                       |  4 ++
>  .../gdb.dwarf2/dw2-using-debug-str.exp        | 41 +++++++++++++++++++
>  4 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/dwarf2/section.c b/gdb/dwarf2/section.c
> index a839c47ecca..8e1c0197c85 100644
> --- a/gdb/dwarf2/section.c
> +++ b/gdb/dwarf2/section.c
> @@ -54,6 +54,7 @@ dwarf2_section_info::get_bfd_owner () const
>        section = get_containing_section ();
>        gdb_assert (!section->is_virtual);
>      }
> +  gdb_assert (section->s.section != nullptr);
>    return section->s.section->owner;
>  }
>  
> @@ -83,6 +84,7 @@ dwarf2_section_info::get_file_name () const
>  {
>    bfd *abfd = get_bfd_owner ();
>  
> +  gdb_assert (abfd != nullptr);
>    return bfd_get_filename (abfd);
>  }
>  
> @@ -194,8 +196,14 @@ dwarf2_section_info::read_string (struct objfile *objfile, LONGEST str_offset,
>  {
>    read (objfile);
>    if (buffer == NULL)
> -    error (_("%s used without %s section [in module %s]"),
> -	   form_name, get_name (), get_file_name ());
> +    {
> +      if (get_bfd_section () == nullptr)
> +	error (_("Dwarf Error: %s used without required section"),
> +	       form_name);
> +      else
> +	error (_("Dwarf Error: %s used without %s section [in module %s]"),
> +	       form_name, get_name (), get_file_name ());
> +    }
>    if (str_offset >= size)
>      error (_("%s pointing outside of %s section [in module %s]"),
>  	   form_name, get_name (), get_file_name ());
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
> index 3184f9078cb..62febdb0a99 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-using-debug-str.exp
> @@ -99,3 +99,44 @@ if ![runto_main] {
>  # fictional, it only exists in the DWARF, but it contains lots of nice
>  # field names, all of which are stored in the .debug_str section.
>  gdb_test "p global_var" " = \\{aa = 0, bb = 0, cc = 0\\}"
> +
> +# Now copy the executable, and remove the .debug_str section.  This
> +# creates an executable with an invalid DWARF configuration.  GDB
> +# should give an error when trying to read the debug information from
> +# this executable.
> +set binfile_no_debug_str "${binfile}-no-debug-str"
> +set args "--remove-section .debug_str $binfile ${binfile_no_debug_str}"
> +if {[run_on_host "objcopy" [gdb_find_objcopy] "$args"]} {
> +    perror "failed to run objcopy"
> +    return -1
> +}
> +
> +# Restart GDB, but don't load an executable.  When we do load the
> +# executable we're going to get an error, which we check for below.
> +clean_restart
> +
> +# This pattern is hit when GDB does not use -readnow (i.e. the default
> +# behaviour).
> +set pattern1 \
> +    [multi_line \
> +        "Reading symbols from \[^\r\n\]+" \
> +        "Dwarf Error: DW_FORM_strp used without required section" \
> +        "\\(No debugging symbols \[^\r\n\]+\\)"]
> +
> +# This pattern is hit when GDB does use -readnow (e.g. running with
> +# --target_board=readnow).
> +set pattern2 \
> +    [multi_line \
> +        "Reading symbols from \[^\r\n\]+" \
> +        "Expanding full symbols from \[^\r\n\]+" \
> +        "Dwarf Error: DW_FORM_strp used without required section"]
> +
> +# Load the executable, we expect an error from the DWARF parser.
> +gdb_test_multiple "file $binfile_no_debug_str" "file $testfile" {
> +    -wrap -re $pattern1 {
> +       pass $gdb_test_name
> +    }
> +    -re -wrap "$pattern2" {
> +       pass $gdb_test_name
> +    }
> +}
> -- 
> 2.25.4
> 

Re: [PATCH] gdb: Handle missing .debug_str section

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Anyway, I didn't do any of that redesign, I just wanted to stop GDB
Andrew> crashing for now, so instead we get this:

Andrew>   Dwarf Error: DW_FORM_strp used without required section

Andrew> Which isn't the best, but in context, isn't too bad:

It seems reasonable to me.
Thank you for doing this.

Tom