[RFC/RFA] GDB crash when using command lines due to memory corruption

[RFC/RFA] GDB crash when using command lines due to memory corruption

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

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  <brobecker@gnat.com>

        * 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 ();
}


[-- Attachment #2: commands2.diff --]
[-- Type: text/plain, Size: 3678 bytes --]

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.  */

Re: [RFC/RFA] GDB crash when using command lines due to memory corruption

[Klee Dienes]

A safer change for 5.3 might be the patch I submitted on October 30th.

	http://sources.redhat.com/ml/gdb-patches/2002-10/msg00586.html

Rather than deal with sharing the command-line structure, I just 
updated bpstat_copy to match its comment, and do a deep copy of the 
command lines as well as the value.  I don't really have a strong 
opinion about copying the command lines vs. managing them the way Joel 
proposes, although my patch does have the argument of simplicity going 
for it.  On the other hand, if/when we go to a more sophistiated 
command-line evaluator, we'll probably want the command body to be some 
opaque and externally managed structure anyway.

Whichever patch we end up taking, though, we should be sure to update 
the comment in bpstat_copy and add my proposed change to the test suite.

Re: [RFC/RFA] GDB crash when using command lines due to memory corruption

[Michael Snyder]

Klee Dienes wrote:
> 
> A safer change for 5.3 might be the patch I submitted on October 30th.
> 
>         http://sources.redhat.com/ml/gdb-patches/2002-10/msg00586.html
> 
> Rather than deal with sharing the command-line structure, I just
> updated bpstat_copy to match its comment, and do a deep copy of the
> command lines as well as the value.  I don't really have a strong
> opinion about copying the command lines vs. managing them the way Joel
> proposes, although my patch does have the argument of simplicity going
> for it.  On the other hand, if/when we go to a more sophistiated
> command-line evaluator, we'll probably want the command body to be some
> opaque and externally managed structure anyway.
> 
> Whichever patch we end up taking, though, we should be sure to update
> the comment in bpstat_copy and add my proposed change to the test suite.

Joel, would Klee's change satisfy your requirement?

Re: [RFC/RFA] GDB crash when using command lines due to memory corruption

[Joel Brobecker]

> > A safer change for 5.3 might be the patch I submitted on October 30th.
> > 
> >         http://sources.redhat.com/ml/gdb-patches/2002-10/msg00586.html
> > 
> 
> Joel, would Klee's change satisfy your requirement?

Absolutely. Klee's change is actually the first solution I came up with,
but for some reason I remembered that there was a reluctance to copy
this command lines structure more often than necessary. To be fair, I
really like Klee's change over mine, as it is much simpler and much more
maintainable. Can Klee's change be approved, and can Klee do the
checkin? I can also do the checkin on Klee's behalf if he's busy.
He's addition testcase is also a useful addition to our testsuite.

One small detail, is about memory management. Did anybody make sure that
there is no memory leak introduced in this change?

Speaking of memory leaks, there is the small change is
bpstat_stop_status which I will resubmit separately.

Thanks,
-- 
Joel

Re: [RFC/RFA] GDB crash when using command lines due to memory corruption

[Joel Brobecker]

> A safer change for 5.3 might be the patch I submitted on October 30th.
> 
> 	http://sources.redhat.com/ml/gdb-patches/2002-10/msg00586.html

For the record, this patch fixes a few regressions (apart from the
original problem).  With GNAT as the compiler (still 2.8.1 based):

Before:

        # of expected passes            8305
        # of unexpected failures        47
        # of unexpected successes       10
        # of expected failures          167
        # of untested testcases         3
        # of unsupported tests          2

After:

        # of expected passes            8309
        # of unexpected failures        38
        # of unexpected successes       10
        # of expected failures          172
        # of untested testcases         3
        # of unsupported tests          2

(thanks Klee for pointing out at your patch)
-- 
Joel

Re: [RFC/RFA] GDB crash when using command lines due to memory corruption

[Daniel Jacobowitz]

On Thu, Dec 12, 2002 at 11:04:06AM +0100, Joel Brobecker wrote:
> > A safer change for 5.3 might be the patch I submitted on October 30th.
> > 
> > 	http://sources.redhat.com/ml/gdb-patches/2002-10/msg00586.html
> 
> For the record, this patch fixes a few regressions (apart from the
> original problem).  With GNAT as the compiler (still 2.8.1 based):
> 
> Before:
> 
>         # of expected passes            8305
>         # of unexpected failures        47
>         # of unexpected successes       10
>         # of expected failures          167
>         # of untested testcases         3
>         # of unsupported tests          2
> 
> After:
> 
>         # of expected passes            8309
>         # of unexpected failures        38
>         # of unexpected successes       10
>         # of expected failures          172
>         # of untested testcases         3
>         # of unsupported tests          2
> 
> (thanks Klee for pointing out at your patch)

I can't think what in our testsuite could be affected by this.  Are you
sure they're real changes?  Which tests?  Also note that both passes
and expected failures went up, which is fishy.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] (ping) GDB crash when using command lines due to memory corruption

