[PATCH] Don't crash if parse_definition_macro's 'body' is NULL

[PATCH] Don't crash if parse_definition_macro's 'body' is NULL

[Sergio Durigan Junior]

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(+)

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

Re: [PATCH] Don't crash if parse_definition_macro's 'body' is NULL

[Tom Tromey]

>>>>> "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

Re: [PATCH] Don't crash if parse_definition_macro's 'body' is NULL

[Sergio Durigan Junior]

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,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[PATCH] Don't crash if dwarf_decode_macro_bytes's 'body' is NULL

[Sergio Durigan Junior]

Hi,

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

During the Fedora RPM build process, 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.

The underlying problem is that Fedora's rpm-build's "debugedit"
program will silently corrupt .debug_macro strings when a binary is
compiled with -g3.  This is being taken care of by Mark Wielaard,
here:

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

However, I still feel it's important to make GDB more resilient
against invalid DWARF input, so I'm proposing this rather simple patch
to catch the situation when "body == NULL" (i.e., it's probably been
corrupted) and issue a complaint.  This is not a real fix to the
problem, of course, but at least GDB is able to finish without
segfaulting.

OK for master?

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

	Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1708192
	* dwarf2read.c (dwarf_decode_macro_bytes): Check whether 'body' is
	NULL, and complain if that's the case.
---
 gdb/ChangeLog    |  6 ++++++
 gdb/dwarf2read.c | 18 +++++++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 19458ccc72..d64ed7df04 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-05-14  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-12  Paul Naert  <paul.naert@polymtl.ca>
 
 	* language.c (language_sniff_from_mangled_name): Fix "langauge"
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index b29c089606..63622ba76f 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -24609,7 +24609,23 @@ dwarf_decode_macro_bytes (struct dwarf2_cu *cu,
 			 line == 0 ? _("zero") : _("non-zero"), line, body);
 
 	    if (is_define)
-	      parse_macro_definition (current_file, line, body);
+	      {
+		if (body != NULL)
+		  parse_macro_definition (current_file, line, body);
+		else
+		  {
+		    /* Fedora's rpm-build's "debugedit" binary
+		       corrupted .debug_macro sections.
+
+		       For more info, see
+		       https://bugzilla.redhat.com/show_bug.cgi?id=1708786 */
+		    complaint (_("debug info gives %s invalid macro definition "
+				 "without body (corrupted?) at line %d"),
+			       at_commandline ? _("command-line")
+			       : _("in-file"),
+			       line == 0 ? _("zero") : _("non-zero"), line);
+		  }
+	      }
 	    else
 	      {
 		gdb_assert (macinfo_type == DW_MACRO_undef
-- 
2.17.2

Re: [PATCH] Don't crash if dwarf_decode_macro_bytes's 'body' is NULL

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> OK for master?

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

Sergio> 	Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1708192
Sergio> 	* dwarf2read.c (dwarf_decode_macro_bytes): Check whether 'body' is
Sergio> 	NULL, and complain if that's the case.

Sergio> +		    complaint (_("debug info gives %s invalid macro definition "
Sergio> +				 "without body (corrupted?) at line %d"),
Sergio> +			       at_commandline ? _("command-line")
Sergio> +			       : _("in-file"),
Sergio> +			       line == 0 ? _("zero") : _("non-zero"), line);

This seems weird since it doesn't use current_file, and if I'm reading
correctly, there are 3 arguments but only 2 % substitutions.  The
compiler should catch the latter, so that's doubly strange.  I think for
a complaint it's fine to emit a line==0 or whatever, mostly it's for
helping to track down the incorrect spot.

Tom

Re: [PATCH] Don't crash if dwarf_decode_macro_bytes's 'body' is NULL

[Sergio Durigan Junior]

On Tuesday, May 14 2019, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> OK for master?
>
> Sergio> gdb/ChangeLog:
> Sergio> 2019-05-14  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> Sergio> 	Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1708192
> Sergio> 	* dwarf2read.c (dwarf_decode_macro_bytes): Check whether 'body' is
> Sergio> 	NULL, and complain if that's the case.
>
> Sergio> +		    complaint (_("debug info gives %s invalid macro definition "
> Sergio> +				 "without body (corrupted?) at line %d"),
> Sergio> +			       at_commandline ? _("command-line")
> Sergio> +			       : _("in-file"),
> Sergio> +			       line == 0 ? _("zero") : _("non-zero"), line);
>
> This seems weird since it doesn't use current_file, and if I'm reading
> correctly, there are 3 arguments but only 2 % substitutions.  The
> compiler should catch the latter, so that's doubly strange.  I think for
> a complaint it's fine to emit a line==0 or whatever, mostly it's for
> helping to track down the incorrect spot.

Sorry, you're right, I left one extra argument there.  I was testing the
patch on a VM, where I had everything correct, but then I made some
extra adjustments and forgot to remove the extra "line == 0 ? _("zero")
: _("non-zero")".

As for the filename, I was following what other complaints were doing.
But I also agree that having a filename would be best.  What do you
think of:

    complaint (_("debug info gives %s invalid macro definition "
		 "without body (corrupted?) at line %d"
		 "on file %s"),
	       at_commandline ? _("command-line")
	       : _("in-file"),
	       line, current_file->filename);

?

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Don't crash if dwarf_decode_macro_bytes's 'body' is NULL

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> Sorry, you're right, I left one extra argument there.  I was testing the
Sergio> patch on a VM, where I had everything correct, but then I made some
Sergio> extra adjustments and forgot to remove the extra "line == 0 ? _("zero")
Sergio> : _("non-zero")".

Sergio> As for the filename, I was following what other complaints were doing.

I think it's fine to just follow the other ones there.

Sergio> But I also agree that having a filename would be best.  What do you
Sergio> think of:

Sergio>     complaint (_("debug info gives %s invalid macro definition "
Sergio> 		 "without body (corrupted?) at line %d"
Sergio> 		 "on file %s"),
Sergio> 	       at_commandline ? _("command-line")
Sergio> 	       : _("in-file"),
Sergio> 	       line, current_file->filename);

Sergio> ?

Works for me.  This is ok, thanks.

Tom

Re: [PATCH] Don't crash if dwarf_decode_macro_bytes's 'body' is NULL

[Andreas Schwab]

On Mai 14 2019, Sergio Durigan Junior <sergiodj@redhat.com> wrote:

>     complaint (_("debug info gives %s invalid macro definition "
> 		 "without body (corrupted?) at line %d"
> 		 "on file %s"),
> 	       at_commandline ? _("command-line")
> 	       : _("in-file"),
> 	       line, current_file->filename);

Note that this is difficult to translate.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Don't crash if dwarf_decode_macro_bytes's 'body' is NULL

[Tom Tromey]

>>>>> "Andreas" == Andreas Schwab <schwab@suse.de> writes:

Andreas> On Mai 14 2019, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>> complaint (_("debug info gives %s invalid macro definition "
>> "without body (corrupted?) at line %d"
>> "on file %s"),
>> at_commandline ? _("command-line")
>> : _("in-file"),
>> line, current_file->filename);

Andreas> Note that this is difficult to translate.

True.  However, there are other similar ones nearby; and it is just a
complaint (turned off by default); and nobody seems very serious about
translating gdb anyhow -- there are no translations in-tree.

Tom

Re: [PATCH] Don't crash if dwarf_decode_macro_bytes's 'body' is NULL

[Sergio Durigan Junior]

On Wednesday, May 15 2019, Tom Tromey wrote:

>>>>>> "Andreas" == Andreas Schwab <schwab@suse.de> writes:
>
> Andreas> On Mai 14 2019, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>> complaint (_("debug info gives %s invalid macro definition "
>>> "without body (corrupted?) at line %d"
>>> "on file %s"),
>>> at_commandline ? _("command-line")
>>> : _("in-file"),
>>> line, current_file->filename);
>
> Andreas> Note that this is difficult to translate.
>
> True.  However, there are other similar ones nearby; and it is just a
> complaint (turned off by default); and nobody seems very serious about
> translating gdb anyhow -- there are no translations in-tree.

Thanks; I was going to point out that there are several other strings
that are hard to translate in the file.

I pushed the patch now, thank you.

7bede82892a06e6c26989803e70f53697392dcf9

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[PATCH] Don't crash is dwarf_decode_macro_bytes's 'body' is NULL, even when '!is_define'

[Sergio Durigan Junior]

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

On commit 7bede82892a06e6c26989803e70f53697392dcf9 ("Don't crash if
dwarf_decode_macro_bytes's 'body' is NULL"), I was too strict when
checking if 'body' is NULL: the check only comprised the case when
'is_define' is true.  However, the corruption of .debug_macro by
rpmbuild's "debugedit" also affects the case when 'is_define' is
false, i.e., when the macro is being undefined.

This commit improves the check and covers both cases now.  This has
been tested on Fedora 30 with a problematic debuginfo, and I don't see
a segfault anymore.

OK to push?

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

	Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1708192
	Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1715008
	* dwarf2read.c (dwarf_decode_macro_bytes): Move check to see if
	'body' is NULL to the outter 'if', protecting the '!is_define'
	situation as well.
---
 gdb/ChangeLog    |  8 ++++++++
 gdb/dwarf2read.c | 31 ++++++++++++++-----------------
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f122f5b21f..48e5847b13 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2019-05-29  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1708192
+	Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1715008
+	* dwarf2read.c (dwarf_decode_macro_bytes): Move check to see if
+	'body' is NULL to the outter 'if', protecting the '!is_define'
+	situation as well.
+
 2019-05-28  Tom Tromey  <tromey@adacore.com>
 
 	* ada-lang.c (ada_remove_Xbn_suffix)
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index f48b931a3f..d1c7a8e67c 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -24635,25 +24635,22 @@ dwarf_decode_macro_bytes (struct dwarf2_cu *cu,
 			 is_define ? _("definition") : _("undefinition"),
 			 line == 0 ? _("zero") : _("non-zero"), line, body);
 
-	    if (is_define)
+	    if (body == NULL)
 	      {
-		if (body != NULL)
-		  parse_macro_definition (current_file, line, body);
-		else
-		  {
-		    /* Fedora's rpm-build's "debugedit" binary
-		       corrupted .debug_macro sections.
-
-		       For more info, see
-		       https://bugzilla.redhat.com/show_bug.cgi?id=1708786 */
-		    complaint (_("debug info gives %s invalid macro definition "
-				 "without body (corrupted?) at line %d"
-				 "on file %s"),
-			       at_commandline ? _("command-line")
-			       : _("in-file"),
-			       line, current_file->filename);
-		  }
+		/* Fedora's rpm-build's "debugedit" binary
+		   corrupted .debug_macro sections.
+
+		   For more info, see
+		   https://bugzilla.redhat.com/show_bug.cgi?id=1708786 */
+		complaint (_("debug info gives %s invalid macro %s "
+			     "without body (corrupted?) at line %d "
+			     "on file %s"),
+			   at_commandline ? _("command-line") : _("in-file"),
+			   is_define ? _("definition") : _("undefinition"),
+			   line, current_file->filename);
 	      }
+	    else if (is_define)
+	      parse_macro_definition (current_file, line, body);
 	    else
 	      {
 		gdb_assert (macinfo_type == DW_MACRO_undef
-- 
2.21.0

Re: [PATCH] Don't crash is dwarf_decode_macro_bytes's 'body' is NULL, even when '!is_define'

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1715008
Sergio> On commit 7bede82892a06e6c26989803e70f53697392dcf9 ("Don't crash if
Sergio> dwarf_decode_macro_bytes's 'body' is NULL"), I was too strict when
Sergio> checking if 'body' is NULL: the check only comprised the case when
Sergio> 'is_define' is true.  However, the corruption of .debug_macro by
Sergio> rpmbuild's "debugedit" also affects the case when 'is_define' is
Sergio> false, i.e., when the macro is being undefined.

Sergio> This commit improves the check and covers both cases now.  This has
Sergio> been tested on Fedora 30 with a problematic debuginfo, and I don't see
Sergio> a segfault anymore.

Sergio> OK to push?

Thanks for doing this.  This is ok.

Tom

Re: [PATCH] Don't crash is dwarf_decode_macro_bytes's 'body' is NULL, even when '!is_define'

[Sergio Durigan Junior]

On Wednesday, May 29 2019, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1715008
> Sergio> On commit 7bede82892a06e6c26989803e70f53697392dcf9 ("Don't crash if
> Sergio> dwarf_decode_macro_bytes's 'body' is NULL"), I was too strict when
> Sergio> checking if 'body' is NULL: the check only comprised the case when
> Sergio> 'is_define' is true.  However, the corruption of .debug_macro by
> Sergio> rpmbuild's "debugedit" also affects the case when 'is_define' is
> Sergio> false, i.e., when the macro is being undefined.
>
> Sergio> This commit improves the check and covers both cases now.  This has
> Sergio> been tested on Fedora 30 with a problematic debuginfo, and I don't see
> Sergio> a segfault anymore.
>
> Sergio> OK to push?
>
> Thanks for doing this.  This is ok.

Thanks for the review, Tom.

Pushed: 955b06fa576df1a6954263043ea3f3a5b9ad5940

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/