Patchwork Don't crash if parse_definition_macro's 'body' is NULL

login
register
mail settings
Submitter Sergio Durigan Junior
Date May 10, 2019, 9:04 p.m.
Message ID <20190510210425.2750-1-sergiodj@redhat.com>
Download mbox | patch
Permalink /patch/32640/
State New
Headers show

Comments

Sergio Durigan Junior - May 10, 2019, 9:04 p.m.
Hi,

Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1708192
      https://bugzilla.redhat.com/show_bug.cgi?id=1708786

Fedora's rpm-build's "debugedit" program will silently corrupt
.debug_macro strings when a binary is compiled with -g3.  Later in the
build phase, gdb-add-index is invoked to extract the DWARF index from
the binary, and GDB will segfault because
dwarf2read.c:parse_definition_macro's 'body' variable is NULL.

This very simple patch is just a safeguard against this scenario; it
is not a fix for the problem (which actually happens on "debugedit",
and which Mark Wielaard is already working on), but at least it makes
GDB not crash on invalid DWARF, which is a plus IMO.

OK for master?

gdb/ChangeLog:
2019-05-10  Sergio Durigan Junior  <sergiodj@redhat.com>

	Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1708192
	* dwarf2read.c (parse_macro_definition): Check whether 'body' is
	NULL, and complain/return if that's the case.
---
 gdb/ChangeLog    | 6 ++++++
 gdb/dwarf2read.c | 7 +++++++
 2 files changed, 13 insertions(+)
Tom Tromey - May 13, 2019, 2:01 p.m.
>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> This very simple patch is just a safeguard against this scenario; it
Sergio> is not a fix for the problem (which actually happens on "debugedit",
Sergio> and which Mark Wielaard is already working on), but at least it makes
Sergio> GDB not crash on invalid DWARF, which is a plus IMO.

I don't really get why read_direct_string (and the other functions like
it) returns NULL when it sees an empty string.  How is something like
"#define X" represented such that this doesn't return NULL?

Other complaints in the caller of parse_macro_definition provide a bit
more context, like the macro name, source file, and line number:

	      complaint (_("debug info gives %s macro %s with %s line %d: %s"),
			 at_commandline ? _("command-line") : _("in-file"),
			 is_define ? _("definition") : _("undefinition"),
			 line == 0 ? _("zero") : _("non-zero"), line, body);

... so maybe the new complaint could as well.

Tom
Sergio Durigan Junior - May 14, 2019, 8:54 p.m.
On Monday, May 13 2019, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> This very simple patch is just a safeguard against this scenario; it
> Sergio> is not a fix for the problem (which actually happens on "debugedit",
> Sergio> and which Mark Wielaard is already working on), but at least it makes
> Sergio> GDB not crash on invalid DWARF, which is a plus IMO.
>
> I don't really get why read_direct_string (and the other functions like
> it) returns NULL when it sees an empty string.  How is something like
> "#define X" represented such that this doesn't return NULL?

Thanks for the review.

We've already talked on IRC, but I'll just mention it here as well:

When read_indirect_string_at_offset is called (from
dwarf_decode_macro_bytes), you see this code:

  ...
  if (sect->buffer[str_offset] == '\0')
    return NULL;
  ...

You were wondering why a case like "#define X" didn't trigger this bug,
because NULL should be returned.  However, as I found, in this scenario
sect->buffer will contain "X ".  I.e., it will always contain the
macro's name + its (optional) value.  What we're actually dealing with
here, in debugedit's case, is the corruption of the .debug_macro
section, which renders the define useless.

A more in-depth analysis by Keith can be found here:

  https://bugzilla.redhat.com/show_bug.cgi?id=1684303#c2

> Other complaints in the caller of parse_macro_definition provide a bit
> more context, like the macro name, source file, and line number:
>
> 	      complaint (_("debug info gives %s macro %s with %s line %d: %s"),
> 			 at_commandline ? _("command-line") : _("in-file"),
> 			 is_define ? _("definition") : _("undefinition"),
> 			 line == 0 ? _("zero") : _("non-zero"), line, body);
>
> ... so maybe the new complaint could as well.

I did my best and updated the complaint to contain more info.  I'm
afraid the only extra bit I was able to add was the line number, which,
by my tests, will not be entirely correct (perhaps due to the corruption
itself).  I'll send the patch soon.

Thanks,

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4da409633a..53a4721cb3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@ 
+2019-05-10  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1708192
+	* dwarf2read.c (parse_macro_definition): Check whether 'body' is
+	NULL, and complain/return if that's the case.
+
 2019-05-10  Simon Marchi  <simon.marchi@efficios.com>
 
 	* contrib/cc-with-tweaks.sh: Validate dwz's work.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index b29c089606..e270e7cef3 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -24161,6 +24161,13 @@  parse_macro_definition (struct macro_source_file *file, int line,
 {
   const char *p;
 
+  if (body == NULL)
+    {
+      complaint (_("macro debug info contains a malformed "
+		   "(null) macro definition"));
+      return;
+    }
+
   /* The body string takes one of two forms.  For object-like macro
      definitions, it should be: