diff mbox

[RFC] gdb/arm-tdep.c: Add sanity check on fp before trying to, access memory.

Message ID 5195EE16.9020700@linaro.org
State Rejected
Headers show

Commit Message

Will Newton May 17, 2013, 8:45 a.m. UTC 
Add a sanity check on the frame pointer before trying to access memory. The
check aims to prevent a "Cannot access memory at address" error being printed
when the frame pointer is zero or otherwise below the current sp. This only
affects the case where no symbols are available and has been seen with the
KVM debug stub. The frame pointer is read every time the frame_id is
requested so something as simple as "print $pc" can cause an error to be
printed.

No new testsuite failures configured with armv7l-unknown-linux-gnueabihf.

gdb/ChangeLog:

2013-05-17  Will Newton  <will.newton@linaro.org>

	* arm-tdep.c (arm_scan_prologue): Check the frame pointer looks
	valid by comparing to current sp before reading it.
---
 gdb/arm-tdep.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Will Newton May 24, 2013, 2:32 p.m. UTC | #1 
Ping?

I'm not sure if this is the right approach to the problem, but if
anyone has any better ideas I would be interested.

Thanks,

On 17 May 2013 09:45, Will Newton <will.newton@linaro.org> wrote:
>
> Add a sanity check on the frame pointer before trying to access memory. The
> check aims to prevent a "Cannot access memory at address" error being printed
> when the frame pointer is zero or otherwise below the current sp. This only
> affects the case where no symbols are available and has been seen with the
> KVM debug stub. The frame pointer is read every time the frame_id is
> requested so something as simple as "print $pc" can cause an error to be
> printed.
>
> No new testsuite failures configured with armv7l-unknown-linux-gnueabihf.
>
> gdb/ChangeLog:
>
> 2013-05-17  Will Newton  <will.newton@linaro.org>
>
>         * arm-tdep.c (arm_scan_prologue): Check the frame pointer looks
>         valid by comparing to current sp before reading it.
> ---
>  gdb/arm-tdep.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index b169e35..162aea8 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -1979,10 +1979,15 @@ arm_scan_prologue (struct frame_info *this_frame,
>          Then, we can find the value of our frame pointer on entrance to
>          the callee (or at the present moment if this is the innermost frame).
>          The value stored there should be the address of the stmfd + 8.  */
> -      CORE_ADDR frame_loc;
> +      CORE_ADDR frame_loc, current_sp;
>        LONGEST return_value;
>
>        frame_loc = get_frame_register_unsigned (this_frame, ARM_FP_REGNUM);
> +      current_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> +
> +      /* Sanity check frame pointer.  */
> +      if (frame_loc < current_sp)
> +       return;
>        if (!safe_read_memory_integer (frame_loc, 4, byte_order, &return_value))
>          return;
>        else
> --
> 1.8.1.4
>



--
Will Newton
Toolchain Working Group, Linaro
Joel Brobecker May 27, 2013, 9:46 a.m. UTC | #2 
> I'm not sure if this is the right approach to the problem, but if
> anyone has any better ideas I would be interested.

As you said, I suspect that this is not the right place for fixing
the problem. I think something wrong happened, and my first instinct
is that this should be caught earlier.

We'd need more info as to why you get there...
Will Newton May 27, 2013, 1:07 p.m. UTC | #3 
On 27 May 2013 10:46, Joel Brobecker <brobecker@adacore.com> wrote:

Hi Joel,

>> I'm not sure if this is the right approach to the problem, but if
>> anyone has any better ideas I would be interested.
>
> As you said, I suspect that this is not the right place for fixing
> the problem. I think something wrong happened, and my first instinct
> is that this should be caught earlier.
>
> We'd need more info as to why you get there...

The call stack to get here is:

#2 0x001c69c2 in read_memory_unsigned_integer (memaddr=936, len=4,
byte_order=BFD_ENDIAN_LITTLE) at /root/gdb-7.5/gdb/corefile.c:312
#3 0x0004c3ea in arm_analyze_prologue (gdbarch=0x4c5690,
prologue_start=936, prologue_end=1000, cache=0x4c9950) at
/root/gdb-7.5/gdb/arm-tdep.c:1695
#4 0x0004cf1e in arm_scan_prologue (this_frame=0x3f9cf0,
cache=0x4c9950) at /root/gdb-7.5/gdb/arm-tdep.c:1998
#5 0x0004cf4c in arm_make_prologue_cache (this_frame=0x3f9cf0) at
/root/gdb-7.5/gdb/arm-tdep.c:2011
#6 0x0004cffe in arm_prologue_this_id (this_frame=0x3f9cf0,
this_cache=0x3f9cfc, this_id=0x3f9d20) at
/root/gdb-7.5/gdb/arm-tdep.c:2041
#7 0x0021ee44 in get_frame_id (fi=0x3f9cf0) at /root/gdb-7.5/gdb/frame.c:338
#8 0x00108c80 in value_of_register (regnum=15, frame=0x3f9cf0) at
/root/gdb-7.5/gdb/findvar.c:298

