[RFA] Fix gdb/15827 (crash w/corrupt DWARF)

Message ID 532C7624.4080704@redhat.com
State Committed
Headers

Commit Message

Keith Seitz March 21, 2014, 5:25 p.m. UTC
  Hi, Joel!

Thank you for having a look at this.

On 03/21/2014 10:14 AM, Joel Brobecker wrote:
> Use "(void)" instead of "()". There was a recent policy clarification
> regarding the CS to be using with testcases, and basically we decided
> to try to follow the GCS as much as we reasonably could.

Cut-n-paste-o. Fixed.


>> +# If we get here and gdb hasn't crashed, the tests pass.
>> +pass "corrupt DWARF"
>
> That's just me but I usually do a "print 1" test, just to make sure
> that even if the testing framework did not detect the GDB process
> dying, the "print 1" test definitely will. Not important on most,
> if not all platforms, but ISTR some odd platforms where this helped.
> That's just a suggestion, you don't have to follow it.

Actually, I think that's a very good idea (which did not occur to me). 
My big hesitation with this is that this failure gets reported as 
UNRESOLVED. While I try to be studious about checking 
XFAIL/UNRESOLVED/ERROR, I sometimes overlook these in favor of a raw 
PASS/FAIL check in gdb.sum.

I've attached a revision with those two changes (ChangeLog remains 
unchanged).

Keith

ChangeLog
2014-03-20  Keith Seitz  <keiths@redhat.com>

	PR gdb/15827
	* dwarf2read.c (skip_one_die): Check that all relative-offset
	sibling DIEs fall within range of the current reader's buffer.
	(read_partial_die): Likewise.

testsuite/ChangeLog
2014-03-20  Keith Seitz  <keiths@redhat.com>

	PR gdb/15827
	* gdb.dwarf2/corrupt.c: New file.
	* gdb.dwarf2/corrupt.exp: New file.
  

Comments

Joel Brobecker March 21, 2014, 5:54 p.m. UTC | #1
> ChangeLog
> 2014-03-20  Keith Seitz  <keiths@redhat.com>
> 
> 	PR gdb/15827
> 	* dwarf2read.c (skip_one_die): Check that all relative-offset
> 	sibling DIEs fall within range of the current reader's buffer.
> 	(read_partial_die): Likewise.
> 
> testsuite/ChangeLog
> 2014-03-20  Keith Seitz  <keiths@redhat.com>
> 
> 	PR gdb/15827
> 	* gdb.dwarf2/corrupt.c: New file.
> 	* gdb.dwarf2/corrupt.exp: New file.

FAOD, this version looks good to me.

Thanks!
  
Keith Seitz April 16, 2014, 9:50 p.m. UTC | #2
On 03/21/2014 10:54 AM, Joel Brobecker wrote:
>> ChangeLog
>> 2014-03-20  Keith Seitz  <keiths@redhat.com>
>>
>> 	PR gdb/15827
>> 	* dwarf2read.c (skip_one_die): Check that all relative-offset
>> 	sibling DIEs fall within range of the current reader's buffer.
>> 	(read_partial_die): Likewise.
>>
>> testsuite/ChangeLog
>> 2014-03-20  Keith Seitz  <keiths@redhat.com>
>>
>> 	PR gdb/15827
>> 	* gdb.dwarf2/corrupt.c: New file.
>> 	* gdb.dwarf2/corrupt.exp: New file.
>
> FAOD, this version looks good to me.
>

I don't know why I didn't see this response, but, well, I didn't.

Sorry about the delay -- I've now pushed this patch in.

Thank you for the review.

Keith
  

Patch

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 705dc2d..c30b1b3 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -7103,6 +7103,8 @@  skip_one_die (const struct die_reader_specs *reader, const gdb_byte *info_ptr,
 	      if (sibling_ptr < info_ptr)
 		complaint (&symfile_complaints,
 			   _("DW_AT_sibling points backwards"));
+	      else if (sibling_ptr > reader->buffer_end)
+		dwarf2_section_buffer_overflow_complaint (reader->die_section);
 	      else
 		return sibling_ptr;
 	    }
@@ -15416,6 +15418,8 @@  read_partial_die (const struct die_reader_specs *reader,
 	      if (sibling_ptr < info_ptr)
 		complaint (&symfile_complaints,
 			   _("DW_AT_sibling points backwards"));
+	      else if (sibling_ptr > reader->buffer_end)
+		dwarf2_section_buffer_overflow_complaint (reader->die_section);
 	      else
 		part_die->sibling = sibling_ptr;
 	    }
diff --git a/gdb/testsuite/gdb.dwarf2/corrupt.c b/gdb/testsuite/gdb.dwarf2/corrupt.c
new file mode 100644
index 0000000..bcd5fd8
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/corrupt.c
@@ -0,0 +1,24 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Dummy main function.  */
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/corrupt.exp b/gdb/testsuite/gdb.dwarf2/corrupt.exp
new file mode 100644
index 0000000..048ae0c
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/corrupt.exp
@@ -0,0 +1,77 @@ 
+# Copyright 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test corrupt DWARF input
+# PR gdb/15827
+
+load_lib dwarf.exp
+
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile corrupt.c corrupt.S
+
+# Make the DWARF used for the test.
+#
+# Here we put DW_AT_sibling DIEs into the output which
+# point off into la-la land.  The whole purpose is to simulate
+# corrupt DWARF information and make sure that GDB can handle it
+# without crashing.
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    cu {} {
+	compile_unit {} {
+	    declare_labels int_label
+
+	    int_label: base_type {
+		{byte_size 4}
+		{name "int"}
+	    }
+
+	    enumeration_type {
+		{name "ENUM"}
+		{byte_size 4}
+	    } {
+		enumerator {
+		    {name "A"}
+		    {const_value 0}
+		}
+		enumerator {
+		    {name "B"}
+		    {const_value 1}
+		    {sibling 12345678 DW_FORM_ref4}
+		} {
+		    base_type {
+			{byte_size 1}
+			{name "char"}
+		    }
+		}
+		array_type {
+		    {type :$int_label}
+		    {sibling 12345678 DW_FORM_ref4}
+		}
+	    }
+	}
+    }
+}
+
+if {[prepare_for_testing $testfile.exp $testfile \
+	 [list $srcfile $asm_file] {nodebug}]} {
+    return -1
+}
+
+gdb_test "print 1" "= 1" "recover from corrupt DWARF"