Don't crash if dwarf_decode_macro_bytes's 'body' is NULL
Commit Message
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(-)
Comments
>>>>> "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
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" == 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
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" == 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
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
@@ -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"
@@ -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