diff mbox series

gdb: fix post-hook execution for remote targets

Message ID 20230414151709.30352-1-jan.vrany@labware.com
State New
Headers
Series gdb: fix post-hook execution for remote targets |

Commit Message

Jan Vrany April 14, 2023, 3:17 p.m. UTC 
  Commit b5661ff2 ("gdb: fix possible use-after-free when
executing commands") attempted to fix possible use-after-free
in case command redefines itself.

Commit 37e5833d ("gdb: fix command lookup in execute_command ()")
updated the previous fix to handle subcommands as well by using the
original command string to lookup the command again after its execution.

This fixed the test in gdb.base/define.exp but it turned out that it
does not work (at least) for "target remote" and "target extended-remote".

The problem is that the command buffer P passed to execute_command ()
gets overwritten in dont_repeat () while executing "target remote"
command itself:

	#0  dont_repeat () at top.c:822
	#1  0x000055555730982a in target_preopen (from_tty=1) at target.c:2483
	#2  0x000055555711e911 in remote_target::open_1 (name=0x55555881c7fe ":1234", from_tty=1, extended_p=0)
	    at remote.c:5946
	#3  0x000055555711d577 in remote_target::open (name=0x55555881c7fe ":1234", from_tty=1) at remote.c:5272
	#4  0x00005555573062f2 in open_target (args=0x55555881c7fe ":1234", from_tty=1, command=0x5555589d0490)
	    at target.c:853
	#5  0x0000555556ad22fa in cmd_func (cmd=0x5555589d0490, args=0x55555881c7fe ":1234", from_tty=1)
	    at cli/cli-decode.c:2737
	#6  0x00005555573487fd in execute_command (p=0x55555881c802 "4", from_tty=1) at top.c:688

Therefore the second call to lookup_cmd () at line 697 fails to find
command because the original command string is gone.

This commit addresses this particular problem by creating a *copy* of
original command string for the sole purpose of using it after command
execution to lookup the command again. It may not be the most efficient
way but it's safer given that command buffer is shared and overwritten
in hard-to-foresee situations.

Tested on x86_64-linux.

PR 30249
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30249
---
 gdb/top.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jan Vrany May 3, 2023, 2:40 p.m. UTC | #1 
Polite ping. 

Jan

On Fri, 2023-04-14 at 17:17 +0200, Jan Vrany wrote:
> Commit b5661ff2 ("gdb: fix possible use-after-free when
> executing commands") attempted to fix possible use-after-free
> in case command redefines itself.
> 
> Commit 37e5833d ("gdb: fix command lookup in execute_command ()")
> updated the previous fix to handle subcommands as well by using the
> original command string to lookup the command again after its execution.
> 
> This fixed the test in gdb.base/define.exp but it turned out that it
> does not work (at least) for "target remote" and "target extended-remote".
> 
> The problem is that the command buffer P passed to execute_command ()
> gets overwritten in dont_repeat () while executing "target remote"
> command itself:
> 
> 	#0  dont_repeat () at top.c:822
> 	#1  0x000055555730982a in target_preopen (from_tty=1) at target.c:2483
> 	#2  0x000055555711e911 in remote_target::open_1 (name=0x55555881c7fe ":1234", from_tty=1, extended_p=0)
> 	    at remote.c:5946
> 	#3  0x000055555711d577 in remote_target::open (name=0x55555881c7fe ":1234", from_tty=1) at remote.c:5272
> 	#4  0x00005555573062f2 in open_target (args=0x55555881c7fe ":1234", from_tty=1, command=0x5555589d0490)
> 	    at target.c:853
> 	#5  0x0000555556ad22fa in cmd_func (cmd=0x5555589d0490, args=0x55555881c7fe ":1234", from_tty=1)
> 	    at cli/cli-decode.c:2737
> 	#6  0x00005555573487fd in execute_command (p=0x55555881c802 "4", from_tty=1) at top.c:688
> 
> Therefore the second call to lookup_cmd () at line 697 fails to find
> command because the original command string is gone.
> 
> This commit addresses this particular problem by creating a *copy* of
> original command string for the sole purpose of using it after command
> execution to lookup the command again. It may not be the most efficient
> way but it's safer given that command buffer is shared and overwritten
> in hard-to-foresee situations.
> 
> Tested on x86_64-linux.
> 
> PR 30249
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30249
> ---
>  gdb/top.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/top.c b/gdb/top.c
> index 81f74f72f61..63798789553 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -575,6 +575,7 @@ execute_command (const char *p, int from_tty)
>    struct cmd_list_element *c;
>    const char *line;
>    const char *cmd_start = p;
> +  std::string cmd_copy = p;
>  
>    auto cleanup_if_error = make_scope_exit (bpstat_clear_actions);
>    scoped_value_mark cleanup = prepare_execute_command ();
> @@ -692,7 +693,7 @@ execute_command (const char *p, int from_tty)
>  	 We need to lookup the command again since during its execution,
>  	 a command may redefine itself.  In this case, C pointer
>  	 becomes invalid so we need to look it up again.  */
> -      const char *cmd2 = cmd_start;
> +      const char *cmd2 = cmd_copy.c_str ();
>        c = lookup_cmd (&cmd2, cmdlist, "", nullptr, 1, 1);
>        if (c != nullptr)
>  	execute_cmd_post_hook (c);
Jan Vrany May 10, 2023, 11:14 a.m. UTC | #2 
Polite ping 2.

Jan


On Wed, 2023-05-03 at 15:40 +0100, Jan Vrany wrote:
> Polite ping. 
> 
> Jan
> 
> On Fri, 2023-04-14 at 17:17 +0200, Jan Vrany wrote:
> > Commit b5661ff2 ("gdb: fix possible use-after-free when
> > executing commands") attempted to fix possible use-after-free
> > in case command redefines itself.
> > 
> > Commit 37e5833d ("gdb: fix command lookup in execute_command ()")
> > updated the previous fix to handle subcommands as well by using the
> > original command string to lookup the command again after its execution.
> > 
> > This fixed the test in gdb.base/define.exp but it turned out that it
> > does not work (at least) for "target remote" and "target extended-remote".
> > 
> > The problem is that the command buffer P passed to execute_command ()
> > gets overwritten in dont_repeat () while executing "target remote"
> > command itself:
> > 
> > 	#0  dont_repeat () at top.c:822
> > 	#1  0x000055555730982a in target_preopen (from_tty=1) at target.c:2483
> > 	#2  0x000055555711e911 in remote_target::open_1 (name=0x55555881c7fe ":1234", from_tty=1, extended_p=0)
> > 	    at remote.c:5946
> > 	#3  0x000055555711d577 in remote_target::open (name=0x55555881c7fe ":1234", from_tty=1) at remote.c:5272
> > 	#4  0x00005555573062f2 in open_target (args=0x55555881c7fe ":1234", from_tty=1, command=0x5555589d0490)
> > 	    at target.c:853
> > 	#5  0x0000555556ad22fa in cmd_func (cmd=0x5555589d0490, args=0x55555881c7fe ":1234", from_tty=1)
> > 	    at cli/cli-decode.c:2737
> > 	#6  0x00005555573487fd in execute_command (p=0x55555881c802 "4", from_tty=1) at top.c:688
> > 
> > Therefore the second call to lookup_cmd () at line 697 fails to find
> > command because the original command string is gone.
> > 
> > This commit addresses this particular problem by creating a *copy* of
> > original command string for the sole purpose of using it after command
> > execution to lookup the command again. It may not be the most efficient
> > way but it's safer given that command buffer is shared and overwritten
> > in hard-to-foresee situations.
> > 
> > Tested on x86_64-linux.
> > 
> > PR 30249
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30249
> > ---
> >  gdb/top.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/top.c b/gdb/top.c
> > index 81f74f72f61..63798789553 100644
> > --- a/gdb/top.c
> > +++ b/gdb/top.c
> > @@ -575,6 +575,7 @@ execute_command (const char *p, int from_tty)
> >    struct cmd_list_element *c;
> >    const char *line;
> >    const char *cmd_start = p;
> > +  std::string cmd_copy = p;
> >  
> >    auto cleanup_if_error = make_scope_exit (bpstat_clear_actions);
> >    scoped_value_mark cleanup = prepare_execute_command ();
> > @@ -692,7 +693,7 @@ execute_command (const char *p, int from_tty)
> >  	 We need to lookup the command again since during its execution,
> >  	 a command may redefine itself.  In this case, C pointer
> >  	 becomes invalid so we need to look it up again.  */
> > -      const char *cmd2 = cmd_start;
> > +      const char *cmd2 = cmd_copy.c_str ();
> >        c = lookup_cmd (&cmd2, cmdlist, "", nullptr, 1, 1);
> >        if (c != nullptr)
> >  	execute_cmd_post_hook (c);
>
Tom Tromey May 10, 2023, 3:07 p.m. UTC | #3 
>>>>> "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:

Sorry about the delay on this.

Jan> The problem is that the command buffer P passed to execute_command ()
Jan> gets overwritten in dont_repeat () while executing "target remote"
Jan> command itself:

Jan> 	#0  dont_repeat () at top.c:822

I wonder if it would be possible to reimplement dont_repeat so that it
does not overwrite the command.  Like, could it just set a flag?

Jan> @@ -575,6 +575,7 @@ execute_command (const char *p, int from_tty)
Jan>    struct cmd_list_element *c;
Jan>    const char *line;
Jan>    const char *cmd_start = p;
Jan> +  std::string cmd_copy = p;

A bit after this, there's a null check for p:

  /* This can happen when command_line_input hits end of file.  */
  if (p == NULL)
    {
      cleanup_if_error.release ();
      return;
    }

If that can really happen, then cmd_copy has to be created afterward the
check.

Is it possible to write a test case for this?  Even one that only fails
with ASAN?

Tom
diff mbox series

Patch

diff --git a/gdb/top.c b/gdb/top.c
index 81f74f72f61..63798789553 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -575,6 +575,7 @@  execute_command (const char *p, int from_tty)
   struct cmd_list_element *c;
   const char *line;
   const char *cmd_start = p;
+  std::string cmd_copy = p;
 
   auto cleanup_if_error = make_scope_exit (bpstat_clear_actions);
   scoped_value_mark cleanup = prepare_execute_command ();
@@ -692,7 +693,7 @@  execute_command (const char *p, int from_tty)
 	 We need to lookup the command again since during its execution,
 	 a command may redefine itself.  In this case, C pointer
 	 becomes invalid so we need to look it up again.  */
-      const char *cmd2 = cmd_start;
+      const char *cmd2 = cmd_copy.c_str ();
       c = lookup_cmd (&cmd2, cmdlist, "", nullptr, 1, 1);
       if (c != nullptr)
 	execute_cmd_post_hook (c);