RFA: Fix check for no-saved-pc

RFA: Fix check for no-saved-pc

[Michael Snyder]

There's a check in get_prev_frame to see if the next saved pc
is zero.  I think it has an off-by-one error, and is checking 
for the pc of the wrong frame.

To test this, I took the corefile that's generated by the
testsuite (gdb.base/corefile.exp), edited it with khexedit, 
and nulled out one of the saved return addresses on the stack.

This is the gdb output before my change:

Core was generated by `gdb/testsuite/gdb.base/coremaker'.
Program terminated with signal 6, Aborted.
#0  0x40000402 in __kernel_vsyscall ()
(gdb) bt
#0  0x40000402 in __kernel_vsyscall ()
#1  0x00a7bfa0 in raise () from /lib/libc.so.6
#2  0x00a7d8b1 in abort () from /lib/libc.so.6
#3  0x080486ce in func2 () at gdb/testsuite/gdb.base/coremaker.c:127
#4  0x080486d9 in func1 () at gdb/testsuite/gdb.base/coremaker.c:133
#5  0x00000000 in ?? ()
(gdb) 

And this is the output with the attached patch:

(gdb) bt
#0  0x40000402 in __kernel_vsyscall ()
#1  0x00a7bfa0 in raise () from /lib/libc.so.6
#2  0x00a7d8b1 in abort () from /lib/libc.so.6
#3  0x080486ce in func2 () at gdb/testsuite/gdb.base/coremaker.c:127
#4  0x080486d9 in func1 () at gdb/testsuite/gdb.base/coremaker.c:133
Backtrace stopped: frame did not save the PC
(gdb) 

No other test suite changes, tests run on native i386 linux.


2007-11-30  Michael Snyder  <msnyder@specifix.com>

	* frame.c (get_prev_frame): Remove unused local variable.
	(get_prev_frame): Check for null saved pc in the following
	("prev") frame, not the current (already valid) frame.
	Set up stop_reason conditions for printing stop reason.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.235
diff -u -p -r1.235 frame.c
--- frame.c	2 Nov 2007 14:47:27 -0000	1.235
+++ frame.c	30 Nov 2007 22:48:47 -0000
@@ -1355,8 +1355,6 @@ inside_entry_func (struct frame_info *th
 struct frame_info *
 get_prev_frame (struct frame_info *this_frame)
 {
-  struct frame_info *prev_frame;
-
   /* Return the inner-most frame, when the caller passes in NULL.  */
   /* NOTE: cagney/2002-11-09: Not sure how this would happen.  The
      caller should have previously obtained a valid frame using
@@ -1469,9 +1467,12 @@ get_prev_frame (struct frame_info *this_
   if (this_frame->level > 0
       && get_frame_type (this_frame) == NORMAL_FRAME
       && get_frame_type (get_next_frame (this_frame)) == NORMAL_FRAME
-      && get_frame_pc (this_frame) == 0)
+      && frame_pc_unwind (this_frame) == 0)
     {
       frame_debug_got_null_frame (gdb_stdlog, this_frame, "zero PC");
+      this_frame->stop_reason = UNWIND_NO_SAVED_PC;
+      this_frame->prev_p = 1;
+      this_frame->prev = NULL;
       return NULL;
     }
 

Re: RFA: Fix check for no-saved-pc

[Daniel Jacobowitz]

On Fri, Nov 30, 2007 at 02:40:31PM -0800, Michael Snyder wrote:
> There's a check in get_prev_frame to see if the next saved pc
> is zero.  I think it has an off-by-one error, and is checking 
> for the pc of the wrong frame.

Mark K. and I have had roughly a month's worth of discussion on this
check over the last two years; it's where it is on purpose.  Here's
the last conversation:

  http://sourceware.org/ml/gdb-patches/2006-05/msg00196.html
  http://sourceware.org/ml/gdb-patches/2006-07/msg00296.html

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFA: Fix check for no-saved-pc

[Michael Snyder]

On Fri, 2007-11-30 at 18:00 -0500, Daniel Jacobowitz wrote:
> On Fri, Nov 30, 2007 at 02:40:31PM -0800, Michael Snyder wrote:
> > There's a check in get_prev_frame to see if the next saved pc
> > is zero.  I think it has an off-by-one error, and is checking 
> > for the pc of the wrong frame.
> 
> Mark K. and I have had roughly a month's worth of discussion on this
> check over the last two years; it's where it is on purpose.  Here's
> the last conversation:
> 
>   http://sourceware.org/ml/gdb-patches/2006-05/msg00196.html
>   http://sourceware.org/ml/gdb-patches/2006-07/msg00296.html

All right.  Then let's leave that test alone, and add another
test, much later on, to detect and report this situation.

Here's my revised patch.  Testsuites un-affected.

As before, the effect of this change is to have gdb print
a more informative message instead of a meaningless zero-frame.

2007-11-30  Michael Snyder  <msnyder@specifix.com>

	* frame.c (get_prev_frame): Remove unused local variable.
	(get_prev_frame_1): Check for null saved pc in the calling 
	frame.  Set up stop_reason conditions for printing stop reason.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.235
diff -u -p -r1.235 frame.c
--- frame.c	2 Nov 2007 14:47:27 -0000	1.235
+++ frame.c	1 Dec 2007 01:30:01 -0000
@@ -1248,6 +1248,16 @@ get_prev_frame_1 (struct frame_info *thi
 	  this_frame->prev = NULL;
 	  return NULL;
 	}
+      /* Also check for a null saved_pc.  At this point it can 
+	 only be a bad thing.  */
+      if (frame_pc_unwind (this_frame) == 0)
+	{
+	  frame_debug_got_null_frame (gdb_stdlog, this_frame, 
+				      "zero PC");
+	  this_frame->stop_reason = UNWIND_NO_SAVED_PC;
+	  this_frame->prev = NULL;
+	  return NULL;
+	}
     }
 
   /* Allocate the new frame but do not wire it in to the frame chain.
@@ -1355,8 +1365,6 @@ inside_entry_func (struct frame_info *th
 struct frame_info *
 get_prev_frame (struct frame_info *this_frame)
 {
-  struct frame_info *prev_frame;
-
   /* Return the inner-most frame, when the caller passes in NULL.  */
   /* NOTE: cagney/2002-11-09: Not sure how this would happen.  The
      caller should have previously obtained a valid frame using

Re: RFA: Fix check for no-saved-pc

[Michael Snyder]

Mark, is the 2nd form of the patch acceptable to you?

On Fri, 2007-11-30 at 17:37 -0800, Michael Snyder wrote:
> On Fri, 2007-11-30 at 18:00 -0500, Daniel Jacobowitz wrote:
> > On Fri, Nov 30, 2007 at 02:40:31PM -0800, Michael Snyder wrote:
> > > There's a check in get_prev_frame to see if the next saved pc
> > > is zero.  I think it has an off-by-one error, and is checking 
> > > for the pc of the wrong frame.
> > 
> > Mark K. and I have had roughly a month's worth of discussion on this
> > check over the last two years; it's where it is on purpose.  Here's
> > the last conversation:
> > 
> >   http://sourceware.org/ml/gdb-patches/2006-05/msg00196.html
> >   http://sourceware.org/ml/gdb-patches/2006-07/msg00296.html
> 
> All right.  Then let's leave that test alone, and add another
> test, much later on, to detect and report this situation.
> 
> Here's my revised patch.  Testsuites un-affected.
> 
> As before, the effect of this change is to have gdb print
> a more informative message instead of a meaningless zero-frame.
> 
> 2007-11-30  Michael Snyder  <msnyder@specifix.com>
> 
> 	* frame.c (get_prev_frame): Remove unused local variable.
> 	(get_prev_frame_1): Check for null saved pc in the calling 
> 	frame.  Set up stop_reason conditions for printing stop reason.
> 
> Index: frame.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/frame.c,v
> retrieving revision 1.235
> diff -u -p -r1.235 frame.c
> --- frame.c	2 Nov 2007 14:47:27 -0000	1.235
> +++ frame.c	1 Dec 2007 01:30:01 -0000
> @@ -1248,6 +1248,16 @@ get_prev_frame_1 (struct frame_info *thi
>  	  this_frame->prev = NULL;
>  	  return NULL;
>  	}
> +      /* Also check for a null saved_pc.  At this point it can 
> +	 only be a bad thing.  */
> +      if (frame_pc_unwind (this_frame) == 0)
> +	{
> +	  frame_debug_got_null_frame (gdb_stdlog, this_frame, 
> +				      "zero PC");
> +	  this_frame->stop_reason = UNWIND_NO_SAVED_PC;
> +	  this_frame->prev = NULL;
> +	  return NULL;
> +	}
>      }
>  
>    /* Allocate the new frame but do not wire it in to the frame chain.
> @@ -1355,8 +1365,6 @@ inside_entry_func (struct frame_info *th
>  struct frame_info *
>  get_prev_frame (struct frame_info *this_frame)
>  {
> -  struct frame_info *prev_frame;
> -
>    /* Return the inner-most frame, when the caller passes in NULL.  */
>    /* NOTE: cagney/2002-11-09: Not sure how this would happen.  The
>       caller should have previously obtained a valid frame using
> 
> 

Re: RFA: Fix check for no-saved-pc

[Mark Kettenis]

> From: Michael Snyder <msnyder@specifix.com>
> Date: Fri, 30 Nov 2007 17:37:24 -0800
> 
> On Fri, 2007-11-30 at 18:00 -0500, Daniel Jacobowitz wrote:
> > On Fri, Nov 30, 2007 at 02:40:31PM -0800, Michael Snyder wrote:
> > > There's a check in get_prev_frame to see if the next saved pc
> > > is zero.  I think it has an off-by-one error, and is checking 
> > > for the pc of the wrong frame.
> > 
> > Mark K. and I have had roughly a month's worth of discussion on this
> > check over the last two years; it's where it is on purpose.  Here's
> > the last conversation:
> > 
> >   http://sourceware.org/ml/gdb-patches/2006-05/msg00196.html
> >   http://sourceware.org/ml/gdb-patches/2006-07/msg00296.html
> 
> All right.  Then let's leave that test alone, and add another
> test, much later on, to detect and report this situation.
> 
> Here's my revised patch.  Testsuites un-affected.
> 
> As before, the effect of this change is to have gdb print
> a more informative message instead of a meaningless zero-frame.

It's not meaningless; it's a very valuabl hint that the stack has been
corrupted.  You'll get very similar output if youd chosen some random
value instead of zero when you corrupted the stack.  Why is zero special?

If your answer to that question is something like: "because on arm the
outermost frame gets marked by a zero pc", then please implement
proper backtrace termination for that case in the arm unwinder.

Mark

Re: RFA: Fix check for no-saved-pc

[Michael Snyder]

On Tue, 2007-12-04 at 19:05 +0100, Mark Kettenis wrote:
> > From: Michael Snyder <msnyder@specifix.com>
> > Date: Fri, 30 Nov 2007 17:37:24 -0800
> > 
> > On Fri, 2007-11-30 at 18:00 -0500, Daniel Jacobowitz wrote:
> > > On Fri, Nov 30, 2007 at 02:40:31PM -0800, Michael Snyder wrote:
> > > > There's a check in get_prev_frame to see if the next saved pc
> > > > is zero.  I think it has an off-by-one error, and is checking 
> > > > for the pc of the wrong frame.
> > > 
> > > Mark K. and I have had roughly a month's worth of discussion on this
> > > check over the last two years; it's where it is on purpose.  Here's
> > > the last conversation:
> > > 
> > >   http://sourceware.org/ml/gdb-patches/2006-05/msg00196.html
> > >   http://sourceware.org/ml/gdb-patches/2006-07/msg00296.html
> > 
> > All right.  Then let's leave that test alone, and add another
> > test, much later on, to detect and report this situation.
> > 
> > Here's my revised patch.  Testsuites un-affected.
> > 
> > As before, the effect of this change is to have gdb print
> > a more informative message instead of a meaningless zero-frame.
> 
> It's not meaningless; it's a very valuable hint that the stack has been
> corrupted. 

My poor choice of words.  What I meant was more like, one is a
"hint" and the other is an explicit statement.  A person does
not need to know what this hint means if gdb tells them
explicitly.


>  You'll get very similar output if youd chosen some random
> value instead of zero when you corrupted the stack.  Why is zero special?

A good question, and the only answer I can give is "it just is".
You are right, this is a special case -- but it seems, in practice,
to be one that comes up fairly frequently, eg. when a wild pointer
or out of bounds array writes zeroes over a region of stack.

Why zero and not some other value?  Just because it is fairly
common to write zeroes into something, I guess.  The real 
answer is because we or our users see it a lot.


> If your answer to that question is something like: "because on arm the
> outermost frame gets marked by a zero pc", 

No, that's not it at all.  This has nothing to do with any
particular architecture (although some may or may not be
more vulnerable to it than others).  This has nothing to 
do with any deliberate attempt to "mark" the outermost frame.

OTOH, one context in which it does come up is when there
has been no attempt at all to mark the outermost frame -- 
as eg. the first frame of a new thread -- and the frame's
memory comes up full of zeros (either because that's the
default value for un-initialized memory, or because the
thread library initializes it to zero).

GDB looks where the function prologue says the return address
should be stored, and finds zero.

Re: RFA: Fix check for no-saved-pc

[Michael Snyder]

Ping?

On Tue, 2007-12-04 at 15:11 -0800, Michael Snyder wrote:
> On Tue, 2007-12-04 at 19:05 +0100, Mark Kettenis wrote:
> > > From: Michael Snyder <msnyder@specifix.com>
> > > Date: Fri, 30 Nov 2007 17:37:24 -0800
> > > 
> > > On Fri, 2007-11-30 at 18:00 -0500, Daniel Jacobowitz wrote:
> > > > On Fri, Nov 30, 2007 at 02:40:31PM -0800, Michael Snyder wrote:
> > > > > There's a check in get_prev_frame to see if the next saved pc
> > > > > is zero.  I think it has an off-by-one error, and is checking 
> > > > > for the pc of the wrong frame.
> > > > 
> > > > Mark K. and I have had roughly a month's worth of discussion on this
> > > > check over the last two years; it's where it is on purpose.  Here's
> > > > the last conversation:
> > > > 
> > > >   http://sourceware.org/ml/gdb-patches/2006-05/msg00196.html
> > > >   http://sourceware.org/ml/gdb-patches/2006-07/msg00296.html
> > > 
> > > All right.  Then let's leave that test alone, and add another
> > > test, much later on, to detect and report this situation.
> > > 
> > > Here's my revised patch.  Testsuites un-affected.
> > > 
> > > As before, the effect of this change is to have gdb print
> > > a more informative message instead of a meaningless zero-frame.
> > 
> > It's not meaningless; it's a very valuable hint that the stack has been
> > corrupted. 
> 
> My poor choice of words.  What I meant was more like, one is a
> "hint" and the other is an explicit statement.  A person does
> not need to know what this hint means if gdb tells them
> explicitly.
> 
> 
> >  You'll get very similar output if youd chosen some random
> > value instead of zero when you corrupted the stack.  Why is zero special?
> 
> A good question, and the only answer I can give is "it just is".
> You are right, this is a special case -- but it seems, in practice,
> to be one that comes up fairly frequently, eg. when a wild pointer
> or out of bounds array writes zeroes over a region of stack.
> 
> Why zero and not some other value?  Just because it is fairly
> common to write zeroes into something, I guess.  The real 
> answer is because we or our users see it a lot.
> 
> 
> > If your answer to that question is something like: "because on arm the
> > outermost frame gets marked by a zero pc", 
> 
> No, that's not it at all.  This has nothing to do with any
> particular architecture (although some may or may not be
> more vulnerable to it than others).  This has nothing to 
> do with any deliberate attempt to "mark" the outermost frame.
> 
> OTOH, one context in which it does come up is when there
> has been no attempt at all to mark the outermost frame -- 
> as eg. the first frame of a new thread -- and the frame's
> memory comes up full of zeros (either because that's the
> default value for un-initialized memory, or because the
> thread library initializes it to zero).
> 
> GDB looks where the function prologue says the return address
> should be stored, and finds zero.
> 
> 
> 
> 

Re: RFA: Fix check for no-saved-pc

[Mark Kettenis]

> From: Michael Snyder <msnyder@specifix.com>
> Date: Wed, 12 Dec 2007 11:05:23 -0800
> 
> Ping?

Bleah, I tried to find a window of a bit more than 15 minutes to think
about a reply for a week and a half and failed :(.

> > > It's not meaningless; it's a very valuable hint that the stack has been
> > > corrupted. 
> > 
> > My poor choice of words.  What I meant was more like, one is a
> > "hint" and the other is an explicit statement.  A person does
> > not need to know what this hint means if gdb tells them
> > explicitly.

It doesn't really add much more information:

  ? 0x00000000 foo

isn't really less explicit than

  Saved pc is zero

and words have a much bigger tendency to be ambiguous than factual
information.  That said, I don't really object to explicitly stating
why we are terminating the backtrace, as long as the zero-pc
stackframe is still visible and accessable.  And we need to complain
somewhat loudly otherwise people won't even notice something is wrong
and won't fix the buffer overflows in their broken code.

> > >  You'll get very similar output if youd chosen some random
> > > value instead of zero when you corrupted the stack.  Why is zero special?
> > 
> > A good question, and the only answer I can give is "it just is".
> > You are right, this is a special case -- but it seems, in practice,
> > to be one that comes up fairly frequently, eg. when a wild pointer
> > or out of bounds array writes zeroes over a region of stack.
> > 
> > Why zero and not some other value?  Just because it is fairly
> > common to write zeroes into something, I guess.  The real 
> > answer is because we or our users see it a lot.

Well, I guess a common failure scenario could be using memset() on a
buffer allocated on the stack and specifying the wrong size.  But in
my experience other values are just as common.  People probably just
don't notice because in that case we continue the backtrace until we
eventually hit zero.

> > OTOH, one context in which it does come up is when there
> > has been no attempt at all to mark the outermost frame -- 
> > as eg. the first frame of a new thread -- and the frame's
> > memory comes up full of zeros (either because that's the
> > default value for un-initialized memory, or because the
> > thread library initializes it to zero).

Yes, if you fall off the stack, I can imagine you hit zeroes much more
often.  I've repeatedly stated that people should fix their threading
libraries to explicitly mark the end of the stack such that this
doesn't happen, or that we should change GDB such that we terminate
the stack at the thread entry point, much like we do for main().  I
really don't want to cripple GDB because people think that's too
difficult.

Yes, I think your diff cripple GDB.  The zero-pc frame might have
useful information that could help me track down the memory
corruption.

> > GDB looks where the function prologue says the return address
> > should be stored, and finds zero.

Or GDB might just be confused and look in the wrong place.

Re: RFA: Fix check for no-saved-pc

[Michael Snyder]

On Sun, 2007-12-16 at 21:06 +0100, Mark Kettenis wrote:
> > From: Michael Snyder <msnyder@specifix.com>
> > Date: Wed, 12 Dec 2007 11:05:23 -0800
> > 
> > Ping?
> 
> Bleah, I tried to find a window of a bit more than 15 minutes to think
> about a reply for a week and a half and failed :(.
> 
> > > > It's not meaningless; it's a very valuable hint that the stack has been
> > > > corrupted. 
> > > 
> > > My poor choice of words.  What I meant was more like, one is a
> > > "hint" and the other is an explicit statement.  A person does
> > > not need to know what this hint means if gdb tells them
> > > explicitly.
> 
> It doesn't really add much more information:
> 
>   ? 0x00000000 foo
> 
> isn't really less explicit than
> 
>   Saved pc is zero

Well sure it is -- if you're a naive user. 


> Yes, if you fall off the stack, I can imagine you hit zeroes much more
> often.  I've repeatedly stated that people should fix their threading
> libraries to explicitly mark the end of the stack such that this
> doesn't happen, or that we should change GDB such that we terminate
> the stack at the thread entry point, much like we do for main().  I
> really don't want to cripple GDB because people think that's too
> difficult.

We don't have control over those thread libraries, so 
all we can do is (1) ask them, and (2) try to put work
arounds to prevent bogus behavior in gdb.  I don't think
it's wrong to do that.  

Cripple gdb?  It's only crippling gdb if you think that
a saved PC of zero is a legitimate possibility.  I think
that possibility is remote beyond being worthy of consideration.


> Yes, I think your diff cripple GDB.  The zero-pc frame might have
> useful information that could help me track down the memory
> corruption.

Well, then it's really a conflict of interests between the
naive user, and the user who is sophisticated enough to 
(a) know what a stack frame of 0x00000000 implies, and
(b) know how to extract info from a blown stack frame.

I think the naive users outnumber the ones with that
level of sophistication by at least 100's to one, if
not 1000's to one.  We really need to weigh their 
interests a little more heavily.