There are no symbols available (this is the KVM stub) so
find_pc_partial_function fails, which drops us into the else in
arm_scan_prologue. At which point the code tries to unwind using the
frame pointer. However there is no guarantee that the frame pointer is
valid here as far as I can tell - in the case I have seen it is 0, but
it could be any value.

The worst case is the frame pointer is not valid and we get an error
printed. This is not the end of the world, but seems a bit excessive
when all we tried to do was "print $pc".

--
Will Newton
Toolchain Working Group, Linaro
Luis Machado May 27, 2013, 1:33 p.m. UTC | #4 
On 05/27/2013 03:07 PM, Will Newton wrote:
> On 27 May 2013 10:46, Joel Brobecker <brobecker@adacore.com> wrote:
>
> Hi Joel,
>
>>> I'm not sure if this is the right approach to the problem, but if
>>> anyone has any better ideas I would be interested.
>>
>> As you said, I suspect that this is not the right place for fixing
>> the problem. I think something wrong happened, and my first instinct
>> is that this should be caught earlier.
>>
>> We'd need more info as to why you get there...
>
> The call stack to get here is:
>
> #2 0x001c69c2 in read_memory_unsigned_integer (memaddr=936, len=4,
> byte_order=BFD_ENDIAN_LITTLE) at /root/gdb-7.5/gdb/corefile.c:312
> #3 0x0004c3ea in arm_analyze_prologue (gdbarch=0x4c5690,
> prologue_start=936, prologue_end=1000, cache=0x4c9950) at
> /root/gdb-7.5/gdb/arm-tdep.c:1695
> #4 0x0004cf1e in arm_scan_prologue (this_frame=0x3f9cf0,
> cache=0x4c9950) at /root/gdb-7.5/gdb/arm-tdep.c:1998
> #5 0x0004cf4c in arm_make_prologue_cache (this_frame=0x3f9cf0) at
> /root/gdb-7.5/gdb/arm-tdep.c:2011
> #6 0x0004cffe in arm_prologue_this_id (this_frame=0x3f9cf0,
> this_cache=0x3f9cfc, this_id=0x3f9d20) at
> /root/gdb-7.5/gdb/arm-tdep.c:2041
> #7 0x0021ee44 in get_frame_id (fi=0x3f9cf0) at /root/gdb-7.5/gdb/frame.c:338
> #8 0x00108c80 in value_of_register (regnum=15, frame=0x3f9cf0) at
> /root/gdb-7.5/gdb/findvar.c:298
>
> There are no symbols available (this is the KVM stub) so
> find_pc_partial_function fails, which drops us into the else in
> arm_scan_prologue. At which point the code tries to unwind using the
> frame pointer. However there is no guarantee that the frame pointer is
> valid here as far as I can tell - in the case I have seen it is 0, but
> it could be any value.
>
> The worst case is the frame pointer is not valid and we get an error
> printed. This is not the end of the world, but seems a bit excessive
> when all we tried to do was "print $pc".

Sounds like the stack of unwinders needs to be improved. Maybe a 
unwinder designed to deal with invalid frame pointers, preventing memory 
accesses that may be obviously wrong.

Luis
diff mbox

Patch

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index b169e35..162aea8 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -1979,10 +1979,15 @@  arm_scan_prologue (struct frame_info *this_frame,
 	 Then, we can find the value of our frame pointer on entrance to
 	 the callee (or at the present moment if this is the innermost frame).
 	 The value stored there should be the address of the stmfd + 8.  */
-      CORE_ADDR frame_loc;
+      CORE_ADDR frame_loc, current_sp;
       LONGEST return_value;

       frame_loc = get_frame_register_unsigned (this_frame, ARM_FP_REGNUM);
+      current_sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+
+      /* Sanity check frame pointer.  */
+      if (frame_loc < current_sp)
+	return;
       if (!safe_read_memory_integer (frame_loc, 4, byte_order, &return_value))
         return;
       else