From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12919 invoked by alias); 11 Dec 2002 17:38:08 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 12908 invoked from network); 11 Dec 2002 17:38:06 -0000 Received: from unknown (HELO takamaka.act-europe.fr) (212.157.227.202) by sources.redhat.com with SMTP; 11 Dec 2002 17:38:06 -0000 Received: by takamaka.act-europe.fr (Postfix, from userid 507) id C9960D2D29; Wed, 11 Dec 2002 18:38:05 +0100 (CET) Date: Wed, 11 Dec 2002 10:01:00 -0000 From: Joel Brobecker To: gdb-patches@sources.redhat.com Subject: [RFC/RFA] GDB crash when using command lines due to memory corruption Message-ID: <20021211173805.GG25575@gnat.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="CE+1k2dSO48ffgeK" Content-Disposition: inline User-Agent: Mutt/1.4i X-SW-Source: 2002-12/txt/msg00359.txt.bz2 --CE+1k2dSO48ffgeK Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 3394 This problem was reproduced using GDB freshly rebuilt from trunk on x86-linux, but is not platform dependent. Using the sources below, the following causes some problems... (gdb) b stopme Breakpoint 1 at 0x8048513: file foo.c, line 9. (gdb) commands Type commands for when breakpoint 1 is hit, one per line. End with a line saying just "end". >call callme () >end (gdb) run Starting program: foo Breakpoint 1, stopme () at foo.c:9 9 } warning: Invalid control type in command structure. warning: Invalid control type in command structure. warning: Invalid control type in command structure. zsh: 29624 segmentation fault ./gdb-public.ref/gdb/gdb foo I would guess that the same happens on 5.3 too, but I haven't tried yet. Anyone with a handy recent 5.3 build? What happens is that the "call" command causes the inferior to resume execution. According to the documentation, any command that causes the inferior to resume automatically ends the execution of the commands list. The following patch tries to fix this problem (and other minor nits), but does not try to enforce this rule. 2002-12-11 J. Brobecker * breakpoint.c (bpstat_clear): Do not deallocate the commands here, as the associated bpstat_copy does not perform a deep copy of this field. This is more symetrical, and it makes sure we don't have a double deallocation while execution a "call" command inside a commands list, as this command clears the global stop_bpstat after having "saved" it. (bpstat_do_actions): Deallocate the commands that we just executed. This is now needed, as this is no longer done by bpstat_clear. Make sure to not leave any dangling pointer behind. (bpstat_stop_status): Only copy the part of the commands list that will be executed. Otherwise, this would result in a small memory leak when using silent breakpoint commands. This change is probably independent from the other changes, but I did not have the courage to test it independently. This change does not introduce any regression. I am willing to contribute a new test for this case when a fix for this crash is checked in. Please accept my appologies for submitting the change in bpstat_stop_status together with the other changes. I seriously lack time right now :-(. This is really a small additional change, so hopefully it is ok if it tags along with the other more serious one. I verified as much as I could with all the weird scenarios I could think of using the gdb testsuite that there was no memory leak by using traces inside copy_commands_line and free_commands_lines. As for 5.3, it took me a while to figure out all (really all?) the possible scenarios that could happen, and the consequences on bpstat_do_actions. The fact that the bpstat given is the __global__ bpstat is really nasty, and makes this change risky, in my opinion. I would there simply add a note in the PROBLEMS file. Unless, of course, somebody with more GDB experience than me could confidently approve it... Ok to apply? -- Joel ---- foo.c ---- void callme (void) { } void stopme (void) { } int main (void) { int i; for (i=0; i < 3; i++) stopme (); } --CE+1k2dSO48ffgeK Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="commands2.diff" Content-length: 3678 Index: breakpoint.c =================================================================== RCS file: /nile.c/cvs/Dev/gdb/gdb-5/gdb/breakpoint.c,v retrieving revision 1.1.1.1.2.3.2.9 diff -c -3 -p -r1.1.1.1.2.3.2.9 breakpoint.c *** breakpoint.c 29 Aug 2002 16:41:28 -0000 1.1.1.1.2.3.2.9 --- breakpoint.c 11 Dec 2002 17:08:52 -0000 *************** bpstat_clear (bpstat *bsp) *** 1686,1692 **** q = p->next; if (p->old_val != NULL) value_free (p->old_val); - free_command_lines (&p->commands); xfree (p); p = q; } --- 1686,1691 ---- *************** bpstat_do_actions (bpstat *bsp) *** 1827,1832 **** --- 1826,1832 ---- bpstat bs; struct cleanup *old_chain; struct command_line *cmd; + struct command_line *saved_cmd; /* Avoid endless recursion if a `source' command is contained in bs->commands. */ *************** top: *** 1852,1857 **** --- 1852,1858 ---- for (; bs != NULL; bs = bs->next) { cmd = bs->commands; + saved_cmd = cmd; while (cmd != NULL) { execute_control_command (cmd); *************** top: *** 1861,1866 **** --- 1862,1877 ---- else cmd = cmd->next; } + + /* Deallocate the commands line that we just executed. We can not + deallocate directly the commands line in bs, as bs may have + changed while inside excecute_command_control if the command + caused the inferior to resume. Note that there is no memory + leak when an error is raised inside execute_control_command, + because the error catching mechanism automatically deallocates + it from the global stop_bpstat. */ + free_command_lines (&saved_cmd); + if (breakpoint_proceeded) /* The inferior is proceeded by the command; bomb out now. The bpstat chain has been blown away by wait_for_inferior. *************** top: *** 1868,1874 **** to look at, so start over. */ goto top; else ! free_command_lines (&bs->commands); } do_cleanups (old_chain); } --- 1879,1889 ---- to look at, so start over. */ goto top; else ! /* Since we deallocated the command_lines above, make sure ! to reset the commands in the bpstat, so make sure we do ! not leave any dangling pointer. */ ! bs->commands = NULL; ! } do_cleanups (old_chain); } *************** bpstat_stop_status (CORE_ADDR *pc, int n *** 2643,2651 **** /* We will stop here */ if (b->disposition == disp_disable) b->enable_state = bp_disabled; - bs->commands = copy_command_lines (b->commands); if (b->silent) bs->print = 0; if (bs->commands && (STREQ ("silent", bs->commands->line) || (xdb_commands && STREQ ("Q", bs->commands->line)))) --- 2658,2666 ---- /* We will stop here */ if (b->disposition == disp_disable) b->enable_state = bp_disabled; if (b->silent) bs->print = 0; + bs->commands = b->commands; if (bs->commands && (STREQ ("silent", bs->commands->line) || (xdb_commands && STREQ ("Q", bs->commands->line)))) *************** bpstat_stop_status (CORE_ADDR *pc, int n *** 2653,2658 **** --- 2668,2677 ---- bs->commands = bs->commands->next; bs->print = 0; } + /* Copy the part of the command lines that will be executed, + as the breakpoint may disappear before or during its + execution. */ + bs->commands = copy_command_lines (bs->commands); } } /* Print nothing for this entry if we dont stop or if we dont print. */ --CE+1k2dSO48ffgeK--