* [PATCH 0/2] Memory corruption caused by failure to notice architecture change. @ 2013-09-05 14:24 Andrew Burgess 2013-09-05 14:26 ` [PATCH 1/2] Add an assert that we're not overflowing the register cache Andrew Burgess 2013-09-05 14:27 ` [PATCH 2/2] Notice architecture changes even when the register window is not open Andrew Burgess 0 siblings, 2 replies; 8+ messages in thread From: Andrew Burgess @ 2013-09-05 14:24 UTC (permalink / raw) To: insight 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] Add an assert that we're not overflowing the register cache. 2013-09-05 14:24 [PATCH 0/2] Memory corruption caused by failure to notice architecture change Andrew Burgess @ 2013-09-05 14:26 ` Andrew Burgess 2013-09-05 18:55 ` Keith Seitz 2013-09-05 14:27 ` [PATCH 2/2] Notice architecture changes even when the register window is not open Andrew Burgess 1 sibling, 1 reply; 8+ messages in thread From: Andrew Burgess @ 2013-09-05 14:26 UTC (permalink / raw) To: insight 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 **)); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Add an assert that we're not overflowing the register cache. 2013-09-05 14:26 ` [PATCH 1/2] Add an assert that we're not overflowing the register cache Andrew Burgess @ 2013-09-05 18:55 ` Keith Seitz 2013-09-06 9:36 ` Andrew Burgess 0 siblings, 1 reply; 8+ messages in thread From: Keith Seitz @ 2013-09-05 18:55 UTC (permalink / raw) To: Andrew Burgess; +Cc: insight 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Add an assert that we're not overflowing the register cache. 2013-09-05 18:55 ` Keith Seitz @ 2013-09-06 9:36 ` Andrew Burgess 0 siblings, 0 replies; 8+ messages in thread From: Andrew Burgess @ 2013-09-06 9:36 UTC (permalink / raw) To: insight; +Cc: Keith Seitz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] Notice architecture changes even when the register window is not open. 2013-09-05 14:24 [PATCH 0/2] Memory corruption caused by failure to notice architecture change Andrew Burgess 2013-09-05 14:26 ` [PATCH 1/2] Add an assert that we're not overflowing the register cache Andrew Burgess @ 2013-09-05 14:27 ` Andrew Burgess 2013-09-05 19:53 ` Keith Seitz 1 sibling, 1 reply; 8+ messages in thread From: Andrew Burgess @ 2013-09-05 14:27 UTC (permalink / raw) To: insight 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Notice architecture changes even when the register window is not open. 2013-09-05 14:27 ` [PATCH 2/2] Notice architecture changes even when the register window is not open Andrew Burgess @ 2013-09-05 19:53 ` Keith Seitz 2013-09-06 9:21 ` Andrew Burgess 2013-09-06 9:40 ` Andrew Burgess 0 siblings, 2 replies; 8+ messages in thread From: Keith Seitz @ 2013-09-05 19:53 UTC (permalink / raw) To: Andrew Burgess; +Cc: insight 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Notice architecture changes even when the register window is not open. 2013-09-05 19:53 ` Keith Seitz @ 2013-09-06 9:21 ` Andrew Burgess 2013-09-06 9:40 ` Andrew Burgess 1 sibling, 0 replies; 8+ messages in thread From: Andrew Burgess @ 2013-09-06 9:21 UTC (permalink / raw) To: Keith Seitz; +Cc: insight 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Notice architecture changes even when the register window is not open. 2013-09-05 19:53 ` Keith Seitz 2013-09-06 9:21 ` Andrew Burgess @ 2013-09-06 9:40 ` Andrew Burgess 1 sibling, 0 replies; 8+ messages in thread From: Andrew Burgess @ 2013-09-06 9:40 UTC (permalink / raw) To: insight; +Cc: Keith Seitz 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-09-06 9:40 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-09-05 14:24 [PATCH 0/2] Memory corruption caused by failure to notice architecture change Andrew Burgess 2013-09-05 14:26 ` [PATCH 1/2] Add an assert that we're not overflowing the register cache Andrew Burgess 2013-09-05 18:55 ` Keith Seitz 2013-09-06 9:36 ` Andrew Burgess 2013-09-05 14:27 ` [PATCH 2/2] Notice architecture changes even when the register window is not open Andrew Burgess 2013-09-05 19:53 ` Keith Seitz 2013-09-06 9:21 ` Andrew Burgess 2013-09-06 9:40 ` Andrew Burgess
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).