[PATCH] gdb: fix post-hook execution for remote targets

[PATCH] gdb: fix post-hook execution for remote targets

[Jan Vrany]

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);
-- 
2.39.2

Re: [PATCH] gdb: fix post-hook execution for remote targets

[Jan Vraný]

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);

Re: [PATCH] gdb: fix post-hook execution for remote targets

[Jan Vraný]

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);
> 

Re: [PATCH] gdb: fix post-hook execution for remote targets

[Tom Tromey]

>>>>> "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

[PATCH v2] gdb: fix post-hook execution for remote targets

[Jan Vrany]

> 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?

Yes, that'd make things clear. I tried couple things but each time
something broke. The issue seem to be code like (top.c, execute_command()):id

      if (repeat_arguments != NULL && cmd_start == saved_command_line)
	{
	  gdb_assert (strlen (args_pointer) >= strlen (repeat_arguments));
	  strcpy (saved_command_line + (args_pointer - cmd_start),
		  repeat_arguments);
	}

Here we compare the value of saved_command_line so we cannot easily
set the flag and then when reading input and flag is set, return empty string
instead of saved_command_line. Truth to be told, I don't understand how it works
and what exactly (for example) the code above does or why it is needed. 

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

Done that, see below. 

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

I checked that with my version of GCC, std::string s = nullptr; throws an
error and abort()s but I could not find a situation when p == NULL.

The testsuite passes on my machine (even with ASAN). I also tried to open
two command line interpreters (using new-io console ...) and then close
the PTY but still, I did not hit the case of p being NULL.

Jan


-- >8 --
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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdb/top.c b/gdb/top.c
index 0b819091d11..92de30a1472 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -471,6 +471,8 @@ execute_command (const char *p, int from_tty)
       return;
     }
 
+  std::string cmd_copy = p;
+
   target_log_command (p);
 
   while (*p == ' ' || *p == '\t')
@@ -577,7 +579,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);
-- 
2.39.2

Re: [PATCH v2] gdb: fix post-hook execution for remote targets

[Tom Tromey]

>>>>> "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:

>> 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> Yes, that'd make things clear. I tried couple things but each time
Jan> something broke.

Yeah, the older the code is, the more likely it is to have some bizarre
hidden dependency.

Jan> Here we compare the value of saved_command_line so we cannot easily
Jan> set the flag and then when reading input and flag is set, return empty string
Jan> instead of saved_command_line. Truth to be told, I don't understand how it works
Jan> and what exactly (for example) the code above does or why it is needed. 

Me too.

Jan> I checked that with my version of GCC, std::string s = nullptr; throws an
Jan> error and abort()s but I could not find a situation when p == NULL.

It's possible that there isn't a case, but also possible that it's just
never tested.  Difficult to reason about these things.

Anyway, this is ok.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

[pushed] gdb: fix post-hook execution for remote targets

[Jan Vrany]

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

Approved-By: Tom Tromey <tom@tromey.com>
---
 gdb/top.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdb/top.c b/gdb/top.c
index 0b819091d11..92de30a1472 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -471,6 +471,8 @@ execute_command (const char *p, int from_tty)
       return;
     }
 
+  std::string cmd_copy = p;
+
   target_log_command (p);
 
   while (*p == ' ' || *p == '\t')
@@ -577,7 +579,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);
-- 
2.39.2