[RFA] GDB/622 - clear current breakpoint in commands causes trouble

[RFA] GDB/622 - clear current breakpoint in commands causes trouble

[Joel Brobecker]

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

As described in PR GDB/622:

Using the program in the gdb.base/commands.exp testcase, the following
session shows that clearing the current breakpoint inside its commands
list causes a memory-corruption problem:

       (gdb) b factorial
       Breakpoint 1 at 0x8048582: file ./gdb.base/run.c, line 77.
       (gdb) commands
       Type commands for when breakpoint 1 is hit, one per line.
       End with a line saying just "end".
       >silent
       >printf "factorial command-list executed\n"
       >clear factorial
       >cont
       >end
       (gdb) run 1
       Starting program: [...]/gdb.base/commands 1
       factorial command-list executed
       warning: Invalid control type in command structure.      <<<--- (1)
       (gdb)

(1) shows that the command-list becomes corrupted, and as a consequence,
the execution is not resumed.  Instead, the expected output from the run
command is:

       Starting program: [...]/gdb.base/run 1
       factorial command-list executed
       1

       Program exited normally.
       (gdb)

The fix consists into executing a copy of the commands list, to protect
this execution from using a list that has been freed. The attached patch
introduces no regression. A new test has also been added to commands.exp.

Ok to apply?

2002-07-31  Joel Brobecker  <brobecker@gnat.com>

        * cli/cli-scripts.c (copy_command_lines): New function.
        (make_cleanup_free_command_lines): Make this function non static.

        * defs.h (copy_command_lines): Add definition.
        (make_cleanup_free_command_lines): Add definition.

        * breakpoint.c (bpstat_do_actions): Execute a temporary copy of
        the command-list associated to each breakpoint hit, in order to
        avoid accessing a dangling pointer, in case one of the commands
        in the list causes the breakpoint to be deleted.

Thanks,
-- 
Joel

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

Index: cli/cli-script.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
retrieving revision 1.13
diff -c -3 -p -r1.13 cli-script.c
*** cli/cli-script.c	30 Jul 2002 13:45:14 -0000	1.13
--- cli/cli-script.c	31 Jul 2002 19:49:40 -0000
*************** extern void do_restore_instream_cleanup 
*** 42,49 ****
  
  /* Prototypes for local functions */
  
! static struct cleanup *
! 	make_cleanup_free_command_lines (struct command_line **arg);
  
  static enum command_control_type
  	recurse_read_control_structure (struct command_line *current_cmd);
--- 42,48 ----
  
  /* Prototypes for local functions */
  
! struct cleanup *make_cleanup_free_command_lines (struct command_line **arg);
  
  static enum command_control_type
  	recurse_read_control_structure (struct command_line *current_cmd);
*************** do_free_command_lines_cleanup (void *arg
*** 1007,1016 ****
    free_command_lines (arg);
  }
  
! static struct cleanup *
  make_cleanup_free_command_lines (struct command_line **arg)
  {
    return make_cleanup (do_free_command_lines_cleanup, arg);
  }
  \f
  static void
--- 1006,1045 ----
    free_command_lines (arg);
  }
  
! struct cleanup *
  make_cleanup_free_command_lines (struct command_line **arg)
  {
    return make_cleanup (do_free_command_lines_cleanup, arg);
+ }
+ 
+ struct command_line *
+ copy_command_lines (struct command_line *cmds)
+ {
+   struct command_line *result = NULL;
+ 
+   if (cmds)
+     {
+       result = (struct command_line *) xmalloc (sizeof (struct command_line));
+ 
+       result->next = copy_command_lines (cmds->next);
+       result->line = strdup (cmds->line);
+       result->control_type = cmds->control_type;
+       result->body_count = cmds->body_count;
+       if (cmds->body_count > 0)
+         {
+           int i;
+ 
+           result->body_list = (struct command_line **)
+             xmalloc (sizeof (struct command_line *) * cmds->body_count);
+ 
+           for (i = 0; i < cmds->body_count; i++)
+             result->body_list[i] = copy_command_lines (cmds->body_list[i]);
+         }
+       else
+         result->body_list = NULL;
+     }
+ 
+   return result;
  }
  \f
  static void
Index: defs.h
===================================================================
RCS file: /cvs/src/src/gdb/defs.h,v
retrieving revision 1.93
diff -c -3 -p -r1.93 defs.h
*** defs.h	24 Jul 2002 22:46:48 -0000	1.93
--- defs.h	31 Jul 2002 19:49:40 -0000
*************** struct command_line
*** 653,658 ****
--- 653,661 ----
  extern struct command_line *read_command_lines (char *, int);
  
  extern void free_command_lines (struct command_line **);
