[commited] Detect bad debug info

[commited] Detect bad debug info

[Andrew Stubbs]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]

Hi,

I have just committed the attached patch (approved privately by Daniel 
Jacobowitz).

The patch causes GDB to fail gracefully when it encounters a particular 
flavour of bad debug info.

Andrew Stubbs

[-- Attachment #2: bad-debug.patch --]
[-- Type: text/x-diff, Size: 1223 bytes --]

2008-09-19  Andrew Stubbs  <ams@codesourcery.com>

	* frame.c (get_frame_register_bytes): Detect bad debug info.

Index: gdb/frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.251
diff -u -p -r1.251 frame.c
--- gdb/frame.c	26 Aug 2008 17:40:24 -0000	1.251
+++ gdb/frame.c	19 Sep 2008 18:10:34 -0000
@@ -796,6 +796,8 @@ get_frame_register_bytes (struct frame_i
 			  CORE_ADDR offset, int len, gdb_byte *myaddr)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
+  int i;
+  int maxsize;
 
   /* Skip registers wholly inside of OFFSET.  */
   while (offset >= register_size (gdbarch, regnum))
@@ -804,6 +806,22 @@ get_frame_register_bytes (struct frame_i
       regnum++;
     }
 
+  /* Detect bad debug info.  */
+  maxsize = -offset;
+  for (i = regnum; i < gdbarch_num_regs (gdbarch); i++)
+    {
+      int thissize = register_size (gdbarch, i);
+      if (thissize == 0)
+	break;
+      maxsize += thissize;
+    }
+  if (len > maxsize)
+    {
+      warning (_("Bad debug information detected: "
+		 "Attempt to read %d bytes from registers."), len);
+      return 0;
+    }
+
   /* Copy the data.  */
   while (len > 0)
     {

Re: [commited] Detect bad debug info

[Mark Kettenis]

> Date: Fri, 19 Sep 2008 19:16:12 +0100
> From: Andrew Stubbs <ams@codesourcery.com>
> 
> Hi,
> 
> I have just committed the attached patch (approved privately by Daniel 
> Jacobowitz).
> 
> The patch causes GDB to fail gracefully when it encounters a particular 
> flavour of bad debug info.

Would it be possible to add some detail about this?  "a particular
falvour of bad debug info" is pretty non-informative.

Thanks,

Mark

> 2008-09-19  Andrew Stubbs  <ams@codesourcery.com>
> 
> 	* frame.c (get_frame_register_bytes): Detect bad debug info.
> 
> Index: gdb/frame.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/frame.c,v
> retrieving revision 1.251
> diff -u -p -r1.251 frame.c
> --- gdb/frame.c	26 Aug 2008 17:40:24 -0000	1.251
> +++ gdb/frame.c	19 Sep 2008 18:10:34 -0000
> @@ -796,6 +796,8 @@ get_frame_register_bytes (struct frame_i
>  			  CORE_ADDR offset, int len, gdb_byte *myaddr)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (frame);
> +  int i;
> +  int maxsize;
>  
>    /* Skip registers wholly inside of OFFSET.  */
>    while (offset >= register_size (gdbarch, regnum))
> @@ -804,6 +806,22 @@ get_frame_register_bytes (struct frame_i
>        regnum++;
>      }
>  
> +  /* Detect bad debug info.  */
> +  maxsize = -offset;
> +  for (i = regnum; i < gdbarch_num_regs (gdbarch); i++)
> +    {
> +      int thissize = register_size (gdbarch, i);
> +      if (thissize == 0)
> +	break;
> +      maxsize += thissize;
> +    }
> +  if (len > maxsize)
> +    {
> +      warning (_("Bad debug information detected: "
> +		 "Attempt to read %d bytes from registers."), len);
> +      return 0;
> +    }
> +
>    /* Copy the data.  */
>    while (len > 0)
>      {
> 
> --------------060704060100080904030200--
> 

Re: [commited] Detect bad debug info

[Andrew Stubbs]

Mark Kettenis wrote:
> Would it be possible to add some detail about this?  "a particular
> falvour of bad debug info" is pretty non-informative.

Sorry, you've caught me out. :)

I don't really know in what way the debug info was bad. I only have a 
bad binary (no source code, no compiler). I suspect that it had marked 
an a array as being in registers, rather than a pointer to the array, 
but it doesn't really matter. What I do know is that the effect was that 
the debugger was attempting to read an impossibly large quantity of data 
from the register file. The result was a assertion failure and a bad 
user experience.

The patch ensures that the debugger never attempts to read beyond the 
end of the register file.

Note that the binary in question comes from a proprietary compiler, and 
I believe that that has now been fixed.

This change is merely a robustness enhancement.

If you would like more details, please ask.

Andrew Stubbs

P.S. Sorry for posting the patch to the wrong list. Finger trouble. :(

Re: [commited] Detect bad debug info

[Daniel Jacobowitz]

On Mon, Sep 22, 2008 at 10:41:29AM +0100, Andrew Stubbs wrote:
> Mark Kettenis wrote:
>> Would it be possible to add some detail about this?  "a particular
>> falvour of bad debug info" is pretty non-informative.
>
> Sorry, you've caught me out. :)
>
> I don't really know in what way the debug info was bad. I only have a bad 
> binary (no source code, no compiler). I suspect that it had marked an a 
> array as being in registers, rather than a pointer to the array, but it 
> doesn't really matter. What I do know is that the effect was that the 
> debugger was attempting to read an impossibly large quantity of data from 
> the register file. The result was a assertion failure and a bad user 
> experience.

You are correct.  Something was confused by the automatic decay of
arrays to pointers, and there was a DW_OP_reg0 instead of DW_OP_breg0
because of that.

> The patch ensures that the debugger never attempts to read beyond the end 
> of the register file.

Could you explain this in the code?  I think it's more enlightening
than "bad debug info".

-- 
Daniel Jacobowitz
CodeSourcery

Re: [commited] Detect bad debug info

[Andrew Stubbs]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

Daniel Jacobowitz wrote:
> On Mon, Sep 22, 2008 at 10:41:29AM +0100, Andrew Stubbs wrote:
>> The patch ensures that the debugger never attempts to read beyond the end 
>> of the register file.
> 
> Could you explain this in the code?  I think it's more enlightening
> than "bad debug info".

Very well, how about the attached?

Andrew

[-- Attachment #2: comment.patch --]
[-- Type: text/x-diff, Size: 929 bytes --]

2008-09-22  Andrew Stubbs  <ams@codesourcery.com>

	* frame.c (get_frame_register_bytes): Comment improvments.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.252
diff -u -p -r1.252 frame.c
--- frame.c	19 Sep 2008 18:12:17 -0000	1.252
+++ frame.c	22 Sep 2008 13:12:09 -0000
@@ -806,13 +806,14 @@ get_frame_register_bytes (struct frame_i
       regnum++;
     }
 
-  /* Detect bad debug info.  */
+  /* Ensure that we will not read beyond the end of the register file.
+     This can only ever happen if the debug information is bad.  */
   maxsize = -offset;
   for (i = regnum; i < gdbarch_num_regs (gdbarch); i++)
     {
       int thissize = register_size (gdbarch, i);
       if (thissize == 0)
-	break;
+	break;	/* This register is not available on this architecture.  */
       maxsize += thissize;
     }
   if (len > maxsize)

Re: [commited] Detect bad debug info

[Daniel Jacobowitz]

On Mon, Sep 22, 2008 at 02:12:44PM +0100, Andrew Stubbs wrote:
> Daniel Jacobowitz wrote:
>> On Mon, Sep 22, 2008 at 10:41:29AM +0100, Andrew Stubbs wrote:
>>> The patch ensures that the debugger never attempts to read beyond the 
>>> end of the register file.
>>
>> Could you explain this in the code?  I think it's more enlightening
>> than "bad debug info".
>
> Very well, how about the attached?

OK, thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [commited] Detect bad debug info

[Andrew Stubbs]

Daniel Jacobowitz wrote:
> OK, thanks.

Committed.

Re: [commited] Detect bad debug info

[Mark Kettenis]

> Date: Mon, 22 Sep 2008 14:12:44 +0100
> From: Andrew Stubbs <ams@codesourcery.com>
> 
> Daniel Jacobowitz wrote:
> > On Mon, Sep 22, 2008 at 10:41:29AM +0100, Andrew Stubbs wrote:
> >> The patch ensures that the debugger never attempts to read beyond the end 
> >> of the register file.
> > 
> > Could you explain this in the code?  I think it's more enlightening
> > than "bad debug info".
> 
> Very well, how about the attached?

Thanks, that defenitely removes some serious question marks for me.

> 2008-09-22  Andrew Stubbs  <ams@codesourcery.com>
> 
> 	* frame.c (get_frame_register_bytes): Comment improvments.
> 
> Index: frame.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/frame.c,v
> retrieving revision 1.252
> diff -u -p -r1.252 frame.c
> --- frame.c	19 Sep 2008 18:12:17 -0000	1.252
> +++ frame.c	22 Sep 2008 13:12:09 -0000
> @@ -806,13 +806,14 @@ get_frame_register_bytes (struct frame_i
>        regnum++;
>      }
>  
> -  /* Detect bad debug info.  */
> +  /* Ensure that we will not read beyond the end of the register file.
> +     This can only ever happen if the debug information is bad.  */
>    maxsize = -offset;
>    for (i = regnum; i < gdbarch_num_regs (gdbarch); i++)
>      {
>        int thissize = register_size (gdbarch, i);
>        if (thissize == 0)
> -	break;
> +	break;	/* This register is not available on this architecture.  */
>        maxsize += thissize;
>      }
>    if (len > maxsize)

Re: [commited] Detect bad debug info

[Andreas Schwab]

Andrew Stubbs <ams@codesourcery.com> writes:

> +  /* Detect bad debug info.  */
> +  maxsize = -offset;
> +  for (i = regnum; i < gdbarch_num_regs (gdbarch); i++)

This is broken, it completely ignores the pseudo regs, which badly
breaks ia64 and many other targets.  Committed as obvious.

Andreas.

2008-09-24  Andreas Schwab  <schwab@suse.de>

	* frame.c (get_frame_register_bytes): Take pseudo registers into
	account.

--- frame.c.~1.253.~	2008-09-23 11:46:45.000000000 +0200
+++ frame.c	2008-09-24 14:09:33.000000000 +0200
@@ -809,7 +809,8 @@ get_frame_register_bytes (struct frame_i
   /* Ensure that we will not read beyond the end of the register file.
      This can only ever happen if the debug information is bad.  */
   maxsize = -offset;
-  for (i = regnum; i < gdbarch_num_regs (gdbarch); i++)
+  for (i = regnum;
+       i < gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch); i++)
     {
       int thissize = register_size (gdbarch, i);
       if (thissize == 0)

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [commited] Detect bad debug info

[Andreas Schwab]

Andreas Schwab <schwab@suse.de> writes:

> Andrew Stubbs <ams@codesourcery.com> writes:
>
>> +  /* Detect bad debug info.  */
>> +  maxsize = -offset;
>> +  for (i = regnum; i < gdbarch_num_regs (gdbarch); i++)
>
> This is broken, it completely ignores the pseudo regs, which badly
> breaks ia64 and many other targets.  Committed as obvious.
>
> Andreas.
>
> 2008-09-24  Andreas Schwab  <schwab@suse.de>
>
> 	* frame.c (get_frame_register_bytes): Take pseudo registers into
> 	account.

Also checked this in to avoid useless function calls.

Andreas.

--- ChangeLog.~1.9844.~	2008-09-24 14:15:35.000000000 +0200
+++ ChangeLog	2008-09-24 14:54:30.000000000 +0200
@@ -1,7 +1,7 @@
 2008-09-24  Andreas Schwab  <schwab@suse.de>
 
 	* frame.c (get_frame_register_bytes): Take pseudo registers into
-	account.
+	account.  Avoid excessive function calls.
 
 2008-09-23  Doug Evans  <dje@google.com>
 
--- frame.c.~1.254.~	2008-09-24 14:09:33.000000000 +0200
+++ frame.c	2008-09-24 14:53:45.000000000 +0200
@@ -798,6 +798,7 @@ get_frame_register_bytes (struct frame_i
   struct gdbarch *gdbarch = get_frame_arch (frame);
   int i;
   int maxsize;
+  int numregs;
 
   /* Skip registers wholly inside of OFFSET.  */
   while (offset >= register_size (gdbarch, regnum))
@@ -809,8 +810,8 @@ get_frame_register_bytes (struct frame_i
   /* Ensure that we will not read beyond the end of the register file.
      This can only ever happen if the debug information is bad.  */
   maxsize = -offset;
-  for (i = regnum;
-       i < gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch); i++)
+  numregs = gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch);
+  for (i = regnum; i < numregs; i++)
     {
       int thissize = register_size (gdbarch, i);
       if (thissize == 0)

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."