[PATCH 0/2] Memory corruption caused by failure to notice architecture change.

[PATCH 0/2] Memory corruption caused by failure to notice architecture change.

[Andrew Burgess]

Turns out that in a particular use case insight does not spot a change
of architecture, and then starts corrupting memory by accessing off the
end of the register cache.

I've got two patches,

[1/2] - Adds an assert that would detect this bug.

[2/2] - My attempt at a fix.  I've only had a quick look over the tcl
code, (and I'm no tcl expert), so I suspect some feedback on this one.

Cheers,
Andrew

[PATCH 1/2] Add an assert that we're not overflowing the register cache.

[Andrew Burgess]

I'm running on x86-64 linux, and I'm building with --enable-targets=all
(not sure if this makes a difference).  As I start up insight the
setup_architecture_data method is called, and this allocates a register
cache, however, at this point, without know what target binary will be
loaded the exact number of target registers is unknown.

Next I load up a target executable, and run to main.

Finally, I open up the register window.  At this point an attempt is
made to access the register cache for all available registers, however,
the number of registers has grown as a result of loading a target
executable, insight then proceeds to corrupt a random area of memory
off the end of the register cache.

This patch doesn't fix the issue, but adds an assert to detect the bug.

OK to apply?

Thanks,
Andrew

gdb/gdbtk/ChangeLog

2013-09-05  Andrew Burgess  <aburgess@broadcom.com>
 
	* generic/gdbk-register.c (old_regs_count): New variable.
	(register_changed_p): Add new assert.
	(setup_architecture_data): Initialise old_regs_count.
 
Index: ./gdb/gdbtk/generic/gdbtk-register.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtk/generic/gdbtk-register.c,v
retrieving revision 1.48
diff -u -p -r1.48 gdbtk-register.c
--- ./gdb/gdbtk/generic/gdbtk-register.c	5 Sep 2013 13:14:28 -0000	1.48
+++ ./gdb/gdbtk/generic/gdbtk-register.c	5 Sep 2013 13:46:08 -0000
@@ -63,6 +63,7 @@ static void get_register_types (int regn
    It is an array of (NUM_REGS+NUM_PSEUDO_REGS)*MAX_REGISTER_RAW_SIZE bytes. */
 
 static char *old_regs = NULL;
+static int old_regs_count = 0;
 static int *regformat = (int *)NULL;
 static struct type **regtype = (struct type **)NULL;
 
@@ -443,6 +444,7 @@ static void
 register_changed_p (int regnum, map_arg arg)
 {
   gdb_byte raw_buffer[MAX_REGISTER_SIZE];
+  gdb_assert (regnum < old_regs_count);
 
   if (!target_has_registers
       || !deprecated_frame_register_read (get_selected_frame (NULL), regnum,
@@ -472,6 +474,7 @@ setup_architecture_data (void)
 
   numregs = (gdbarch_num_regs (get_current_arch ())
 	     + gdbarch_num_pseudo_regs (get_current_arch ()));
+  old_regs_count = numregs;
   old_regs = xcalloc (1, numregs * MAX_REGISTER_SIZE + 1);
   regformat = (int *)xcalloc (numregs, sizeof(int));
   regtype = (struct type **)xcalloc (numregs, sizeof(struct type **));

[PATCH 2/2] Notice architecture changes even when the register window is not open.

[Andrew Burgess]

The register cache can be accessed through the TCL proc "gdb_reginfo",
which can be called from anywhere, there's currently only one other
caller in "actiondlg.tcl", but more callers could easily be added.  I
believe that currently any caller to gdb_reginfo needs to (a) know if
they are accessing the reg-cache, and (b) if they are listen for arch
changed events and update the reg-cache.  I think this a bad situation
to be in as, the reg-cache would ideally be hidden from the user of
"gdb_reginfo", and secondly, we only really need to update the reg-cache
once per architecture change, not many times, which could happen if many
users are all trying to keep the reg-cache upto date.

Phew! So, the patch below moves the call to "gdb_reg_arch_changed" into
the top-level tcl event handler, we need to ensure that it gets called
before any of the event-handling windows see the architecture change
event as they might access the reg-cache.  I could in-theory have moved
the call to "gdb_reg_arch_changed" into the top-level window event
handler, I didn't do this for two reasons, (1) I didn't know if the top
level window always exists, though I suspect it does, but really (2) I
didn't know if there was any control on the order the windows receive
the events...

Anyway, let me know what you think, all feedback welcome.

Thanks,
Andrew

gdb/gdbtk/ChangeLog

2013-09-05  Andrew Burgess  <aburgess@broadcom.com>

	* library/interface.tcl (gdbtk_tcl_architecture_changed): Add call
	to gdb_reg_arch_changed.
	* library/regwin.itb (arch_changed): Remove call to
	gdb_reg_arch_changed.

Index: ./gdb/gdbtk/library/interface.tcl
===================================================================
RCS file: /cvs/src/src/gdb/gdbtk/library/interface.tcl,v
retrieving revision 1.60
diff -u -p -r1.60 interface.tcl
--- ./gdb/gdbtk/library/interface.tcl	9 Oct 2009 01:23:55 -0000	1.60
+++ ./gdb/gdbtk/library/interface.tcl	5 Sep 2013 14:19:53 -0000
@@ -1815,6 +1815,9 @@ proc initialize_gdbtk {} {
 # The architecture changed. Inform the UI.
 proc gdbtk_tcl_architecture_changed {} {
   set e [ArchChangedEvent \#auto]
+  # First perform global actions as a result of the architecture change.
+  gdb_reg_arch_changed $e 
+  # Now dispatch to all the other even handlers.
   GDBEventHandler::dispatch $e
   delete object $e
 }
Index: ./gdb/gdbtk/library/regwin.itb
===================================================================
RCS file: /cvs/src/src/gdb/gdbtk/library/regwin.itb,v
retrieving revision 1.30
diff -u -p -r1.30 regwin.itb
--- ./gdb/gdbtk/library/regwin.itb	25 May 2012 10:34:32 -0000	1.30
+++ ./gdb/gdbtk/library/regwin.itb	5 Sep 2013 14:19:53 -0000
@@ -932,9 +932,6 @@ itcl::body RegWin::_select_group {} {
 # ------------------------------------------------------------------
 itcl::body RegWin::arch_changed {event} {
 
-  # Update internal register caches
-  gdb_reg_arch_changed
-
   # Relayout the table
   _layout_table
 

Re: [PATCH 1/2] Add an assert that we're not overflowing the register cache.

[Keith Seitz]

On 09/05/2013 07:26 AM, Andrew Burgess wrote:
> This patch doesn't fix the issue, but adds an assert to detect the bug.

If I understand correctly, the next patch fixes the actual problem, but 
this is added in case it happens again. Yes?

 > OK to apply?

Yes. Thanks!

Keith

Re: [PATCH 2/2] Notice architecture changes even when the register window is not open.

[Keith Seitz]

On 09/05/2013 07:27 AM, Andrew Burgess wrote:
> I think this a bad situation to be in as, the reg-cache would ideally
> be hidden from the user of "gdb_reginfo", and secondly, we only
> really need to update the reg-cache once per architecture change, not
> many times, which could happen if many users are all trying to keep
> the reg-cache upto date.

Ouch. I believe at the time, this was never considered. I'm not even 
sure if it was possible. Nonetheless, it clearly *is* possible now, so 
thank you for tracking this down.

BTW, what architecture are you using to trigger this bug?

> Anyway, let me know what you think, all feedback welcome.

One little comment below.

> Index: ./gdb/gdbtk/library/interface.tcl
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbtk/library/interface.tcl,v
> retrieving revision 1.60
> diff -u -p -r1.60 interface.tcl
> --- ./gdb/gdbtk/library/interface.tcl	9 Oct 2009 01:23:55 -0000	1.60
> +++ ./gdb/gdbtk/library/interface.tcl	5 Sep 2013 14:19:53 -0000
> @@ -1815,6 +1815,9 @@ proc initialize_gdbtk {} {
>   # The architecture changed. Inform the UI.
>   proc gdbtk_tcl_architecture_changed {} {
>     set e [ArchChangedEvent \#auto]
> +  # First perform global actions as a result of the architecture change.
> +  gdb_reg_arch_changed $e
> +  # Now dispatch to all the other even handlers.

typo ("even[t] handlers")

>     GDBEventHandler::dispatch $e
>     delete object $e
>   }

Okay with that change.

Keith

Re: [PATCH 2/2] Notice architecture changes even when the register window is not open.

[Andrew Burgess]

On 05/09/2013 8:00 PM, Keith Seitz wrote:
> On 09/05/2013 07:27 AM, Andrew Burgess wrote:
>> I think this a bad situation to be in as, the reg-cache would ideally
>> be hidden from the user of "gdb_reginfo", and secondly, we only
>> really need to update the reg-cache once per architecture change, not
>> many times, which could happen if many users are all trying to keep
>> the reg-cache upto date.
> 
> Ouch. I believe at the time, this was never considered. I'm not even
> sure if it was possible. Nonetheless, it clearly *is* possible now, so
> thank you for tracking this down.
> 
> BTW, what architecture are you using to trigger this bug?

"Just" x86-64.  Once the assert in my previous patch is added it's
easy to see a failure.  I configure gdb with "--enable-libmcheck
--enable-targets=all".

$ uname -a
Linux HOSTNAME 2.6.18-274.17.1.el5 #1 SMP Wed Jan 4 22:45:44 EST 2012 x86_64 x86_64 x86_64 GNU/Linux
$ cat hello.c
#include <stdio.h>

int
main ()
{  
  printf ("Hello World\n");

  return 0;
}
$ gcc --version
gcc (GCC) 4.7.2
$ gcc -g -o hello.x86 hello.c
$ gdb -i insight
# Then ...
#   File > Open > <Select: hello.x86>
#   Run Button (Stops at start of main)
#   Registers Button (Assert Fires)

With the assertion removed, the original SEGV I saw was triggered
by: once the register window had opened up I modified the contents of 
the rsp register to "0x0", hit return, then closed the register window.
Next I clicked the "Next" button (I wanted to see insight handle a SEGV
in the target application), I see a warning that the target has received a 
SIGSEGV, click OK, and then insight crashes, also with a SEGV.

Phew! Obviously there's going to be some randomness depending on memory
layout by the compiler, so it's you might not see the exact behaviour I
saw...

HTH
Andrew

Re: [PATCH 1/2] Add an assert that we're not overflowing the register cache.

[Andrew Burgess]

On 05/09/2013 7:55 PM, Keith Seitz wrote:
> On 09/05/2013 07:26 AM, Andrew Burgess wrote:
>> This patch doesn't fix the issue, but adds an assert to detect the bug.
> 
> If I understand correctly, the next patch fixes the actual problem, but
> this is added in case it happens again. Yes?

That's correct.

> 
>> OK to apply?
> 
> Yes. Thanks!

Applied.

Thanks,
Andrew

Re: [PATCH 2/2] Notice architecture changes even when the register window is not open.

[Andrew Burgess]

On 05/09/2013 8:00 PM, Keith Seitz wrote:
> On 09/05/2013 07:27 AM, Andrew Burgess wrote:
> 
> One little comment below.
> 
>> Index: ./gdb/gdbtk/library/interface.tcl
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/gdbtk/library/interface.tcl,v
>> retrieving revision 1.60
>> diff -u -p -r1.60 interface.tcl
>> --- ./gdb/gdbtk/library/interface.tcl    9 Oct 2009 01:23:55 -0000   
>> 1.60
>> +++ ./gdb/gdbtk/library/interface.tcl    5 Sep 2013 14:19:53 -0000
>> @@ -1815,6 +1815,9 @@ proc initialize_gdbtk {} {
>>   # The architecture changed. Inform the UI.
>>   proc gdbtk_tcl_architecture_changed {} {
>>     set e [ArchChangedEvent \#auto]
>> +  # First perform global actions as a result of the architecture change.
>> +  gdb_reg_arch_changed $e
>> +  # Now dispatch to all the other even handlers.
> 
> typo ("even[t] handlers")

Fixed.

> 
>>     GDBEventHandler::dispatch $e
>>     delete object $e
>>   }
> 
> Okay with that change.

Applied.

Thanks,
Andrew