[rfc] Detect dwarf address size mismatch

[rfc] Detect dwarf address size mismatch

[Daniel Jacobowitz]

I just fixed a gas bug which caused MIPS64 Linux kernels to have
corrupt DWARF information.  The .debug_info compilation unit header
listed the address size as 4, but the .debug_line section used 64-bit
addresses.  This caused GDB to parse the last four bits of each
address as if they were instructions in the line number program.

That version of GDB crashed when it got a bogus DW_LNS_set_file with
an out-of-bounds file number (which has already been fixed in HEAD).
But I think this patch is still useful, to detect the mismatch
promptly instead of going off into the woods parsing bad data.
I think I did get HEAD to crash once while testing.

A more intrusive patch could let GDB handle the bad files as their
producer intended, by reading an address of size extended_len - 1,
but I don't think it's worth it when we can fix gas.

Any comments on this patch, or shall I commit it?

-- 
Daniel Jacobowitz
CodeSourcery

2007-07-11  Daniel Jacobowitz  <dan@codesourcery.com>

	* dwarf2read.c (dwarf_decode_lines): Detect address size mismatches.

---
 gdb/dwarf2read.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

Index: gdb-6.6.50/gdb/dwarf2read.c
===================================================================
--- gdb-6.6.50.orig/gdb/dwarf2read.c	2007-07-10 13:25:33.000000000 -0700
+++ gdb-6.6.50/gdb/dwarf2read.c	2007-07-11 07:09:17.000000000 -0700
@@ -6660,7 +6660,7 @@ dwarf_decode_lines (struct line_header *
 {
   gdb_byte *line_ptr;
   gdb_byte *line_end;
-  unsigned int bytes_read;
+  unsigned int bytes_read, extended_len;
   unsigned char op_code, extended_op, adj_opcode;
   CORE_ADDR baseaddr;
   struct objfile *objfile = cu->objfile;
@@ -6730,7 +6730,7 @@ dwarf_decode_lines (struct line_header *
 	  else switch (op_code)
 	    {
 	    case DW_LNS_extended_op:
-	      read_unsigned_leb128 (abfd, line_ptr, &bytes_read);
+	      extended_len = read_unsigned_leb128 (abfd, line_ptr, &bytes_read);
 	      line_ptr += bytes_read;
 	      extended_op = read_1_byte (abfd, line_ptr);
 	      line_ptr += 1;
@@ -6746,6 +6746,12 @@ dwarf_decode_lines (struct line_header *
 		  address = read_address (abfd, line_ptr, cu, &bytes_read);
 		  line_ptr += bytes_read;
 		  address += baseaddr;
+		  if (bytes_read + 1 != extended_len)
+		    {
+		      complaint (&symfile_complaints,
+				 _("bad address size in .debug_line section"));
+		      return;
+		    }
 		  break;
 		case DW_LNE_define_file:
                   {

Re: [rfc] Detect dwarf address size mismatch

[Eli Zaretskii]

> Date: Wed, 11 Jul 2007 10:19:12 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> Any comments on this patch, or shall I commit it?

Looks fine to me.

Re: [rfc] Detect dwarf address size mismatch

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> I just fixed a gas bug which caused MIPS64 Linux kernels to have
> corrupt DWARF information.  The .debug_info compilation unit header
> listed the address size as 4, but the .debug_line section used 64-bit
> addresses.  This caused GDB to parse the last four bits of each
> address as if they were instructions in the line number program.
>
> That version of GDB crashed when it got a bogus DW_LNS_set_file with
> an out-of-bounds file number (which has already been fixed in HEAD).
> But I think this patch is still useful, to detect the mismatch
> promptly instead of going off into the woods parsing bad data.
> I think I did get HEAD to crash once while testing.
>
> A more intrusive patch could let GDB handle the bad files as their
> producer intended, by reading an address of size extended_len - 1,
> but I don't think it's worth it when we can fix gas.
>
> Any comments on this patch, or shall I commit it?

Would it make sense to move the check after the extended_op switch
altogether, and always use extended_len to advance line_ptr?  This
would make GDB more robust against new extended opcodes.

The complaint is definitely appropriate, though.

Re: [rfc] Detect dwarf address size mismatch

[Daniel Jacobowitz]

On Wed, Jul 11, 2007 at 12:27:29PM -0700, Jim Blandy wrote:
> Would it make sense to move the check after the extended_op switch
> altogether, and always use extended_len to advance line_ptr?  This
> would make GDB more robust against new extended opcodes.

I thought about it; it might make sense to do this in the default
case, but not in any of the handled cases, since they need to advance
line_ptr anyway to read multiple items.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Detect dwarf address size mismatch

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> On Wed, Jul 11, 2007 at 12:27:29PM -0700, Jim Blandy wrote:
>> Would it make sense to move the check after the extended_op switch
>> altogether, and always use extended_len to advance line_ptr?  This
>> would make GDB more robust against new extended opcodes.
>
> I thought about it; it might make sense to do this in the default
> case, but not in any of the handled cases, since they need to advance
> line_ptr anyway to read multiple items.

True.  But I think the check should go after the switch in any case;
there's no reason to only apply sanity checking to that one opcode.

But then as long as one's doing that check across all the extended
opcodes, one has the information needed to insulate the surrounding
code from the individual cases' vaguaries.

The first point seems like a definite should-do, unless I'm missing
something; the second one I agree is less clear-cut.

Re: [rfc] Detect dwarf address size mismatch

[Daniel Jacobowitz]

On Wed, Jul 11, 2007 at 01:30:48PM -0700, Jim Blandy wrote:
> True.  But I think the check should go after the switch in any case;
> there's no reason to only apply sanity checking to that one opcode.

Reasonable enough.  I've checked in this version.

-- 
Daniel Jacobowitz
CodeSourcery

2007-07-17  Daniel Jacobowitz  <dan@codesourcery.com>

	* dwarf2read.c (dwarf_decode_lines): Detect address size mismatches.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.227
diff -u -p -r1.227 dwarf2read.c
--- dwarf2read.c	22 Jun 2007 12:26:59 -0000	1.227
+++ dwarf2read.c	17 Jul 2007 12:45:22 -0000
@@ -6668,9 +6668,9 @@ static void
 dwarf_decode_lines (struct line_header *lh, char *comp_dir, bfd *abfd,
 		    struct dwarf2_cu *cu, struct partial_symtab *pst)
 {
-  gdb_byte *line_ptr;
+  gdb_byte *line_ptr, *extended_end;
   gdb_byte *line_end;
-  unsigned int bytes_read;
+  unsigned int bytes_read, extended_len;
   unsigned char op_code, extended_op, adj_opcode;
   CORE_ADDR baseaddr;
   struct objfile *objfile = cu->objfile;
@@ -6745,8 +6745,9 @@ dwarf_decode_lines (struct line_header *
 	  else switch (op_code)
 	    {
 	    case DW_LNS_extended_op:
-	      read_unsigned_leb128 (abfd, line_ptr, &bytes_read);
+	      extended_len = read_unsigned_leb128 (abfd, line_ptr, &bytes_read);
 	      line_ptr += bytes_read;
+	      extended_end = line_ptr + extended_len;
 	      extended_op = read_1_byte (abfd, line_ptr);
 	      line_ptr += 1;
 	      switch (extended_op)
@@ -6792,6 +6793,15 @@ dwarf_decode_lines (struct line_header *
 			     _("mangled .debug_line section"));
 		  return;
 		}
+	      /* Make sure that we parsed the extended op correctly.  If e.g.
+		 we expected a different address size than the producer used,
+		 we may have read the wrong number of bytes.  */
+	      if (line_ptr != extended_end)
+		{
+		  complaint (&symfile_complaints,
+			     _("mangled .debug_line section"));
+		  return;
+		}
 	      break;
 	    case DW_LNS_copy:
 	      if (lh->num_file_names < file)

Re: [rfc] Detect dwarf address size mismatch

[Mark Kettenis]

> Date: Tue, 17 Jul 2007 08:47:11 -0400
> From: Daniel Jacobowitz <drow@false.org>

Hi Daniel,

Sorry I didn't notice this before but:

> @@ -6792,6 +6793,15 @@ dwarf_decode_lines (struct line_header *
>  			     _("mangled .debug_line section"));
>  		  return;
>  		}
> +	      /* Make sure that we parsed the extended op correctly.  If e.g.
> +		 we expected a different address size than the producer used,
> +		 we may have read the wrong number of bytes.  */
> +	      if (line_ptr != extended_end)
> +		{
> +		  complaint (&symfile_complaints,
> +			     _("mangled .debug_line section"));
> +		  return;
> +		}
>  	      break;
>  	    case DW_LNS_copy:
>  	      if (lh->num_file_names < file)

The complaint "mangled .debug_line section" seems a bit unhelpful to
me, especially since there are now two identical messages for what
appears to be somewhat different conditions.  Any chance of changing
it something more distinguishable?

Mark

Re: [rfc] Detect dwarf address size mismatch

[Daniel Jacobowitz]

On Tue, Jul 17, 2007 at 03:01:04PM +0200, Mark Kettenis wrote:
> 
> > @@ -6792,6 +6793,15 @@ dwarf_decode_lines (struct line_header *
> >  			     _("mangled .debug_line section"));
> >  		  return;
> >  		}
> > +	      /* Make sure that we parsed the extended op correctly.  If e.g.
> > +		 we expected a different address size than the producer used,
> > +		 we may have read the wrong number of bytes.  */
> > +	      if (line_ptr != extended_end)
> > +		{
> > +		  complaint (&symfile_complaints,
> > +			     _("mangled .debug_line section"));
> > +		  return;
> > +		}
> >  	      break;
> >  	    case DW_LNS_copy:
> >  	      if (lh->num_file_names < file)
> 
> The complaint "mangled .debug_line section" seems a bit unhelpful to
> me, especially since there are now two identical messages for what
> appears to be somewhat different conditions.  Any chance of changing
> it something more distinguishable?

It was my mistake, not yours, that you didn't notice it before.  It
used to say "bad address size", but Jim suggested I check all extended
ops instead of just DW_LNE_set_address.  So the more specific complaint
is no longer accurate.

How about changing the first one to "unrecognized extended opcode in
.debug_line", and the second one to "mangled extended opcode in
.debug_line"?

I thought about making the unrecognized opcode case more lenient, and
using break instead of return, since we can advance line_ptr the right
amount.  But I think we can't skip unrecognized line opcodes the way
we can skip unrecognized attributes; if the opcode changes the PC,
then the next advance_pc opcode will start from the wrong place, et cetera.

-- 
Daniel Jacobowitz
CodeSourcery