[Joel Brobecker]

There is a patch that was submitted on Oct 30th 2002 but hasn't been
reviewed because it addresses a GDB crash. It also expands break.exp
to test for this.

Can somebody have a look please?

Klee, I'll be happy to check it in for you if you lack the time.

On Wed, Dec 11, 2002 at 01:00:58PM -0500, Klee Dienes wrote:
> A safer change for 5.3 might be the patch I submitted on October 30th.
> 
> 	http://sources.redhat.com/ml/gdb-patches/2002-10/msg00586.html
> 
> Rather than deal with sharing the command-line structure, I just 
> updated bpstat_copy to match its comment, and do a deep copy of the 
> command lines as well as the value.  I don't really have a strong 
> opinion about copying the command lines vs. managing them the way Joel 
> proposes, although my patch does have the argument of simplicity going 
> for it.  On the other hand, if/when we go to a more sophistiated 
> command-line evaluator, we'll probably want the command body to be some 
> opaque and externally managed structure anyway.
> 
> Whichever patch we end up taking, though, we should be sure to update 
> the comment in bpstat_copy and add my proposed change to the test suite.
> 

-- 
Joel

Re: [RFA] (ping) GDB crash when using command lines due to memory corruption

[Joel Brobecker]

> There is a patch that was submitted on Oct 30th 2002 but hasn't been
> reviewed despite the fact that it addresses a GDB crash. It also
> expands break.exp to test for this.

Second ping...

> On Wed, Dec 11, 2002 at 01:00:58PM -0500, Klee Dienes wrote:
> > A safer change for 5.3 might be the patch I submitted on October 30th.
> > 
> > 	http://sources.redhat.com/ml/gdb-patches/2002-10/msg00586.html
> > 
> > Rather than deal with sharing the command-line structure, I just 
> > updated bpstat_copy to match its comment, and do a deep copy of the 
> > command lines as well as the value.  I don't really have a strong 
> > opinion about copying the command lines vs. managing them the way Joel 
> > proposes, although my patch does have the argument of simplicity going 
> > for it.  On the other hand, if/when we go to a more sophistiated 
> > command-line evaluator, we'll probably want the command body to be some 
> > opaque and externally managed structure anyway.
> > 
> > Whichever patch we end up taking, though, we should be sure to update 
> > the comment in bpstat_copy and add my proposed change to the test suite.

Thanks,
-- 
Joel

Re: [RFA] (ping) GDB crash when using command lines due to memory corruption

[Andrew Cagney]

Jim? Michael?

Per the bug database, three variants on this patch have been submitted 
yet nothing got resolved.

Andrew