+ extern struct cleanup *
+   make_cleanup_free_command_lines (struct command_line **arg);
+ extern struct command_line *copy_command_lines (struct command_line *);
  
  /* To continue the execution commands when running gdb asynchronously. 
     A continuation structure contains a pointer to a function to be called 
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.78
diff -c -3 -p -r1.78 breakpoint.c
*** breakpoint.c	26 Jun 2002 05:20:04 -0000	1.78
--- breakpoint.c	31 Jul 2002 19:49:45 -0000
*************** top:
*** 1882,1888 ****
    breakpoint_proceeded = 0;
    for (; bs != NULL; bs = bs->next)
      {
!       cmd = bs->commands;
        while (cmd != NULL)
  	{
  	  execute_control_command (cmd);
--- 1882,1892 ----
    breakpoint_proceeded = 0;
    for (; bs != NULL; bs = bs->next)
      {
!       /* Use a temporary copy of the commands, as one command in the list
!          may cause this breakpoint and its commands to be deleted.  */
!       cmd = copy_command_lines (bs->commands);
!       make_cleanup_free_command_lines (&cmd);
! 
        while (cmd != NULL)
  	{
  	  execute_control_command (cmd);
Index: testsuite/gdb.base/commands.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/commands.exp,v
retrieving revision 1.10
diff -c -3 -p -r1.10 commands.exp
*** testsuite/gdb.base/commands.exp	13 Dec 2001 22:42:23 -0000	1.10
--- testsuite/gdb.base/commands.exp	31 Jul 2002 19:49:45 -0000
*************** proc deprecated_command_test {} {
*** 440,445 ****
--- 440,503 ----
  	    "deprecate with no arguments"
  }
  
+ proc bp_deleted_in_command_test {} {
+     global gdb_prompt
+     
+     gdb_test "set args 1" "" "set args in bp_deleted_in_command_test"
+     delete_breakpoints
+ 
+     # Create a breakpoint, and associate a command-list to it, with
+     # one command that deletes this breakpoint.
+     gdb_test "break factorial" \
+              "Breakpoint \[0-9\]+ at .*: file .*/run.c, line \[0-9\]+\." \
+              "breakpoint in bp_deleted_in_command_test"
+     
+     send_gdb "commands\n"
+     gdb_expect {
+       -re "Type commands for when breakpoint .* is hit, one per line.*>" {
+           pass "begin commands in bp_deleted_in_command_test"
+       }
+       -re "$gdb_prompt $" {fail "begin commands in bp_deleted_in_command_test"}
+       timeout             {fail "(timeout) begin commands bp_deleted_in_command_test"}
+     }
+     send_gdb "silent\n"
+     gdb_expect {
+         -re ">"               {pass "add silent command"}
+         -re "$gdb_prompt $"   {fail "add silent command"}
+         timeout               {fail "(timeout) add silent command"}
+     }
+     send_gdb "printf \"factorial command-list executed\\n\"\n"
+     gdb_expect {
+         -re ">"               {pass "add printf command"}
+         -re "$gdb_prompt $"   {fail "add printf command"}
+         timeout               {fail "(timeout) add printf command"}
+     }
+     send_gdb "clear factorial\n"
+     gdb_expect {
+         -re ">"               {pass "add clear command"}
+         -re "$gdb_prompt $"   {fail "add clear command"}
+         timeout               {fail "(timeout) add clear command"} }
+     send_gdb "cont\n"
+     gdb_expect {
+         -re ">"               {pass "add cont command"}
+         -re "$gdb_prompt $"   {fail "add cont command"}
+         timeout               {fail "(timeout) add cont command"} }
+     send_gdb "end\n"
+     gdb_expect {
+         -re "$gdb_prompt $"   {pass "end commands"}
+         timeout               {fail "(timeout) end commands"}
+     }
+ 
+     gdb_run_cmd
+     gdb_expect {
+         -re ".*factorial command-list executed.*1.*Program exited normally.*" {
+             pass "run factorial until breakpoint"
+         }
+         timeout { fail "(timeout) run factorial until breakpoint" }
+     }
+ 
+ }
+ 
  
  gdbvar_simple_if_test
  gdbvar_simple_while_test
*************** user_defined_command_test
*** 454,456 ****
--- 512,515 ----
  watchpoint_command_test
  test_command_prompt_position
  deprecated_command_test
+ bp_deleted_in_command_test

Re: [RFA] GDB/622 - clear current breakpoint in commands causes trouble

[Joel Brobecker]

ping?

> 2002-07-31  Joel Brobecker  <brobecker@gnat.com>
> 
>         * cli/cli-scripts.c (copy_command_lines): New function.
>         (make_cleanup_free_command_lines): Make this function non static.
> 
>         * defs.h (copy_command_lines): Add definition.
>         (make_cleanup_free_command_lines): Add definition.
> 
>         * breakpoint.c (bpstat_do_actions): Execute a temporary copy of
>         the command-list associated to each breakpoint hit, in order to
>         avoid accessing a dangling pointer, in case one of the commands
>         in the list causes the breakpoint to be deleted.

-- 
Joel

Re: [RFA] GDB/622 - clear current breakpoint in commands causes trouble

[Michael Snyder]

Joel Brobecker wrote:
> 
> ping?
> 
> > 2002-07-31  Joel Brobecker  <brobecker@gnat.com>
> >
> >         * cli/cli-scripts.c (copy_command_lines): New function.
> >         (make_cleanup_free_command_lines): Make this function non static.
> >
> >         * defs.h (copy_command_lines): Add definition.
> >         (make_cleanup_free_command_lines): Add definition.
> >
> >         * breakpoint.c (bpstat_do_actions): Execute a temporary copy of
> >         the command-list associated to each breakpoint hit, in order to
> >         avoid accessing a dangling pointer, in case one of the commands
> >         in the list causes the breakpoint to be deleted.

Joel, 

It looks OK to me, but I'd like to run it by Don Howard, 
who has looked at this before.  Don, this patch looks a lot
simpler than the one you submitted (which, I think, died on
the vine (mea culpa)).  Do you think it will do the job?

Michael

Re: [RFA] GDB/622 - clear current breakpoint in commands causes trouble

[Don Howard]

On Thu, 22 Aug 2002, Michael Snyder wrote:

> Joel Brobecker wrote:
> > 
> > ping?
> > 
> > > 2002-07-31  Joel Brobecker  <brobecker@gnat.com>
> > >
> > >         * cli/cli-scripts.c (copy_command_lines): New function.
> > >         (make_cleanup_free_command_lines): Make this function non static.
> > >
> > >         * defs.h (copy_command_lines): Add definition.
> > >         (make_cleanup_free_command_lines): Add definition.
> > >
> > >         * breakpoint.c (bpstat_do_actions): Execute a temporary copy of
> > >         the command-list associated to each breakpoint hit, in order to
> > >         avoid accessing a dangling pointer, in case one of the commands
> > >         in the list causes the breakpoint to be deleted.
> 
> Joel, 
> 
> It looks OK to me, but I'd like to run it by Don Howard, 
> who has looked at this before.  Don, this patch looks a lot
> simpler than the one you submitted (which, I think, died on
> the vine (mea culpa)).  Do you think it will do the job?
> 
> Michael
> 

Yes this patch looks very much like one of my earlier attempts.  Joel's
copy_command_lines() is simpler and catches a few bugs that I noticed in
my implementation.  One question: is it important to use xstrdup() over
plain strdup()?

You already pointed out the cleanup issues.  Once that is corrected, I'd
recomend it.


-- 
dhoward@redhat.com
gdb engineering

Re: [RFA] GDB/622 - clear current breakpoint in commands causestrouble

[Michael Snyder]

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

Don Howard wrote:
> 
> On Thu, 22 Aug 2002, Michael Snyder wrote:
> 
> > Joel Brobecker wrote:
>
> > > > 2002-07-31  Joel Brobecker  <brobecker@gnat.com>
> > > >
> > > >         * cli/cli-scripts.c (copy_command_lines): New function.
> > > >         (make_cleanup_free_command_lines): Make this function non static.
> > > >
> > > >         * defs.h (copy_command_lines): Add definition.
> > > >         (make_cleanup_free_command_lines): Add definition.
> > > >
> > > >         * breakpoint.c (bpstat_do_actions): Execute a temporary copy of
> > > >         the command-list associated to each breakpoint hit, in order to
> > > >         avoid accessing a dangling pointer, in case one of the commands
> > > >         in the list causes the breakpoint to be deleted.
>
> > It looks OK to me, but I'd like to run it by Don Howard,
> > who has looked at this before.  Don, this patch looks a lot
> > simpler than the one you submitted (which, I think, died on
> > the vine (mea culpa)).  Do you think it will do the job?
>
> Yes this patch looks very much like one of my earlier attempts.  Joel's
> copy_command_lines() is simpler and catches a few bugs that I noticed in
> my implementation.  One question: is it important to use xstrdup() over
> plain strdup()?
> 
> You already pointed out the cleanup issues.  Once that is corrected, I'd
> recomend it.

Joel, Don, 

What do you think of the attached, to fix the cleanup issue?

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

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.82
diff -p -r1.82 breakpoint.c
*** breakpoint.c	23 Aug 2002 20:49:38 -0000	1.82
--- breakpoint.c	23 Aug 2002 23:47:18 -0000
*************** top:
*** 1927,1933 ****
    breakpoint_proceeded = 0;
    for (; bs != NULL; bs = bs->next)
      {
!       cmd = bs->commands;
        while (cmd != NULL)
  	{
  	  execute_control_command (cmd);
--- 1927,1939 ----
    breakpoint_proceeded = 0;
    for (; bs != NULL; bs = bs->next)
      {
!       struct cleanup *copy_cleanup;
! 
!       /* Use a temporary copy of the commands, as one command in the list
!          may cause this breakpoint and its commands to be deleted.  */
!       cmd = copy_command_lines (bs->commands);
!       copy_cleanup = make_cleanup_free_command_lines (&cmd);
! 
        while (cmd != NULL)
  	{
  	  execute_control_command (cmd);
*************** top:
*** 1945,1954 ****
  	goto top;
        else
  	bs->commands = NULL;
-     }
  
!   executing_breakpoint_commands = 0;
!   discard_cleanups (old_chain);
  }
  
  /* This is the normal print function for a bpstat.  In the future,
--- 1951,1960 ----
  	goto top;
        else
  	bs->commands = NULL;
  
!       do_cleanups (copy_cleanup);
!     }
!   do_cleanups (old_chain);
  }
  
  /* This is the normal print function for a bpstat.  In the future,

Re: [RFA] GDB/622 - clear current breakpoint in commands causes trouble

[Kevin Buettner]

On Aug 23, 10:48am, Don Howard wrote:

> One question: is it important to use xstrdup() over
> plain strdup()?

Don is right, xstrdup() should be used instead of strdup().  If there's
some good reason to prefer strdup(), then the return value needs to
be checked.  (Joel's 2002-07-31 patch isn't doing this.)

Kevin

Re: [RFA] GDB/622 - clear current breakpoint in commands causes trouble

[Andrew Cagney]

> On Aug 23, 10:48am, Don Howard wrote:
> 
> 
>> One question: is it important to use xstrdup() over
>> plain strdup()?
> 
> 
> Don is right, xstrdup() should be used instead of strdup().  If there's
> some good reason to prefer strdup(), then the return value needs to
> be checked.  (Joel's 2002-07-31 patch isn't doing this.)

http://sources.redhat.com/gdb/current/onlinedocs/gdbint_13.html#SEC115
(yep)

Re: [RFA] GDB/622 - clear current breakpoint in commands causes trouble

[Michael Snyder]

Joel Brobecker wrote:
> 
> As described in PR GDB/622:
> 
> Using the program in the gdb.base/commands.exp testcase, the following
> session shows that clearing the current breakpoint inside its commands
> list causes a memory-corruption problem:
> 
>        (gdb) b factorial
>        Breakpoint 1 at 0x8048582: file ./gdb.base/run.c, line 77.
>        (gdb) commands
>        Type commands for when breakpoint 1 is hit, one per line.
>        End with a line saying just "end".
>        >silent
>        >printf "factorial command-list executed\n"
>        >clear factorial
>        >cont
>        >end
>        (gdb) run 1
>        Starting program: [...]/gdb.base/commands 1
>        factorial command-list executed
>        warning: Invalid control type in command structure.      <<<--- (1)
>        (gdb)
> 
> (1) shows that the command-list becomes corrupted, and as a consequence,
> the execution is not resumed.  Instead, the expected output from the run
> command is:
> 
>        Starting program: [...]/gdb.base/run 1
>        factorial command-list executed
>        1
> 
>        Program exited normally.
>        (gdb)
> 
> The fix consists into executing a copy of the commands list, to protect
> this execution from using a list that has been freed. The attached patch
> introduces no regression. A new test has also been added to commands.exp.
> 
> Ok to apply?
> 
> 2002-07-31  Joel Brobecker  <brobecker@gnat.com>
> 
>         * cli/cli-scripts.c (copy_command_lines): New function.
>         (make_cleanup_free_command_lines): Make this function non static.
> 
>         * defs.h (copy_command_lines): Add definition.
>         (make_cleanup_free_command_lines): Add definition.
> 
>         * breakpoint.c (bpstat_do_actions): Execute a temporary copy of
>         the command-list associated to each breakpoint hit, in order to
>         avoid accessing a dangling pointer, in case one of the commands
>         in the list causes the breakpoint to be deleted.
> 
> Thanks,
> --
> Joel


Hey Joel, 

I've discovered a slight problem in this patch.
Your clean-ups are never being called.  Look at the bottom of
bpstat_do_actions.  They're just discarded.

The second problem is that, if they ever WERE called, 
they would probably cause a heap corruption error, because
the value of &cmd is always the same.

Michael