>> There is a patch that was submitted on Oct 30th 2002 but hasn't been
>> reviewed despite the fact that it addresses a GDB crash. It also
>> expands break.exp to test for this.
> 
> 
> Second ping...
> 
> 
>> On Wed, Dec 11, 2002 at 01:00:58PM -0500, Klee Dienes wrote:
> 
>> > A safer change for 5.3 might be the patch I submitted on October 30th.
>> > 
>> > 	http://sources.redhat.com/ml/gdb-patches/2002-10/msg00586.html
>> > 
>> > Rather than deal with sharing the command-line structure, I just 
>> > updated bpstat_copy to match its comment, and do a deep copy of the 
>> > command lines as well as the value.  I don't really have a strong 
>> > opinion about copying the command lines vs. managing them the way Joel 
>> > proposes, although my patch does have the argument of simplicity going 
>> > for it.  On the other hand, if/when we go to a more sophistiated 
>> > command-line evaluator, we'll probably want the command body to be some 
>> > opaque and externally managed structure anyway.
>> > 
>> > Whichever patch we end up taking, though, we should be sure to update 
>> > the comment in bpstat_copy and add my proposed change to the test suite.
> 
> 
> Thanks,
> -- Joel 

Re: [RFA] (ping) GDB crash when using command lines due to memory corruption

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 30 bytes --]

Again.

Jim, Michael?

Andrew

[-- Attachment #2: mailbox-message://ac131313@movemail/fsf/gdb/patches#30103427 --]
[-- Type: message/rfc822, Size: 4283 bytes --]

From: Andrew Cagney <ac131313@redhat.com>
To: Michael Snyder <msnyder@redhat.com>, Jim Blandy <jimb@redhat.com>
Cc: Joel Brobecker <brobecker@gnat.com>, gdb-patches@sources.redhat.com
Subject: Re: [RFA] (ping) GDB crash when using command lines due to memory corruption
Date: Tue, 11 Feb 2003 17:55:49 +0100
Message-ID: <3E492B15.50601@redhat.com>

Jim? Michael?

Per the bug database, three variants on this patch have been submitted 
yet nothing got resolved.

Andrew

>> There is a patch that was submitted on Oct 30th 2002 but hasn't been
>> reviewed despite the fact that it addresses a GDB crash. It also
>> expands break.exp to test for this.
> 
> 
> Second ping...
> 
> 
>> On Wed, Dec 11, 2002 at 01:00:58PM -0500, Klee Dienes wrote:
> 
>> > A safer change for 5.3 might be the patch I submitted on October 30th.
>> > 
>> > 	http://sources.redhat.com/ml/gdb-patches/2002-10/msg00586.html
>> > 
>> > Rather than deal with sharing the command-line structure, I just 
>> > updated bpstat_copy to match its comment, and do a deep copy of the 
>> > command lines as well as the value.  I don't really have a strong 
>> > opinion about copying the command lines vs. managing them the way Joel 
>> > proposes, although my patch does have the argument of simplicity going 
>> > for it.  On the other hand, if/when we go to a more sophistiated 
>> > command-line evaluator, we'll probably want the command body to be some 
>> > opaque and externally managed structure anyway.
>> > 
>> > Whichever patch we end up taking, though, we should be sure to update 
>> > the comment in bpstat_copy and add my proposed change to the test suite.
> 
> 
> Thanks,
> -- Joel 

Re: [RFC/RFA] GDB crash when using command lines due to memory corruption

[Daniel Jacobowitz]

On Wed, Dec 11, 2002 at 01:00:58PM -0500, Klee Dienes wrote:
> A safer change for 5.3 might be the patch I submitted on October 30th.
> 
> 	http://sources.redhat.com/ml/gdb-patches/2002-10/msg00586.html
> 
> Rather than deal with sharing the command-line structure, I just 
> updated bpstat_copy to match its comment, and do a deep copy of the 
> command lines as well as the value.  I don't really have a strong 
> opinion about copying the command lines vs. managing them the way Joel 
> proposes, although my patch does have the argument of simplicity going 
> for it.  On the other hand, if/when we go to a more sophistiated 
> command-line evaluator, we'll probably want the command body to be some 
> opaque and externally managed structure anyway.
> 
> Whichever patch we end up taking, though, we should be sure to update 
> the comment in bpstat_copy and add my proposed change to the test suite.

Klee, I'm going to approve the breakpoint parts of this patch now. 
Since this bug really annoyed me, I'm checking them in, too.  Would you
mind reposting the testsuite patch for approval?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer