[PATCH] gdb: fix possible use-after-free when executing commands

[PATCH] gdb: fix possible use-after-free when executing commands

[Jan Vrany]

In principle, `execute_command()` does following:

   struct cmd_list_element *c;
   c = lookup_cmd ( ... );
   ...
   /* If this command has been pre-hooked, run the hook first.  */
   execute_cmd_pre_hook (c);
   ...
   /* ...execute the command `c` ...*/
   ...
   execute_cmd_post_hook (c);

This may lead into use-after-free error.  Imagine the command
being executed is a user-defined Python command that redefines
itself.  In that case, struct `cmd_list_element` pointed to by
`c` is deallocated during its execution so it is no longer valid
when post hook is executed.

To fix this case, this commit looks up the command once again
after it is executed to get pointer to (possibly newly allocated)
`cmd_list_element`.
---
 gdb/top.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/top.c b/gdb/top.c
index e9794184f07..441ca3e14c1 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -655,6 +655,8 @@ execute_command (const char *p, int from_tty)
 	    }
 	}
 
+      std::string c_name(c->name);
+
       /* If this command has been pre-hooked, run the hook first.  */
       execute_cmd_pre_hook (c);
 
@@ -694,7 +696,9 @@ execute_command (const char *p, int from_tty)
       maybe_wait_sync_command_done (was_sync);
 
       /* If this command has been post-hooked, run the hook last.  */
-      execute_cmd_post_hook (c);
+      c = lookup_cmd_exact (c_name.c_str (), cmdlist);
+      if (c != nullptr)
+	execute_cmd_post_hook (c);
 
       if (repeat_arguments != NULL && cmd_start == saved_command_line)
 	{
-- 
2.35.1

Re: [PATCH] gdb: fix possible use-after-free when executing commands

[Tom Tromey]

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

Jan> This may lead into use-after-free error.  Imagine the command
Jan> being executed is a user-defined Python command that redefines
Jan> itself.  In that case, struct `cmd_list_element` pointed to by
Jan> `c` is deallocated during its execution so it is no longer valid
Jan> when post hook is executed.

Thanks for the patch.

Your analysis makes sense to me.  I wouldn't be surprised if there were
other issues along these lines.  Or if this were in bugzilla somewhere.

Jan> +      std::string c_name(c->name);

Space before the paren.  Also I think a comment here explaining why it's
needed would be good.

Jan>        /* If this command has been post-hooked, run the hook last.  */
Jan> -      execute_cmd_post_hook (c);
Jan> +      c = lookup_cmd_exact (c_name.c_str (), cmdlist);
Jan> +      if (c != nullptr)
Jan> +	execute_cmd_post_hook (c);
 
Perhaps a comment here as well explaining the need to redo the lookup.

This is ok with these minor changes.

thanks,
Tom

Re: [PATCH] gdb: fix possible use-after-free when executing commands

[Luis Machado]

Hi,

On 12/9/22 17:55, Tom Tromey wrote:
>>>>>> "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Jan> This may lead into use-after-free error.  Imagine the command
> Jan> being executed is a user-defined Python command that redefines
> Jan> itself.  In that case, struct `cmd_list_element` pointed to by
> Jan> `c` is deallocated during its execution so it is no longer valid
> Jan> when post hook is executed.
> 
> Thanks for the patch.
> 
> Your analysis makes sense to me.  I wouldn't be surprised if there were
> other issues along these lines.  Or if this were in bugzilla somewhere.
> 
> Jan> +      std::string c_name(c->name);
> 
> Space before the paren.  Also I think a comment here explaining why it's
> needed would be good.
> 
> Jan>        /* If this command has been post-hooked, run the hook last.  */
> Jan> -      execute_cmd_post_hook (c);
> Jan> +      c = lookup_cmd_exact (c_name.c_str (), cmdlist);
> Jan> +      if (c != nullptr)
> Jan> +	execute_cmd_post_hook (c);
>   
> Perhaps a comment here as well explaining the need to redo the lookup.
> 
> This is ok with these minor changes.
> 
> thanks,
> Tom

I've spotted gdb.base/define.exp failing today, and bisection stopped in this particular
patch.

target testsuite
one
hello
(gdb) FAIL: gdb.base/define.exp: target testsuite with hooks

Re: [PATCH] gdb: fix possible use-after-free when executing commands

[Jan Vraný]

On Mon, 2022-12-12 at 15:05 +0000, Luis Machado wrote:
> Hi,
> 
> On 12/9/22 17:55, Tom Tromey wrote:
> > > > > > > "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> > 
> > Jan> This may lead into use-after-free error.  Imagine the command
> > Jan> being executed is a user-defined Python command that redefines
> > Jan> itself.  In that case, struct `cmd_list_element` pointed to by
> > Jan> `c` is deallocated during its execution so it is no longer valid
> > Jan> when post hook is executed.
> > 
> > Thanks for the patch.
> > 
> > Your analysis makes sense to me.  I wouldn't be surprised if there were
> > other issues along these lines.  Or if this were in bugzilla somewhere.
> > 
> > Jan> +      std::string c_name(c->name);
> > 
> > Space before the paren.  Also I think a comment here explaining why it's
> > needed would be good.
> > 
> > Jan>        /* If this command has been post-hooked, run the hook last.  */
> > Jan> -      execute_cmd_post_hook (c);
> > Jan> +      c = lookup_cmd_exact (c_name.c_str (), cmdlist);
> > Jan> +      if (c != nullptr)
> > Jan> +	execute_cmd_post_hook (c);
> >   
> > Perhaps a comment here as well explaining the need to redo the lookup.
> > 
> > This is ok with these minor changes.
> > 
> > thanks,
> > Tom
> 
> I've spotted gdb.base/define.exp failing today, and bisection stopped in this particular
> patch.
> 
> target testsuite
> one
> hello
> (gdb) FAIL: gdb.base/define.exp: target testsuite with hooks

Ouch, I'll have a look ASAP. 

Jan

> 

Re: [PATCH] gdb: fix possible use-after-free when executing commands

[Luis Machado]

On 12/12/22 15:05, Luis Machado wrote:
> Hi,
> 
> On 12/9/22 17:55, Tom Tromey wrote:
>>>>>>> "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
>>
>> Jan> This may lead into use-after-free error.  Imagine the command
>> Jan> being executed is a user-defined Python command that redefines
>> Jan> itself.  In that case, struct `cmd_list_element` pointed to by
>> Jan> `c` is deallocated during its execution so it is no longer valid
>> Jan> when post hook is executed.
>>
>> Thanks for the patch.
>>
>> Your analysis makes sense to me.  I wouldn't be surprised if there were
>> other issues along these lines.  Or if this were in bugzilla somewhere.
>>
>> Jan> +      std::string c_name(c->name);
>>
>> Space before the paren.  Also I think a comment here explaining why it's
>> needed would be good.
>>
>> Jan>        /* If this command has been post-hooked, run the hook last.  */
>> Jan> -      execute_cmd_post_hook (c);
>> Jan> +      c = lookup_cmd_exact (c_name.c_str (), cmdlist);
>> Jan> +      if (c != nullptr)
>> Jan> +    execute_cmd_post_hook (c);
>> Perhaps a comment here as well explaining the need to redo the lookup.
>>
>> This is ok with these minor changes.
>>
>> thanks,
>> Tom
> 
> I've spotted gdb.base/define.exp failing today, and bisection stopped in this particular
> patch.
> 
> target testsuite
> one
> hello
> (gdb) FAIL: gdb.base/define.exp: target testsuite with hooks

A correction execution shows the following:

target testsuite
one
hello
two

This is on aarch64-linux Ubuntu 22.04/20.04.

[PATCH] gdb: fix command lookup in execute_command ()

[Jan Vrany]

Hi,

> I've spotted gdb.base/define.exp failing today, and bisection stopped in this particular
> patch.

> target testsuite
> one
> hello
> (gdb) FAIL: gdb.base/define.exp: target testsuite with hooks

My fault, I did not consider the case of hooks on subcommands
(I was not even conciously aware this is possible).

The below patch should fix this case - with it on my system
gdb.base/define.exp passes.

Alternatively I may just revert the commit until better solution
is found.

-- >8 --
Subject: [PATCH] gdb: fix command lookup in execute_command ()

Commit b5661ff2 ("gdb: fix possible use-after-free when
executing commands") used lookup_cmd_exact () to lookup
command again after its execution to avoid possible
use-after-free error.

However this change broke test gdb.base/define.exp which
defines a post-hook for subcommand ("target testsuite").
In this case,  lookup_cmd_exact () returned NULL because
there's no command 'testsuite' in top-level commands.

This commit fixes this case by looking up the command again
using the original command line via lookup_cmd ().
---
 gdb/top.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index 742997808bd..caa08d98cec 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -655,11 +655,6 @@ execute_command (const char *p, int from_tty)
 	    }
 	}
 
-      /* Remember name of the command.  This is needed later when
-	 executing command post-hooks to handle the case when command
-	 is redefined or removed during it's execution.  See below.  */
-      std::string c_name (c->name);
-
       /* If this command has been pre-hooked, run the hook first.  */
       execute_cmd_pre_hook (c);
 
@@ -702,7 +697,8 @@ 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.  */
-      c = lookup_cmd_exact (c_name.c_str (), cmdlist);
+      const char *cmd2 = cmd_start;
+      c = lookup_cmd (&cmd2, cmdlist, "", nullptr, 1, 1);
       if (c != nullptr)
 	execute_cmd_post_hook (c);
 
-- 
2.35.1

Re: [PATCH] gdb: fix command lookup in execute_command ()

[Tom Tromey]

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

Jan> The below patch should fix this case - with it on my system
Jan> gdb.base/define.exp passes.

Jan> Alternatively I may just revert the commit until better solution
Jan> is found.

This patch looks ok to me, but could you say whether you ran all the
tests or just define.exp?  Given the history I think a full regression
test is warranted.

thanks,
Tom

Re: [PATCH] gdb: fix command lookup in execute_command ()

[Simon Marchi]

On 12/13/22 10:05, Tom Tromey wrote:
>>>>>> "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Jan> The below patch should fix this case - with it on my system
> Jan> gdb.base/define.exp passes.
> 
> Jan> Alternatively I may just revert the commit until better solution
> Jan> is found.
> 
> This patch looks ok to me, but could you say whether you ran all the
> tests or just define.exp?  Given the history I think a full regression
> test is warranted.
> 
> thanks,
> Tom

I'll run the patch through my CI job, where I am seeing the define.exp failures
currently.

Simon

Re: [PATCH] gdb: fix command lookup in execute_command ()

[Jan Vraný]

On Tue, 2022-12-13 at 08:05 -0700, Tom Tromey wrote:
> > > > > > "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Jan> The below patch should fix this case - with it on my system
> Jan> gdb.base/define.exp passes.
> 
> Jan> Alternatively I may just revert the commit until better solution
> Jan> is found.
> 
> This patch looks ok to me, but could you say whether you ran all the
> tests or just define.exp?  Given the history I think a full regression
> test is warranted.

I did use try-build which succeeded (build #28) but it seems to me it
runs only very limited number of tests. 

I did run all of gdb.base and did not spot any new regression compared
to master with my (previous, broken) patch reverted. I did not run more
than gdb.base mainly because just gdb.base takes about an hour on my machine.
Also I get weird intermittent failures when running testsuite. 

Jan

> 
> thanks,
> Tom
> 

Re: [PATCH] gdb: fix command lookup in execute_command ()

[Simon Marchi]

On 12/13/22 13:48, Jan Vraný wrote:
> On Tue, 2022-12-13 at 08:05 -0700, Tom Tromey wrote:
>>>>>>> "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
>>
>> Jan> The below patch should fix this case - with it on my system
>> Jan> gdb.base/define.exp passes.
>>
>> Jan> Alternatively I may just revert the commit until better solution
>> Jan> is found.
>>
>> This patch looks ok to me, but could you say whether you ran all the
>> tests or just define.exp?  Given the history I think a full regression
>> test is warranted.
> 
> I did use try-build which succeeded (build #28) but it seems to me it
> runs only very limited number of tests. 
> 
> I did run all of gdb.base and did not spot any new regression compared
> to master with my (previous, broken) patch reverted. I did not run more
> than gdb.base mainly because just gdb.base takes about an hour on my machine.
> Also I get weird intermittent failures when running testsuite. 
> 
> Jan

Here are the unexpected failures I saw:

UNRESOLVED: gdb.base/bp-cmds-execution-x-script.exp: run to end
UNRESOLVED: gdb.base/bp-cmds-run-with-ex.exp: execute bp commands
UNRESOLVED: gdb.base/bp-cmds-sourced-script.exp: source the script
UNRESOLVED: gdb.base/bp-cmds-sourced-script.exp: continue until exit
UNRESOLVED: gdb.base/commands.exp: deprecated_command_test: source file containing xxx_yyy command and its alias
UNRESOLVED: gdb.base/commands.exp: deprecated_command_test: deprecated alias with prefix give a warning
UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: define real_command: input 1: define real_command
UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: alias alias_command = real_command
UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: alias alias_with_args_command = real_command 123
UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: help real_command, before
UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: maintenance deprecate alias_command
UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: maintenance deprecate alias_with_args_command
UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: help real_command, after
UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: delete all breakpoints in delete_breakpoints
UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: break factorial
UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: begin commands
UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: add silent command
UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: add clear command
UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: add printf command
UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: add cont command
UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: end commands
UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: delete all breakpoints in delete_breakpoints
UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: breakpoint
UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: begin commands in bp_deleted_in_command_test
UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: add silent tbreak command
UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: add printf tbreak command
UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: add cont tbreak command
UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: end tbreak commands
UNRESOLVED: gdb.base/commands.exp: stray_arg0_test: #1
UNRESOLVED: gdb.base/commands.exp: stray_arg0_test: #2
UNRESOLVED: gdb.base/commands.exp: stray_arg0_test: #3
UNRESOLVED: gdb.base/commands.exp: stray_arg0_test: #4
UNRESOLVED: gdb.base/commands.exp: source_file_with_indented_comment: source file
UNRESOLVED: gdb.base/commands.exp: recursive_source_test: source file
UNRESOLVED: gdb.base/commands.exp: if_commands_test: set $tem
UNRESOLVED: gdb.base/commands.exp: if_commands_test: if $tem == 2 - if_commands_test 1
UNRESOLVED: gdb.base/commands.exp: if_commands_test: break -q main - if_commands_test 1
UNRESOLVED: gdb.base/commands.exp: if_commands_test: else - if_commands_test 1
UNRESOLVED: gdb.base/commands.exp: if_commands_test: break factorial - if_commands_test 1
UNRESOLVED: gdb.base/commands.exp: if_commands_test: commands - if_commands_test 1
UNRESOLVED: gdb.base/commands.exp: if_commands_test: silent - if_commands_test 1
UNRESOLVED: gdb.base/commands.exp: if_commands_test: set $tem = 3 - if_commands_test 1
UNRESOLVED: gdb.base/commands.exp: if_commands_test: continue - if_commands_test 1
UNRESOLVED: gdb.base/commands.exp: if_commands_test: first end - if_commands_test 1
UNRESOLVED: gdb.base/commands.exp: if_commands_test: second end - if_commands_test 1
UNRESOLVED: gdb.base/commands.exp: if_commands_test: if $tem == 1 - if_commands_test 2
UNRESOLVED: gdb.base/commands.exp: if_commands_test: break -q main - if_commands_test 2
UNRESOLVED: gdb.base/commands.exp: if_commands_test: else - if_commands_test 2
UNRESOLVED: gdb.base/commands.exp: if_commands_test: break factorial - if_commands_test 2
UNRESOLVED: gdb.base/commands.exp: if_commands_test: commands - if_commands_test 2
UNRESOLVED: gdb.base/commands.exp: if_commands_test: silent - if_commands_test 2
UNRESOLVED: gdb.base/commands.exp: if_commands_test: set $tem = 3 - if_commands_test 2
UNRESOLVED: gdb.base/commands.exp: if_commands_test: continue - if_commands_test 2
UNRESOLVED: gdb.base/commands.exp: if_commands_test: first end - if_commands_test 2
UNRESOLVED: gdb.base/commands.exp: if_commands_test: second end - if_commands_test 2
UNRESOLVED: gdb.base/commands.exp: error_clears_commands_left: hook-stop 1
UNRESOLVED: gdb.base/commands.exp: error_clears_commands_left: hook-stop 1a
UNRESOLVED: gdb.base/commands.exp: error_clears_commands_left: hook-stop 1b
UNRESOLVED: gdb.base/commands.exp: error_clears_commands_left: delete all breakpoints in delete_breakpoints
UNRESOLVED: gdb.base/dprintf-execution-x-script.exp: load and run script with -x
UNRESOLVED: gdb.base/dprintf-execution-x-script.exp: load and run script using source command
UNRESOLVED: gdb.base/dprintf-execution-x-script.exp: run again
UNRESOLVED: gdb.base/save-bp.exp: source bps
UNRESOLVED: gdb.base/save-bp.exp: info break (pattern 1) (timeout)
UNRESOLVED: gdb.base/trace-commands.exp: source -v (pattern 4) (timeout)
UNRESOLVED: gdb.base/trace-commands.exp: set trace-commands
UNRESOLVED: gdb.base/trace-commands.exp: show trace-commands says on
UNRESOLVED: gdb.base/trace-commands.exp: simple trace-commands test
UNRESOLVED: gdb.base/trace-commands.exp: nested trace-commands test (pattern 1) (timeout)
UNRESOLVED: gdb.base/trace-commands.exp: define user command (pattern 1) (timeout)
UNRESOLVED: gdb.base/trace-commands.exp: nested trace-commands test with source (pattern 1) (timeout)
UNRESOLVED: gdb.base/trace-commands.exp: depth resets on error part 1 (pattern 1) (timeout)
UNRESOLVED: gdb.base/trace-commands.exp: depth resets on error part 2
UNRESOLVED: gdb.trace/save-trace.exp: relative: read back saved tracepoints
UNRESOLVED: gdb.trace/save-trace.exp: relative: verify recovered tracepoints
DUPLICATE: gdb.trace/save-trace.exp: relative: verify recovered tracepoints
UNRESOLVED: gdb.trace/save-trace.exp: relative: verify default-collect
UNRESOLVED: gdb.trace/save-trace.exp: absolute: save tracepoint definitions
UNRESOLVED: gdb.trace/save-trace.exp: absolute: clear default-collect
UNRESOLVED: gdb.trace/save-trace.exp: absolute: delete tracepoints
UNRESOLVED: gdb.trace/save-trace.exp: absolute: read back saved tracepoints
UNRESOLVED: gdb.trace/save-trace.exp: absolute: verify recovered tracepoints
DUPLICATE: gdb.trace/save-trace.exp: absolute: verify recovered tracepoints
UNRESOLVED: gdb.trace/save-trace.exp: absolute: verify default-collect
UNRESOLVED: gdb.trace/save-trace.exp: verify help save tracepoints

Simon

Re: [PATCH] gdb: fix command lookup in execute_command () commands"

[Jan Vrany]

On Tue, 2022-12-13 at 14:29 -0500, Simon Marchi wrote:
> On 12/13/22 13:48, Jan Vraný wrote:
> > On Tue, 2022-12-13 at 08:05 -0700, Tom Tromey wrote:
> > > > > > > > "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> > > 
> > > Jan> The below patch should fix this case - with it on my system
> > > Jan> gdb.base/define.exp passes.
> > > 
> > > Jan> Alternatively I may just revert the commit until better solution
> > > Jan> is found.
> > > 
> > > This patch looks ok to me, but could you say whether you ran all the
> > > tests or just define.exp?  Given the history I think a full regression
> > > test is warranted.
> > 
> > I did use try-build which succeeded (build #28) but it seems to me it
> > runs only very limited number of tests. 
> > 
> > I did run all of gdb.base and did not spot any new regression compared
> > to master with my (previous, broken) patch reverted. I did not run more
> > than gdb.base mainly because just gdb.base takes about an hour on my machine.
> > Also I get weird intermittent failures when running testsuite. 
> > 
> > Jan
> 
> Here are the unexpected failures I saw:
> 
> UNRESOLVED: gdb.base/bp-cmds-execution-x-script.exp: run to end
> UNRESOLVED: gdb.base/bp-cmds-run-with-ex.exp: execute bp commands
> UNRESOLVED: gdb.base/bp-cmds-sourced-script.exp: source the script
> UNRESOLVED: gdb.base/bp-cmds-sourced-script.exp: continue until exit
> UNRESOLVED: gdb.base/commands.exp: deprecated_command_test: source file containing xxx_yyy command and its alias
> UNRESOLVED: gdb.base/commands.exp: deprecated_command_test: deprecated alias with prefix give a warning
> UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: define real_command: input 1: define real_command
> UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: alias alias_command = real_command
> UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: alias alias_with_args_command = real_command 123
> UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: help real_command, before
> UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: maintenance deprecate alias_command
> UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: maintenance deprecate alias_with_args_command
> UNRESOLVED: gdb.base/commands.exp: deprecated_command_alias_help_test: help real_command, after
> UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: delete all breakpoints in delete_breakpoints
> UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: break factorial
> UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: begin commands
> UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: add silent command
> UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: add clear command
> UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: add printf command
> UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: add cont command
> UNRESOLVED: gdb.base/commands.exp: bp_deleted_in_command_test: end commands
> UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: delete all breakpoints in delete_breakpoints
> UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: breakpoint
> UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: begin commands in bp_deleted_in_command_test
> UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: add silent tbreak command
> UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: add printf tbreak command
> UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: add cont tbreak command
> UNRESOLVED: gdb.base/commands.exp: temporary_breakpoint_commands: end tbreak commands
> UNRESOLVED: gdb.base/commands.exp: stray_arg0_test: #1
> UNRESOLVED: gdb.base/commands.exp: stray_arg0_test: #2
> UNRESOLVED: gdb.base/commands.exp: stray_arg0_test: #3
> UNRESOLVED: gdb.base/commands.exp: stray_arg0_test: #4
> UNRESOLVED: gdb.base/commands.exp: source_file_with_indented_comment: source file
> UNRESOLVED: gdb.base/commands.exp: recursive_source_test: source file
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: set $tem
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: if $tem == 2 - if_commands_test 1
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: break -q main - if_commands_test 1
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: else - if_commands_test 1
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: break factorial - if_commands_test 1
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: commands - if_commands_test 1
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: silent - if_commands_test 1
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: set $tem = 3 - if_commands_test 1
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: continue - if_commands_test 1
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: first end - if_commands_test 1
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: second end - if_commands_test 1
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: if $tem == 1 - if_commands_test 2
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: break -q main - if_commands_test 2
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: else - if_commands_test 2
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: break factorial - if_commands_test 2
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: commands - if_commands_test 2
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: silent - if_commands_test 2
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: set $tem = 3 - if_commands_test 2
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: continue - if_commands_test 2
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: first end - if_commands_test 2
> UNRESOLVED: gdb.base/commands.exp: if_commands_test: second end - if_commands_test 2
> UNRESOLVED: gdb.base/commands.exp: error_clears_commands_left: hook-stop 1
> UNRESOLVED: gdb.base/commands.exp: error_clears_commands_left: hook-stop 1a
> UNRESOLVED: gdb.base/commands.exp: error_clears_commands_left: hook-stop 1b
> UNRESOLVED: gdb.base/commands.exp: error_clears_commands_left: delete all breakpoints in delete_breakpoints
> UNRESOLVED: gdb.base/dprintf-execution-x-script.exp: load and run script with -x
> UNRESOLVED: gdb.base/dprintf-execution-x-script.exp: load and run script using source command
> UNRESOLVED: gdb.base/dprintf-execution-x-script.exp: run again
> UNRESOLVED: gdb.base/save-bp.exp: source bps
> UNRESOLVED: gdb.base/save-bp.exp: info break (pattern 1) (timeout)
> UNRESOLVED: gdb.base/trace-commands.exp: source -v (pattern 4) (timeout)
> UNRESOLVED: gdb.base/trace-commands.exp: set trace-commands
> UNRESOLVED: gdb.base/trace-commands.exp: show trace-commands says on
> UNRESOLVED: gdb.base/trace-commands.exp: simple trace-commands test
> UNRESOLVED: gdb.base/trace-commands.exp: nested trace-commands test (pattern 1) (timeout)
> UNRESOLVED: gdb.base/trace-commands.exp: define user command (pattern 1) (timeout)
> UNRESOLVED: gdb.base/trace-commands.exp: nested trace-commands test with source (pattern 1) (timeout)
> UNRESOLVED: gdb.base/trace-commands.exp: depth resets on error part 1 (pattern 1) (timeout)
> UNRESOLVED: gdb.base/trace-commands.exp: depth resets on error part 2
> UNRESOLVED: gdb.trace/save-trace.exp: relative: read back saved tracepoints
> UNRESOLVED: gdb.trace/save-trace.exp: relative: verify recovered tracepoints
> DUPLICATE: gdb.trace/save-trace.exp: relative: verify recovered tracepoints
> UNRESOLVED: gdb.trace/save-trace.exp: relative: verify default-collect
> UNRESOLVED: gdb.trace/save-trace.exp: absolute: save tracepoint definitions
> UNRESOLVED: gdb.trace/save-trace.exp: absolute: clear default-collect
> UNRESOLVED: gdb.trace/save-trace.exp: absolute: delete tracepoints
> UNRESOLVED: gdb.trace/save-trace.exp: absolute: read back saved tracepoints
> UNRESOLVED: gdb.trace/save-trace.exp: absolute: verify recovered tracepoints
> DUPLICATE: gdb.trace/save-trace.exp: absolute: verify recovered tracepoints
> UNRESOLVED: gdb.trace/save-trace.exp: absolute: verify default-collect
> UNRESOLVED: gdb.trace/save-trace.exp: verify help save tracepoints
>

Hmm. I do not see these failures. When I run tests with second patch applied
atop of dc3fb44540 ("gdb/testsuite: avoid creating temp file in gdb/testsuite/ directory")
using command:

    make -C gdb check RUNTESTFLAGS='TRANSCRIPT=y gdb.base/*.exp'

Then I do not see them:

    $ grep UNRESOLVED gdb.sum.with-second-patch
    UNRESOLVED: gdb.base/gdb-sigterm.exp: 50 SIGTERM passes
    $ grep DUPLICATE gdb.sum.with-second-patch
    $

When I run gdb.base tests with on commit dc3fb44540 but with first
patch (b5661ff24) `git revert`ed, the only difference in tests I see is

$ diff -u gdb.sum.with-second-patch gdb.sum.with-first-patch-reverted
--- gdb.sum.with-second-patch	2022-12-14 09:14:46.790229064 +0000
+++ gdb.sum.with-first-patch-reverted	2022-12-14 09:40:53.460377645 +0000
@@ -1,4 +1,4 @@
-Test run by jv on Wed Dec 14 08:19:53 2022
+Test run by jv on Wed Dec 14 09:16:33 2022
 Native configuration is x86_64-pc-linux-gnu
 
 		=== gdb tests ===
@@ -1931,9 +1931,9 @@
 PASS: gdb.base/step-over-syscall.exp: clone: displaced=on: set displaced-stepping on
 PASS: gdb.base/step-over-syscall.exp: clone: displaced=on: single step over clone
 PASS: gdb.base/step-over-syscall.exp: clone: displaced=on: check_pc_after_cross_syscall: get hexadecimal valueof "$pc"
-PASS: gdb.base/step-over-syscall.exp: clone: displaced=on: check_pc_after_cross_syscall: single step over clone final pc
+KFAIL: gdb.base/step-over-syscall.exp: clone: displaced=on: check_pc_after_cross_syscall: single step over clone final pc (PRMS: gdb/19675)
 PASS: gdb.base/step-over-syscall.exp: clone: displaced=on: break marker
-KFAIL: gdb.base/step-over-syscall.exp: clone: displaced=on: continue to marker (clone) (PRMS: gdb/19675)
+KPASS: gdb.base/step-over-syscall.exp: clone: displaced=on: continue to marker (clone) (PRMS gdb/19675)
 Running /home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.base/siginfo-infcall.exp ...
 PASS: gdb.base/siginfo-infcall.exp: continue to SIGUSR1
 PASS: gdb.base/siginfo-infcall.exp: p callme ()
@@ -58069,9 +58069,10 @@
 
 		=== gdb Summary ===
 
-# of expected passes		57356
+# of expected passes		57355
 # of unexpected failures	6
 # of expected failures		33
+# of unknown successes		1
 # of known failures		32
 # of unresolved testcases	1
 # of untested testcases		6

gdb.base/step-over-syscall.exp seems to be one of the shaky tests on my machine.
It does not seem to define any custom commands nor does it use post-hooks so
I'd think it is unrelated (but I must admit I do not understand exactly what it
does and which parts of the system it excercises).

So, since the original patch (which I pushed little too hastily) clearly break things
and a fix causes problems elsewhere which I cannot reproduce yet, I suggest to
revert b5661ff2 "gdb: fix possible use-after-free when executing commands"
until I find a better solution (patch below)

What do you think?

-- >8 --
Subject: [PATCH] Revert "gdb: fix possible use-after-free when executing
 commands"

Commit b5661ff2 causes gdb.base/define.exp. Using lookup_cmd ()
instead lookup_cmd_exact () fixed this test but causes failures
elsewhere.

This reverts commit b5661ff2 until a better solution is found.
---
 gdb/top.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index 742997808bd..e9794184f07 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -655,11 +655,6 @@ execute_command (const char *p, int from_tty)
 	    }
 	}
 
-      /* Remember name of the command.  This is needed later when
-	 executing command post-hooks to handle the case when command
-	 is redefined or removed during it's execution.  See below.  */
-      std::string c_name (c->name);
-
       /* If this command has been pre-hooked, run the hook first.  */
       execute_cmd_pre_hook (c);
 
@@ -698,13 +693,8 @@ execute_command (const char *p, int from_tty)
 
       maybe_wait_sync_command_done (was_sync);
 
-      /* If this command has been post-hooked, run the hook last.
-	 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.  */
-      c = lookup_cmd_exact (c_name.c_str (), cmdlist);
-      if (c != nullptr)
-	execute_cmd_post_hook (c);
+      /* If this command has been post-hooked, run the hook last.  */
+      execute_cmd_post_hook (c);
 
       if (repeat_arguments != NULL && cmd_start == saved_command_line)
 	{
-- 
2.35.1

Re: [PATCH] gdb: fix command lookup in execute_command () commands"

[Simon Marchi]

> Hmm. I do not see these failures. When I run tests with second patch applied
> atop of dc3fb44540 ("gdb/testsuite: avoid creating temp file in gdb/testsuite/ directory")
> using command:
> 
>     make -C gdb check RUNTESTFLAGS='TRANSCRIPT=y gdb.base/*.exp'
> 
> Then I do not see them:
> 
>     $ grep UNRESOLVED gdb.sum.with-second-patch
>     UNRESOLVED: gdb.base/gdb-sigterm.exp: 50 SIGTERM passes
>     $ grep DUPLICATE gdb.sum.with-second-patch
>     $

Can you try builing with ASan, or run the test under Valgrind?

This is the kind of failures I see ASan reporting:

=================================================================
==2304586==ERROR: AddressSanitizer: heap-use-after-free on address 0x60300005d340 at pc 0x564a4ec9220a bp 0x7ffd634643f0 sp 0x7ffd634643e0
READ of size 1 at 0x60300005d340 thread T0
    #0 0x564a4ec92209 in lookup_cmd_1(char const**, cmd_list_element*, cmd_list_element**, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, int, bool) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:1990
    #1 0x564a4ec9338e in lookup_cmd(char const**, cmd_list_element*, char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, int, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2147
    #2 0x564a50c8e3b2 in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:701
    #3 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
    #4 0x564a50c8c1d6 in read_command_file(_IO_FILE*) /home/smarchi/src/binutils-gdb/gdb/top.c:461
    #5 0x564a4ed1ea61 in script_from_file(_IO_FILE*, char const*) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1638
    #6 0x564a4ec4cff3 in source_script_from_stream /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:728
    #7 0x564a4ec4d43b in source_script_with_search /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:773
    #8 0x564a4ec4da65 in source_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:831
    #9 0x564a4ec82cbc in do_simple_func /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:95
    #10 0x564a4ec99059 in cmd_func(cmd_list_element*, char const*, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2543
    #11 0x564a50c8e30d in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:692
    #12 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
    #13 0x564a4f67ed39 in command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) /home/smarchi/src/binutils-gdb/gdb/event-top.c:860
    #14 0x564a50ddec69 in tui_command_line_handler /home/smarchi/src/binutils-gdb/gdb/tui/tui-interp.c:104
    #15 0x564a4f67baef in gdb_rl_callback_handler /home/smarchi/src/binutils-gdb/gdb/event-top.c:246
    #16 0x564a512a5683 in rl_callback_read_char /home/smarchi/src/binutils-gdb/readline/readline/callback.c:290
    #17 0x564a4f67b4d9 in gdb_rl_callback_read_char_wrapper_noexcept /home/smarchi/src/binutils-gdb/gdb/event-top.c:188
    #18 0x564a4f67b735 in gdb_rl_callback_read_char_wrapper /home/smarchi/src/binutils-gdb/gdb/event-top.c:221
    #19 0x564a4f67d180 in stdin_event_handler /home/smarchi/src/binutils-gdb/gdb/event-top.c:541
    #20 0x564a51682d64 in handle_file_event /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:573
    #21 0x564a516836a3 in gdb_wait_for_event /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:694
    #22 0x564a5168133c in gdb_do_one_event(int) /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:264
    #23 0x564a4fdc136a in start_event_loop /home/smarchi/src/binutils-gdb/gdb/main.c:411
    #24 0x564a4fdc17b0 in captured_command_loop /home/smarchi/src/binutils-gdb/gdb/main.c:471
    #25 0x564a4fdc6bfb in captured_main /home/smarchi/src/binutils-gdb/gdb/main.c:1330
    #26 0x564a4fdc6ce4 in gdb_main(captured_main_args*) /home/smarchi/src/binutils-gdb/gdb/main.c:1345
    #27 0x564a4e35e33c in main /home/smarchi/src/binutils-gdb/gdb/gdb.c:32
    #28 0x7f3fb381cd8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #29 0x7f3fb381ce3f in __libc_start_main_impl ../csu/libc-start.c:392
    #30 0x564a4e35e114 in _start (/home/smarchi/build/binutils-gdb/gdb/gdb+0xa896114)

0x60300005d340 is located 0 bytes inside of 32-byte region [0x60300005d340,0x60300005d360)
freed by thread T0 here:
    #0 0x7f3fb515bc18 in __interceptor_realloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:164
    #1 0x564a4e603a29 in xrealloc /home/smarchi/src/binutils-gdb/gdb/alloc.c:75
    #2 0x564a51669289 in buffer_grow(buffer*, char const*, unsigned long) /home/smarchi/src/binutils-gdb/gdbsupport/buffer.cc:40
    #3 0x564a4f67de58 in command_line_append_input_line /home/smarchi/src/binutils-gdb/gdb/event-top.c:647
    #4 0x564a4f67e0ce in handle_line_of_input(buffer*, char const*, int, char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:686
    #5 0x564a50c93555 in command_line_input(char const*, char const*) /home/smarchi/src/binutils-gdb/gdb/top.c:1421
    #6 0x564a4ed0cc32 in read_next_line /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:920
    #7 0x564a4ed286f6 in gdb::function_view<char const* ()>::bind<char const*>(char const* (*)())::{lambda(gdb::fv_detail::erased_callable)#1}::operator()(gdb::fv_detail::erased_callable) const /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:326
    #8 0x564a4ed28772 in gdb::function_view<char const* ()>::bind<char const*>(char const* (*)())::{lambda(gdb::fv_detail::erased_callable)#1}::_FUN(gdb::fv_detail::erased_callable) /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:320
    #9 0x564a4ed25288 in gdb::function_view<char const* ()>::operator()() const /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:289
    #10 0x564a4ed1adf2 in read_command_lines_1(gdb::function_view<char const* ()>, int, gdb::function_view<void (char const*)>) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1234
    #11 0x564a4ed1a997 in read_command_lines(char const*, int, int, gdb::function_view<void (char const*)>) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1202
    #12 0x564a4ed1c8e6 in do_define_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1450
    #13 0x564a4ed1d2bc in define_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1500
    #14 0x564a4ec82cbc in do_simple_func /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:95
    #15 0x564a4ec99059 in cmd_func(cmd_list_element*, char const*, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2543
    #16 0x564a50c8e30d in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:692
    #17 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
    #18 0x564a50c8c1d6 in read_command_file(_IO_FILE*) /home/smarchi/src/binutils-gdb/gdb/top.c:461
    #19 0x564a4ed1ea61 in script_from_file(_IO_FILE*, char const*) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1638
    #20 0x564a4ec4cff3 in source_script_from_stream /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:728
    #21 0x564a4ec4d43b in source_script_with_search /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:773
    #22 0x564a4ec4da65 in source_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:831
    #23 0x564a4ec82cbc in do_simple_func /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:95
    #24 0x564a4ec99059 in cmd_func(cmd_list_element*, char const*, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2543
    #25 0x564a50c8e30d in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:692
    #26 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
    #27 0x564a4f67ed39 in command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) /home/smarchi/src/binutils-gdb/gdb/event-top.c:860
    #28 0x564a50ddec69 in tui_command_line_handler /home/smarchi/src/binutils-gdb/gdb/tui/tui-interp.c:104
    #29 0x564a4f67baef in gdb_rl_callback_handler /home/smarchi/src/binutils-gdb/gdb/event-top.c:246

previously allocated by thread T0 here:
    #0 0x7f3fb515b867 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x564a4e603a3b in xrealloc /home/smarchi/src/binutils-gdb/gdb/alloc.c:77
    #2 0x564a51669289 in buffer_grow(buffer*, char const*, unsigned long) /home/smarchi/src/binutils-gdb/gdbsupport/buffer.cc:40
    #3 0x564a4f67de58 in command_line_append_input_line /home/smarchi/src/binutils-gdb/gdb/event-top.c:647
    #4 0x564a4f67e0ce in handle_line_of_input(buffer*, char const*, int, char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:686
    #5 0x564a50c93555 in command_line_input(char const*, char const*) /home/smarchi/src/binutils-gdb/gdb/top.c:1421
    #6 0x564a50c8c1b2 in read_command_file(_IO_FILE*) /home/smarchi/src/binutils-gdb/gdb/top.c:458
    #7 0x564a4ed1ea61 in script_from_file(_IO_FILE*, char const*) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1638
    #8 0x564a4ec4cff3 in source_script_from_stream /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:728
    #9 0x564a4ec4d43b in source_script_with_search /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:773
    #10 0x564a4ec4da65 in source_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:831
    #11 0x564a4ec82cbc in do_simple_func /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:95
    #12 0x564a4ec99059 in cmd_func(cmd_list_element*, char const*, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2543
    #13 0x564a50c8e30d in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:692
    #14 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
    #15 0x564a4f67ed39 in command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) /home/smarchi/src/binutils-gdb/gdb/event-top.c:860
    #16 0x564a50ddec69 in tui_command_line_handler /home/smarchi/src/binutils-gdb/gdb/tui/tui-interp.c:104
    #17 0x564a4f67baef in gdb_rl_callback_handler /home/smarchi/src/binutils-gdb/gdb/event-top.c:246
    #18 0x564a512a5683 in rl_callback_read_char /home/smarchi/src/binutils-gdb/readline/readline/callback.c:290
    #19 0x564a4f67b4d9 in gdb_rl_callback_read_char_wrapper_noexcept /home/smarchi/src/binutils-gdb/gdb/event-top.c:188
    #20 0x564a4f67b735 in gdb_rl_callback_read_char_wrapper /home/smarchi/src/binutils-gdb/gdb/event-top.c:221
    #21 0x564a4f67d180 in stdin_event_handler /home/smarchi/src/binutils-gdb/gdb/event-top.c:541
    #22 0x564a51682d64 in handle_file_event /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:573
    #23 0x564a516836a3 in gdb_wait_for_event /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:694
    #24 0x564a5168133c in gdb_do_one_event(int) /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:264
    #25 0x564a4fdc136a in start_event_loop /home/smarchi/src/binutils-gdb/gdb/main.c:411
    #26 0x564a4fdc17b0 in captured_command_loop /home/smarchi/src/binutils-gdb/gdb/main.c:471
    #27 0x564a4fdc6bfb in captured_main /home/smarchi/src/binutils-gdb/gdb/main.c:1330
    #28 0x564a4fdc6ce4 in gdb_main(captured_main_args*) /home/smarchi/src/binutils-gdb/gdb/main.c:1345
    #29 0x564a4e35e33c in main /home/smarchi/src/binutils-gdb/gdb/gdb.c:32

Simon

Re: [PATCH] gdb: fix command lookup in execute_command () commands"

[Jan Vraný]

On Wed, 2022-12-14 at 10:35 -0500, Simon Marchi wrote:
> > Hmm. I do not see these failures. When I run tests with second patch applied
> > atop of dc3fb44540 ("gdb/testsuite: avoid creating temp file in gdb/testsuite/ directory")
> > using command:
> > 
> >     make -C gdb check RUNTESTFLAGS='TRANSCRIPT=y gdb.base/*.exp'
> > 
> > Then I do not see them:
> > 
> >     $ grep UNRESOLVED gdb.sum.with-second-patch
> >     UNRESOLVED: gdb.base/gdb-sigterm.exp: 50 SIGTERM passes
> >     $ grep DUPLICATE gdb.sum.with-second-patch
> >     $
> 
> Can you try builing with ASan, or run the test under Valgrind?
> 
> This is the kind of failures I see ASan reporting:

Yes, I can try. Probably tomorrow or Friday...

Thanks!

Jan

> 
> =================================================================
> ==2304586==ERROR: AddressSanitizer: heap-use-after-free on address 0x60300005d340 at pc 0x564a4ec9220a bp 0x7ffd634643f0 sp 0x7ffd634643e0
> READ of size 1 at 0x60300005d340 thread T0
>     #0 0x564a4ec92209 in lookup_cmd_1(char const**, cmd_list_element*, cmd_list_element**, std::__cxx11::basic_string<char, std::char_traits<char>,
> std::allocator<char> >*, int, bool) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:1990
>     #1 0x564a4ec9338e in lookup_cmd(char const**, cmd_list_element*, char const*, std::__cxx11::basic_string<char, std::char_traits<char>,
> std::allocator<char> >*, int, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2147
>     #2 0x564a50c8e3b2 in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:701
>     #3 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
>     #4 0x564a50c8c1d6 in read_command_file(_IO_FILE*) /home/smarchi/src/binutils-gdb/gdb/top.c:461
>     #5 0x564a4ed1ea61 in script_from_file(_IO_FILE*, char const*) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1638
>     #6 0x564a4ec4cff3 in source_script_from_stream /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:728
>     #7 0x564a4ec4d43b in source_script_with_search /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:773
>     #8 0x564a4ec4da65 in source_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:831
>     #9 0x564a4ec82cbc in do_simple_func /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:95
>     #10 0x564a4ec99059 in cmd_func(cmd_list_element*, char const*, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2543
>     #11 0x564a50c8e30d in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:692
>     #12 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
>     #13 0x564a4f67ed39 in command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) /home/smarchi/src/binutils-gdb/gdb/event-top.c:860
>     #14 0x564a50ddec69 in tui_command_line_handler /home/smarchi/src/binutils-gdb/gdb/tui/tui-interp.c:104
>     #15 0x564a4f67baef in gdb_rl_callback_handler /home/smarchi/src/binutils-gdb/gdb/event-top.c:246
>     #16 0x564a512a5683 in rl_callback_read_char /home/smarchi/src/binutils-gdb/readline/readline/callback.c:290
>     #17 0x564a4f67b4d9 in gdb_rl_callback_read_char_wrapper_noexcept /home/smarchi/src/binutils-gdb/gdb/event-top.c:188
>     #18 0x564a4f67b735 in gdb_rl_callback_read_char_wrapper /home/smarchi/src/binutils-gdb/gdb/event-top.c:221
>     #19 0x564a4f67d180 in stdin_event_handler /home/smarchi/src/binutils-gdb/gdb/event-top.c:541
>     #20 0x564a51682d64 in handle_file_event /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:573
>     #21 0x564a516836a3 in gdb_wait_for_event /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:694
>     #22 0x564a5168133c in gdb_do_one_event(int) /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:264
>     #23 0x564a4fdc136a in start_event_loop /home/smarchi/src/binutils-gdb/gdb/main.c:411
>     #24 0x564a4fdc17b0 in captured_command_loop /home/smarchi/src/binutils-gdb/gdb/main.c:471
>     #25 0x564a4fdc6bfb in captured_main /home/smarchi/src/binutils-gdb/gdb/main.c:1330
>     #26 0x564a4fdc6ce4 in gdb_main(captured_main_args*) /home/smarchi/src/binutils-gdb/gdb/main.c:1345
>     #27 0x564a4e35e33c in main /home/smarchi/src/binutils-gdb/gdb/gdb.c:32
>     #28 0x7f3fb381cd8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
>     #29 0x7f3fb381ce3f in __libc_start_main_impl ../csu/libc-start.c:392
>     #30 0x564a4e35e114 in _start (/home/smarchi/build/binutils-gdb/gdb/gdb+0xa896114)
> 
> 0x60300005d340 is located 0 bytes inside of 32-byte region [0x60300005d340,0x60300005d360)
> freed by thread T0 here:
>     #0 0x7f3fb515bc18 in __interceptor_realloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:164
>     #1 0x564a4e603a29 in xrealloc /home/smarchi/src/binutils-gdb/gdb/alloc.c:75
>     #2 0x564a51669289 in buffer_grow(buffer*, char const*, unsigned long) /home/smarchi/src/binutils-gdb/gdbsupport/buffer.cc:40
>     #3 0x564a4f67de58 in command_line_append_input_line /home/smarchi/src/binutils-gdb/gdb/event-top.c:647
>     #4 0x564a4f67e0ce in handle_line_of_input(buffer*, char const*, int, char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:686
>     #5 0x564a50c93555 in command_line_input(char const*, char const*) /home/smarchi/src/binutils-gdb/gdb/top.c:1421
>     #6 0x564a4ed0cc32 in read_next_line /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:920
>     #7 0x564a4ed286f6 in gdb::function_view<char const* ()>::bind<char const*>(char const*
> (*)())::{lambda(gdb::fv_detail::erased_callable)#1}::operator()(gdb::fv_detail::erased_callable) const /home/smarchi/src/binutils-
> gdb/gdb/../gdbsupport/function-view.h:326
>     #8 0x564a4ed28772 in gdb::function_view<char const* ()>::bind<char const*>(char const*
> (*)())::{lambda(gdb::fv_detail::erased_callable)#1}::_FUN(gdb::fv_detail::erased_callable) /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-
> view.h:320
>     #9 0x564a4ed25288 in gdb::function_view<char const* ()>::operator()() const /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:289
>     #10 0x564a4ed1adf2 in read_command_lines_1(gdb::function_view<char const* ()>, int, gdb::function_view<void (char const*)>) /home/smarchi/src/binutils-
> gdb/gdb/cli/cli-script.c:1234
>     #11 0x564a4ed1a997 in read_command_lines(char const*, int, int, gdb::function_view<void (char const*)>) /home/smarchi/src/binutils-gdb/gdb/cli/cli-
> script.c:1202
>     #12 0x564a4ed1c8e6 in do_define_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1450
>     #13 0x564a4ed1d2bc in define_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1500
>     #14 0x564a4ec82cbc in do_simple_func /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:95
>     #15 0x564a4ec99059 in cmd_func(cmd_list_element*, char const*, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2543
>     #16 0x564a50c8e30d in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:692
>     #17 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
>     #18 0x564a50c8c1d6 in read_command_file(_IO_FILE*) /home/smarchi/src/binutils-gdb/gdb/top.c:461
>     #19 0x564a4ed1ea61 in script_from_file(_IO_FILE*, char const*) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1638
>     #20 0x564a4ec4cff3 in source_script_from_stream /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:728
>     #21 0x564a4ec4d43b in source_script_with_search /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:773
>     #22 0x564a4ec4da65 in source_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:831
>     #23 0x564a4ec82cbc in do_simple_func /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:95
>     #24 0x564a4ec99059 in cmd_func(cmd_list_element*, char const*, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2543
>     #25 0x564a50c8e30d in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:692
>     #26 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
>     #27 0x564a4f67ed39 in command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) /home/smarchi/src/binutils-gdb/gdb/event-top.c:860
>     #28 0x564a50ddec69 in tui_command_line_handler /home/smarchi/src/binutils-gdb/gdb/tui/tui-interp.c:104
>     #29 0x564a4f67baef in gdb_rl_callback_handler /home/smarchi/src/binutils-gdb/gdb/event-top.c:246
> 
> previously allocated by thread T0 here:
>     #0 0x7f3fb515b867 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
>     #1 0x564a4e603a3b in xrealloc /home/smarchi/src/binutils-gdb/gdb/alloc.c:77
>     #2 0x564a51669289 in buffer_grow(buffer*, char const*, unsigned long) /home/smarchi/src/binutils-gdb/gdbsupport/buffer.cc:40
>     #3 0x564a4f67de58 in command_line_append_input_line /home/smarchi/src/binutils-gdb/gdb/event-top.c:647
>     #4 0x564a4f67e0ce in handle_line_of_input(buffer*, char const*, int, char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:686
>     #5 0x564a50c93555 in command_line_input(char const*, char const*) /home/smarchi/src/binutils-gdb/gdb/top.c:1421
>     #6 0x564a50c8c1b2 in read_command_file(_IO_FILE*) /home/smarchi/src/binutils-gdb/gdb/top.c:458
>     #7 0x564a4ed1ea61 in script_from_file(_IO_FILE*, char const*) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1638
>     #8 0x564a4ec4cff3 in source_script_from_stream /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:728
>     #9 0x564a4ec4d43b in source_script_with_search /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:773
>     #10 0x564a4ec4da65 in source_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:831
>     #11 0x564a4ec82cbc in do_simple_func /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:95
>     #12 0x564a4ec99059 in cmd_func(cmd_list_element*, char const*, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2543
>     #13 0x564a50c8e30d in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:692
>     #14 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
>     #15 0x564a4f67ed39 in command_line_handler(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) /home/smarchi/src/binutils-gdb/gdb/event-top.c:860
>     #16 0x564a50ddec69 in tui_command_line_handler /home/smarchi/src/binutils-gdb/gdb/tui/tui-interp.c:104
>     #17 0x564a4f67baef in gdb_rl_callback_handler /home/smarchi/src/binutils-gdb/gdb/event-top.c:246
>     #18 0x564a512a5683 in rl_callback_read_char /home/smarchi/src/binutils-gdb/readline/readline/callback.c:290
>     #19 0x564a4f67b4d9 in gdb_rl_callback_read_char_wrapper_noexcept /home/smarchi/src/binutils-gdb/gdb/event-top.c:188
>     #20 0x564a4f67b735 in gdb_rl_callback_read_char_wrapper /home/smarchi/src/binutils-gdb/gdb/event-top.c:221
>     #21 0x564a4f67d180 in stdin_event_handler /home/smarchi/src/binutils-gdb/gdb/event-top.c:541
>     #22 0x564a51682d64 in handle_file_event /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:573
>     #23 0x564a516836a3 in gdb_wait_for_event /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:694
>     #24 0x564a5168133c in gdb_do_one_event(int) /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:264
>     #25 0x564a4fdc136a in start_event_loop /home/smarchi/src/binutils-gdb/gdb/main.c:411
>     #26 0x564a4fdc17b0 in captured_command_loop /home/smarchi/src/binutils-gdb/gdb/main.c:471
>     #27 0x564a4fdc6bfb in captured_main /home/smarchi/src/binutils-gdb/gdb/main.c:1330
>     #28 0x564a4fdc6ce4 in gdb_main(captured_main_args*) /home/smarchi/src/binutils-gdb/gdb/main.c:1345
>     #29 0x564a4e35e33c in main /home/smarchi/src/binutils-gdb/gdb/gdb.c:32
> 
> Simon
> 

Re: [PATCH] gdb: fix command lookup in execute_command () commands"

[Tom Tromey]

Simon> Here are the unexpected failures I saw:

[...]

Jan> Hmm. I do not see these failures.

I applied the patch and ran the full test suite.  I'm on x86-64 Fedora 34.

I see two new FAILs but they are in thread tests and so I assume they
are just the usual kind of racy test thing.

I didn't see any of the failures that Simon reports.

So, from my perspective your proposed patch seems fine.  I don't think
we need to back out the previous patch.

Simon, could you maybe double-check your results somehow?  Or pick one
of the failures and try to see why it's happening?

thanks,
Tom

Re: [PATCH] gdb: fix command lookup in execute_command () commands"

[Simon Marchi]

On 12/14/22 10:59, Tom Tromey wrote:
> Simon> Here are the unexpected failures I saw:
> 
> [...]
> 
> Jan> Hmm. I do not see these failures.
> 
> I applied the patch and ran the full test suite.  I'm on x86-64 Fedora 34.
> 
> I see two new FAILs but they are in thread tests and so I assume they
> are just the usual kind of racy test thing.
> 
> I didn't see any of the failures that Simon reports.
> 
> So, from my perspective your proposed patch seems fine.  I don't think
> we need to back out the previous patch.
> 
> Simon, could you maybe double-check your results somehow?  Or pick one
> of the failures and try to see why it's happening?
> 
> thanks,
> Tom

Do you have ASan enabled in that build?

Simon

Re: [PATCH] gdb: fix command lookup in execute_command () commands"

[Tom Tromey]

Simon> Do you have ASan enabled in that build?

Nope, but I tried it and I was able to reproduce the problem.
I should have thought of that earlier :{

Tom

Re: [PATCH] gdb: fix command lookup in execute_command () commands"

[Simon Marchi]

On 12/14/22 13:05, Tom Tromey wrote:
> Simon> Do you have ASan enabled in that build?
> 
> Nope, but I tried it and I was able to reproduce the problem.
> I should have thought of that earlier :{
> 
> Tom

I think the problem is the static buffer used in command_line_input.

The "define set xxx_yyy" command is read into the static buffer here:

    #5 0x564a50c93555 in command_line_input(char const*, char const*) /home/smarchi/src/binutils-gdb/gdb/top.c:1421
    #6 0x564a50c8c1b2 in read_command_file(_IO_FILE*) /home/smarchi/src/binutils-gdb/gdb/top.c:458
    #7 0x564a4ed1ea61 in script_from_file(_IO_FILE*, char const*) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1638
    #8 0x564a4ec4cff3 in source_script_from_stream /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:728
    #9 0x564a4ec4d43b in source_script_with_search /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:773
    #10 0x564a4ec4da65 in source_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:831

That same buffer is used for reading the lines inside the define, here:

    #5 0x564a50c93555 in command_line_input(char const*, char const*) /home/smarchi/src/binutils-gdb/gdb/top.c:1421
    #6 0x564a4ed0cc32 in read_next_line /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:920
    #7 0x564a4ed286f6 in gdb::function_view<char const* ()>::bind<char const*>(char const* (*)())::{lambda(gdb::fv_detail::erased_callable)#1}::operator()(gdb::fv_detail::erased_callable) const /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:326
    #8 0x564a4ed28772 in gdb::function_view<char const* ()>::bind<char const*>(char const* (*)())::{lambda(gdb::fv_detail::erased_callable)#1}::_FUN(gdb::fv_detail::erased_callable) /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:320
    #9 0x564a4ed25288 in gdb::function_view<char const* ()>::operator()() const /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:289
    #10 0x564a4ed1adf2 in read_command_lines_1(gdb::function_view<char const* ()>, int, gdb::function_view<void (char const*)>) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1234
    #11 0x564a4ed1a997 in read_command_lines(char const*, int, int, gdb::function_view<void (char const*)>) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1202
    #12 0x564a4ed1c8e6 in do_define_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1450
    #13 0x564a4ed1d2bc in define_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1500
    #14 0x564a4ec82cbc in do_simple_func /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:95
    #15 0x564a4ec99059 in cmd_func(cmd_list_element*, char const*, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2543
    #16 0x564a50c8e30d in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:692
    #17 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
    #18 0x564a50c8c1d6 in read_command_file(_IO_FILE*) /home/smarchi/src/binutils-gdb/gdb/top.c:461
    #19 0x564a4ed1ea61 in script_from_file(_IO_FILE*, char const*) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1638
    #20 0x564a4ec4cff3 in source_script_from_stream /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:728
    #21 0x564a4ec4d43b in source_script_with_search /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:773
    #22 0x564a4ec4da65 in source_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:831

It then crashes when trying to look up again the command, for the
post-hook, as the pointer to the command text (pointing to the string
that used to be owned by the static buffer) is now stale:

    #0 0x564a4ec92209 in lookup_cmd_1(char const**, cmd_list_element*, cmd_list_element**, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, int, bool) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:1990
    #1 0x564a4ec9338e in lookup_cmd(char const**, cmd_list_element*, char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, int, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2147
    #2 0x564a50c8e3b2 in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:701
    #3 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
    #4 0x564a50c8c1d6 in read_command_file(_IO_FILE*) /home/smarchi/src/binutils-gdb/gdb/top.c:461
    #5 0x564a4ed1ea61 in script_from_file(_IO_FILE*, char const*) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1638
    #6 0x564a4ec4cff3 in source_script_from_stream /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:728
    #7 0x564a4ec4d43b in source_script_with_search /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:773
    #8 0x564a4ec4da65 in source_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:831

From the point of view of execute_command, its `p` argument unexpectedly
becomes stale after calling cmd_func.  A quick fix would be to make
execute_command duplicate `p` on entry and use that.  However, I think
the right fix would be to get rid of the static buffer, have
command_line_input return an allocated buffer / string.  Otherwise, it's
just adding complexity to an area that is difficult to understand fully.

Simon

Re: [PATCH] gdb: fix command lookup in execute_command () commands"

[Simon Marchi]

On 12/14/22 13:30, Simon Marchi via Gdb-patches wrote:
> 
> 
> On 12/14/22 13:05, Tom Tromey wrote:
>> Simon> Do you have ASan enabled in that build?
>>
>> Nope, but I tried it and I was able to reproduce the problem.
>> I should have thought of that earlier :{
>>
>> Tom
> 
> I think the problem is the static buffer used in command_line_input.
> 
> The "define set xxx_yyy" command is read into the static buffer here:
> 
>     #5 0x564a50c93555 in command_line_input(char const*, char const*) /home/smarchi/src/binutils-gdb/gdb/top.c:1421
>     #6 0x564a50c8c1b2 in read_command_file(_IO_FILE*) /home/smarchi/src/binutils-gdb/gdb/top.c:458
>     #7 0x564a4ed1ea61 in script_from_file(_IO_FILE*, char const*) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1638
>     #8 0x564a4ec4cff3 in source_script_from_stream /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:728
>     #9 0x564a4ec4d43b in source_script_with_search /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:773
>     #10 0x564a4ec4da65 in source_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:831
> 
> That same buffer is used for reading the lines inside the define, here:
> 
>     #5 0x564a50c93555 in command_line_input(char const*, char const*) /home/smarchi/src/binutils-gdb/gdb/top.c:1421
>     #6 0x564a4ed0cc32 in read_next_line /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:920
>     #7 0x564a4ed286f6 in gdb::function_view<char const* ()>::bind<char const*>(char const* (*)())::{lambda(gdb::fv_detail::erased_callable)#1}::operator()(gdb::fv_detail::erased_callable) const /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:326
>     #8 0x564a4ed28772 in gdb::function_view<char const* ()>::bind<char const*>(char const* (*)())::{lambda(gdb::fv_detail::erased_callable)#1}::_FUN(gdb::fv_detail::erased_callable) /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:320
>     #9 0x564a4ed25288 in gdb::function_view<char const* ()>::operator()() const /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:289
>     #10 0x564a4ed1adf2 in read_command_lines_1(gdb::function_view<char const* ()>, int, gdb::function_view<void (char const*)>) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1234
>     #11 0x564a4ed1a997 in read_command_lines(char const*, int, int, gdb::function_view<void (char const*)>) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1202
>     #12 0x564a4ed1c8e6 in do_define_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1450
>     #13 0x564a4ed1d2bc in define_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1500
>     #14 0x564a4ec82cbc in do_simple_func /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:95
>     #15 0x564a4ec99059 in cmd_func(cmd_list_element*, char const*, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2543
>     #16 0x564a50c8e30d in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:692
>     #17 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
>     #18 0x564a50c8c1d6 in read_command_file(_IO_FILE*) /home/smarchi/src/binutils-gdb/gdb/top.c:461
>     #19 0x564a4ed1ea61 in script_from_file(_IO_FILE*, char const*) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1638
>     #20 0x564a4ec4cff3 in source_script_from_stream /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:728
>     #21 0x564a4ec4d43b in source_script_with_search /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:773
>     #22 0x564a4ec4da65 in source_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:831
> 
> It then crashes when trying to look up again the command, for the
> post-hook, as the pointer to the command text (pointing to the string
> that used to be owned by the static buffer) is now stale:
> 
>     #0 0x564a4ec92209 in lookup_cmd_1(char const**, cmd_list_element*, cmd_list_element**, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, int, bool) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:1990
>     #1 0x564a4ec9338e in lookup_cmd(char const**, cmd_list_element*, char const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, int, int) /home/smarchi/src/binutils-gdb/gdb/cli/cli-decode.c:2147
>     #2 0x564a50c8e3b2 in execute_command(char const*, int) /home/smarchi/src/binutils-gdb/gdb/top.c:701
>     #3 0x564a4f67dc89 in command_handler(char const*) /home/smarchi/src/binutils-gdb/gdb/event-top.c:616
>     #4 0x564a50c8c1d6 in read_command_file(_IO_FILE*) /home/smarchi/src/binutils-gdb/gdb/top.c:461
>     #5 0x564a4ed1ea61 in script_from_file(_IO_FILE*, char const*) /home/smarchi/src/binutils-gdb/gdb/cli/cli-script.c:1638
>     #6 0x564a4ec4cff3 in source_script_from_stream /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:728
>     #7 0x564a4ec4d43b in source_script_with_search /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:773
>     #8 0x564a4ec4da65 in source_command /home/smarchi/src/binutils-gdb/gdb/cli/cli-cmds.c:831
> 
> From the point of view of execute_command, its `p` argument unexpectedly
> becomes stale after calling cmd_func.  A quick fix would be to make
> execute_command duplicate `p` on entry and use that.  However, I think
> the right fix would be to get rid of the static buffer, have
> command_line_input return an allocated buffer / string.  Otherwise, it's
> just adding complexity to an area that is difficult to understand fully.
> 
> Simon

Just FYI and to avoid duplicate work, I have a patch that looks
promising, that removes the static buffer and uses std::string
throughout these functions (making managing memory easier than with
struct buffer):

https://review.lttng.org/c/binutils-gdb/+/9175

I'm currently waiting for my CI task to run, and I will also need to
write the commit message (tomorrow).

Simon

Re: [PATCH] gdb: fix command lookup in execute_command ()

[Jan Vraný]

On Tue, 2022-12-13 at 14:29 -0500, Simon Marchi wrote:
> On 12/13/22 13:48, Jan Vraný wrote:
> > On Tue, 2022-12-13 at 08:05 -0700, Tom Tromey wrote:
> > > > > > > > "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> > > 
> > > Jan> The below patch should fix this case - with it on my system
> > > Jan> gdb.base/define.exp passes.
> > > 
> > > Jan> Alternatively I may just revert the commit until better solution
> > > Jan> is found.
> > > 
> > > This patch looks ok to me, but could you say whether you ran all the
> > > tests or just define.exp?  Given the history I think a full regression
> > > test is warranted.
> > 
> > I did use try-build which succeeded (build #28) but it seems to me it
> > runs only very limited number of tests. 
> > 
> > I did run all of gdb.base and did not spot any new regression compared
> > to master with my (previous, broken) patch reverted. I did not run more
> > than gdb.base mainly because just gdb.base takes about an hour on my machine.
> > Also I get weird intermittent failures when running testsuite. 
> > 
> > Jan
> 
> Here are the unexpected failures I saw:
> 
> UNRESOLVED: gdb.base/bp-cmds-execution-x-script.exp: run to end
> UNRESOLVED: gdb.base/bp-cmds-run-with-ex.exp: execute bp commands
> UNRESOLVED: gdb.base/bp-cmds-sourced-script.exp: source the script
> ...
> UNRESOLVED: gdb.trace/save-trace.exp: absolute: read back saved tracepoints
> UNRESOLVED: gdb.trace/save-trace.exp: absolute: verify recovered tracepoints
> DUPLICATE: gdb.trace/save-trace.exp: absolute: verify recovered tracepoints
> UNRESOLVED: gdb.trace/save-trace.exp: absolute: verify default-collect
> UNRESOLVED: gdb.trace/save-trace.exp: verify help save tracepoints
> 
> 

I just rebased the below fix atop of Simon's buffer patch
(f8631e5e "gdb: remove static buffer in command_line_input")
and on my machine with ASan-enabled gdb:

  * all of gdb.base pass except 8 tests are shaky (they failed
    even before any of these changes)
  * gdb.python/py-cmd.exp passes

But given the history, I'm far from being sure...

Jan

-- >8 --
 Subject: [PATCH] gdb: fix command lookup in execute_command ()
 
 Commit b5661ff2 ("gdb: fix possible use-after-free when
 executing commands") used lookup_cmd_exact () to lookup
 command again after its execution to avoid possible
 use-after-free error.
 
 However this change broke test gdb.base/define.exp which
 defines a post-hook for subcommand ("target testsuite").
 In this case,  lookup_cmd_exact () returned NULL because
 there's no command 'testsuite' in top-level commands.
 
 This commit fixes this case by looking up the command again
 using the original command line via lookup_cmd ().
 ---
  gdb/top.c | 8 ++------
  1 file changed, 2 insertions(+), 6 deletions(-)
 
 diff --git a/gdb/top.c b/gdb/top.c
 index 742997808bd..caa08d98cec 100644
 --- a/gdb/top.c
 +++ b/gdb/top.c
 @@ -655,11 +655,6 @@ execute_command (const char *p, int from_tty)
  	    }
  	}
  
 -      /* Remember name of the command.  This is needed later when
 -	 executing command post-hooks to handle the case when command
 -	 is redefined or removed during it's execution.  See below.  */
 -      std::string c_name (c->name);
 -
        /* If this command has been pre-hooked, run the hook first.  */
        execute_cmd_pre_hook (c);
  
 @@ -702,7 +697,8 @@ 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.  */
 -      c = lookup_cmd_exact (c_name.c_str (), cmdlist);
 +      const char *cmd2 = cmd_start;
 +      c = lookup_cmd (&cmd2, cmdlist, "", nullptr, 1, 1);
        if (c != nullptr)
  	execute_cmd_post_hook (c);
  

Re: [PATCH] gdb: fix command lookup in execute_command ()

[Simon Marchi]

On 12/16/22 09:07, Jan Vraný wrote:
> On Tue, 2022-12-13 at 14:29 -0500, Simon Marchi wrote:
>> On 12/13/22 13:48, Jan Vraný wrote:
>>> On Tue, 2022-12-13 at 08:05 -0700, Tom Tromey wrote:
>>>>>>>>> "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
>>>>
>>>> Jan> The below patch should fix this case - with it on my system
>>>> Jan> gdb.base/define.exp passes.
>>>>
>>>> Jan> Alternatively I may just revert the commit until better solution
>>>> Jan> is found.
>>>>
>>>> This patch looks ok to me, but could you say whether you ran all the
>>>> tests or just define.exp?  Given the history I think a full regression
>>>> test is warranted.
>>>
>>> I did use try-build which succeeded (build #28) but it seems to me it
>>> runs only very limited number of tests. 
>>>
>>> I did run all of gdb.base and did not spot any new regression compared
>>> to master with my (previous, broken) patch reverted. I did not run more
>>> than gdb.base mainly because just gdb.base takes about an hour on my machine.
>>> Also I get weird intermittent failures when running testsuite. 
>>>
>>> Jan
>>
>> Here are the unexpected failures I saw:
>>
>> UNRESOLVED: gdb.base/bp-cmds-execution-x-script.exp: run to end
>> UNRESOLVED: gdb.base/bp-cmds-run-with-ex.exp: execute bp commands
>> UNRESOLVED: gdb.base/bp-cmds-sourced-script.exp: source the script
>> ...
>> UNRESOLVED: gdb.trace/save-trace.exp: absolute: read back saved tracepoints
>> UNRESOLVED: gdb.trace/save-trace.exp: absolute: verify recovered tracepoints
>> DUPLICATE: gdb.trace/save-trace.exp: absolute: verify recovered tracepoints
>> UNRESOLVED: gdb.trace/save-trace.exp: absolute: verify default-collect
>> UNRESOLVED: gdb.trace/save-trace.exp: verify help save tracepoints
>>
>>
> 
> I just rebased the below fix atop of Simon's buffer patch
> (f8631e5e "gdb: remove static buffer in command_line_input")
> and on my machine with ASan-enabled gdb:
> 
>   * all of gdb.base pass except 8 tests are shaky (they failed
>     even before any of these changes)
>   * gdb.python/py-cmd.exp passes
> 
> But given the history, I'm far from being sure...
> 
> Jan
> 
> -- >8 --
>  Subject: [PATCH] gdb: fix command lookup in execute_command ()
>  
>  Commit b5661ff2 ("gdb: fix possible use-after-free when
>  executing commands") used lookup_cmd_exact () to lookup
>  command again after its execution to avoid possible
>  use-after-free error.
>  
>  However this change broke test gdb.base/define.exp which
>  defines a post-hook for subcommand ("target testsuite").
>  In this case,  lookup_cmd_exact () returned NULL because
>  there's no command 'testsuite' in top-level commands.
>  
>  This commit fixes this case by looking up the command again
>  using the original command line via lookup_cmd ().

I ran the patch through my CI job, looks good.  You can add my

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH] gdb: fix command lookup in execute_command ()

[Jan Vraný]

On Fri, 2022-12-16 at 11:47 -0500, Simon Marchi wrote:
> 
> On 12/16/22 09:07, Jan Vraný wrote:
> > On Tue, 2022-12-13 at 14:29 -0500, Simon Marchi wrote:
> > > On 12/13/22 13:48, Jan Vraný wrote:
> > > > On Tue, 2022-12-13 at 08:05 -0700, Tom Tromey wrote:
> > > > > > > > > > "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> > > > > 
> > > > > Jan> The below patch should fix this case - with it on my system
> > > > > Jan> gdb.base/define.exp passes.
> > > > > 
> > > > > Jan> Alternatively I may just revert the commit until better solution
> > > > > Jan> is found.
> > > > > 
> > > > > This patch looks ok to me, but could you say whether you ran all the
> > > > > tests or just define.exp?  Given the history I think a full regression
> > > > > test is warranted.
> > > > 
> > > > I did use try-build which succeeded (build #28) but it seems to me it
> > > > runs only very limited number of tests. 
> > > > 
> > > > I did run all of gdb.base and did not spot any new regression compared
> > > > to master with my (previous, broken) patch reverted. I did not run more
> > > > than gdb.base mainly because just gdb.base takes about an hour on my machine.
> > > > Also I get weird intermittent failures when running testsuite. 
> > > > 
> > > > Jan
> > > 
> > > Here are the unexpected failures I saw:
> > > 
> > > UNRESOLVED: gdb.base/bp-cmds-execution-x-script.exp: run to end
> > > UNRESOLVED: gdb.base/bp-cmds-run-with-ex.exp: execute bp commands
> > > UNRESOLVED: gdb.base/bp-cmds-sourced-script.exp: source the script
> > > ...
> > > UNRESOLVED: gdb.trace/save-trace.exp: absolute: read back saved tracepoints
> > > UNRESOLVED: gdb.trace/save-trace.exp: absolute: verify recovered tracepoints
> > > DUPLICATE: gdb.trace/save-trace.exp: absolute: verify recovered tracepoints
> > > UNRESOLVED: gdb.trace/save-trace.exp: absolute: verify default-collect
> > > UNRESOLVED: gdb.trace/save-trace.exp: verify help save tracepoints
> > > 
> > > 
> > 
> > I just rebased the below fix atop of Simon's buffer patch
> > (f8631e5e "gdb: remove static buffer in command_line_input")
> > and on my machine with ASan-enabled gdb:
> > 
> >   * all of gdb.base pass except 8 tests are shaky (they failed
> >     even before any of these changes)
> >   * gdb.python/py-cmd.exp passes
> > 
> > But given the history, I'm far from being sure...
> > 
> > Jan
> > 
> > -- >8 --
> >  Subject: [PATCH] gdb: fix command lookup in execute_command ()
> >  
> >  Commit b5661ff2 ("gdb: fix possible use-after-free when
> >  executing commands") used lookup_cmd_exact () to lookup
> >  command again after its execution to avoid possible
> >  use-after-free error.
> >  
> >  However this change broke test gdb.base/define.exp which
> >  defines a post-hook for subcommand ("target testsuite").
> >  In this case,  lookup_cmd_exact () returned NULL because
> >  there's no command 'testsuite' in top-level commands.
> >  
> >  This commit fixes this case by looking up the command again
> >  using the original command line via lookup_cmd ().
> 
> I ran the patch through my CI job, looks good.  You can add my
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> 

Pushed. Thanks Simon for all your help! 

Jan

> Simon
> 

Re: [PATCH] gdb: fix command lookup in execute_command ()

[Tom Tromey]

>>>>> "Jan" == Jan Vraný <Jan.Vrany@labware.com> writes:

Jan> Pushed. Thanks Simon for all your help! 

Do we need this on gdb-13-branch?

Tom

Re: [PATCH] gdb: fix command lookup in execute_command ()

[Jan Vraný]

On Mon, 2022-12-19 at 07:46 -0700, Tom Tromey wrote:
> > > > > > "Jan" == Jan Vraný <Jan.Vrany@labware.com> writes:
> 
> Jan> Pushed. Thanks Simon for all your help! 
> 
> Do we need this on gdb-13-branch?
> 
> Tom
> 
Good point. I think we do, because gdb-13-branch contains
commit b5661ff ("gdb: fix possible use-after-free when executing commands")
that broke post-hooks for subcommands (most notably for `target remote ...`)

Jan

Re: [PATCH] gdb: fix command lookup in execute_command ()

[Tom Tromey]

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

>> Do we need this on gdb-13-branch?

Jan> Good point. I think we do, because gdb-13-branch contains
Jan> commit b5661ff ("gdb: fix possible use-after-free when executing commands")
Jan> that broke post-hooks for subcommands (most notably for `target remote ...`)

I cherry-picked it and rebuilt, and I'm going to push it now.

Tom

Re: [PATCH] gdb: fix possible use-after-free when executing commands

[Simon Marchi]

On 12/8/22 09:20, Jan Vrany via Gdb-patches wrote:
> In principle, `execute_command()` does following:
> 
>    struct cmd_list_element *c;
>    c = lookup_cmd ( ... );
>    ...
>    /* If this command has been pre-hooked, run the hook first.  */
>    execute_cmd_pre_hook (c);
>    ...
>    /* ...execute the command `c` ...*/
>    ...
>    execute_cmd_post_hook (c);
> 
> This may lead into use-after-free error.  Imagine the command
> being executed is a user-defined Python command that redefines
> itself.  In that case, struct `cmd_list_element` pointed to by
> `c` is deallocated during its execution so it is no longer valid
> when post hook is executed.
> 
> To fix this case, this commit looks up the command once again
> after it is executed to get pointer to (possibly newly allocated)
> `cmd_list_element`.

Hi Jan,

Do you think you could write a test to exercise that fix?

Simon

Re: [PATCH] gdb: fix possible use-after-free when executing commands

[Jan Vraný]

On Wed, 2022-12-14 at 14:52 -0500, Simon Marchi wrote:
> 
> On 12/8/22 09:20, Jan Vrany via Gdb-patches wrote:
> > In principle, `execute_command()` does following:
> > 
> >    struct cmd_list_element *c;
> >    c = lookup_cmd ( ... );
> >    ...
> >    /* If this command has been pre-hooked, run the hook first.  */
> >    execute_cmd_pre_hook (c);
> >    ...
> >    /* ...execute the command `c` ...*/
> >    ...
> >    execute_cmd_post_hook (c);
> > 
> > This may lead into use-after-free error.  Imagine the command
> > being executed is a user-defined Python command that redefines
> > itself.  In that case, struct `cmd_list_element` pointed to by
> > `c` is deallocated during its execution so it is no longer valid
> > when post hook is executed.
> > 
> > To fix this case, this commit looks up the command once again
> > after it is executed to get pointer to (possibly newly allocated)
> > `cmd_list_element`.
> 
> Hi Jan,
> 
> Do you think you could write a test to exercise that fix?

Maybe, though I'm not quite sure how to make it fail unless
one uses ASAN or Valgrind to run it like you do. Will give it 
stab. 

Jan

> 
> Simon
> 

Re: [PATCH] gdb: fix possible use-after-free when executing commands

[Simon Marchi]

On 12/14/22 15:39, Jan Vraný wrote:
> On Wed, 2022-12-14 at 14:52 -0500, Simon Marchi wrote:
>>
>> On 12/8/22 09:20, Jan Vrany via Gdb-patches wrote:
>>> In principle, `execute_command()` does following:
>>>
>>>    struct cmd_list_element *c;
>>>    c = lookup_cmd ( ... );
>>>    ...
>>>    /* If this command has been pre-hooked, run the hook first.  */
>>>    execute_cmd_pre_hook (c);
>>>    ...
>>>    /* ...execute the command `c` ...*/
>>>    ...
>>>    execute_cmd_post_hook (c);
>>>
>>> This may lead into use-after-free error.  Imagine the command
>>> being executed is a user-defined Python command that redefines
>>> itself.  In that case, struct `cmd_list_element` pointed to by
>>> `c` is deallocated during its execution so it is no longer valid
>>> when post hook is executed.
>>>
>>> To fix this case, this commit looks up the command once again
>>> after it is executed to get pointer to (possibly newly allocated)
>>> `cmd_list_element`.
>>
>> Hi Jan,
>>
>> Do you think you could write a test to exercise that fix?
> 
> Maybe, though I'm not quite sure how to make it fail unless
> one uses ASAN or Valgrind to run it like you do. Will give it 
> stab. 
> 
> Jan

It's fine if it only fails with ASan / Valgrind enabled, that's the
point of these tools.  They help catch bugs that would otherwise fly
under the radar.

Simon

Re: [PATCH] gdb: fix possible use-after-free when executing commands

[Jan Vrany]

Hi Simon,

> Hi Jan,
> > > 
> > > Do you think you could write a test to exercise that fix?
> > 
> > Maybe, though I'm not quite sure how to make it fail unless
> > one uses ASAN or Valgrind to run it like you do. Will give it 
> > stab. 
> > 
> > Jan
> 
> It's fine if it only fails with ASan / Valgrind enabled, that's the
> point of these tools.  They help catch bugs that would otherwise fly
> under the radar.
> 

Maybe something like the patch below?

With:

 * patch b5661ff2 ("gdb: fix possible use-after-free when executing commands")
   reverted,
 * patch below applied
 * and GDB compiled with ASan,

the new test fails for me. If I comment the redefinition:

diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
index ce26f2d3040..ed628e77d31 100644
--- a/gdb/testsuite/gdb.python/py-cmd.exp
+++ b/gdb/testsuite/gdb.python/py-cmd.exp
@@ -82,7 +82,7 @@ gdb_test_multiline "input command redefining itself" \
   "  def invoke (self, arg, from_tty):" "" \
   "    print (\"redefine_cmd output, msg = %s\" % self._msg)" "" \
   "    self._msg = arg" "" \
-  "    redefine_cmd (arg)" "" \
+  "    #redefine_cmd (arg)" "" \
   "redefine_cmd (\"XXX\")" "" \
   "end" ""

the test start to pass (since it is not redefining itself).

HTH, Jan

-- >8 --
Subject: [PATCH] gdb/testsuite: add test for Python commands redefining itself

This commit add test that creates a Python command that redefines
itself during its execution. This is to test use-after-free in
execute_command ().

This test needs run with ASan enabled in order to fail when it
should.
---
 gdb/testsuite/gdb.python/py-cmd.exp | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
index aa95a459f46..ce26f2d3040 100644
--- a/gdb/testsuite/gdb.python/py-cmd.exp
+++ b/gdb/testsuite/gdb.python/py-cmd.exp
@@ -71,6 +71,29 @@ gdb_test_multiline "input subcommand" \
 
 gdb_test "prefix_cmd subcmd ugh" "subcmd output, arg = ugh" "call subcmd"
 
+# Test command redefining itself
+
+gdb_test_multiline "input command redefining itself" \
+  "python" "" \
+  "class redefine_cmd (gdb.Command):" "" \
+  "  def __init__ (self, msg):" "" \
+  "    super (redefine_cmd, self).__init__ (\"redefine_cmd\", gdb.COMMAND_OBSCURE)" "" \
+  "    self._msg = msg" "" \
+  "  def invoke (self, arg, from_tty):" "" \
+  "    print (\"redefine_cmd output, msg = %s\" % self._msg)" "" \
+  "    self._msg = arg" "" \
+  "    redefine_cmd (arg)" "" \
+  "redefine_cmd (\"XXX\")" "" \
+  "end" ""
+
+gdb_test "redefine_cmd AAA" \
+  "redefine_cmd output, msg = XXX" \
+  "call command redefining itself 1"
+
+gdb_test "redefine_cmd BBB" \
+  "redefine_cmd output, msg = AAA" \
+  "call command redefining itself 2"
+
 # Test prefix command using keyword arguments.
 
 gdb_test_multiline "input prefix command, keyword arguments" \
-- 
2.35.1

Re: [PATCH] gdb: fix possible use-after-free when executing commands

[Simon Marchi]

On 12/15/22 07:57, Jan Vrany via Gdb-patches wrote:
> Hi Simon,
> 
>> Hi Jan,
>>>>
>>>> Do you think you could write a test to exercise that fix?
>>>
>>> Maybe, though I'm not quite sure how to make it fail unless
>>> one uses ASAN or Valgrind to run it like you do. Will give it 
>>> stab. 
>>>
>>> Jan
>>
>> It's fine if it only fails with ASan / Valgrind enabled, that's the
>> point of these tools.  They help catch bugs that would otherwise fly
>> under the radar.
>>
> 
> Maybe something like the patch below?

Thanks for following up!

> 
> With:
> 
>  * patch b5661ff2 ("gdb: fix possible use-after-free when executing commands")
>    reverted,
>  * patch below applied
>  * and GDB compiled with ASan,
> 
> the new test fails for me. If I comment the redefinition:
> 
> diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
> index ce26f2d3040..ed628e77d31 100644
> --- a/gdb/testsuite/gdb.python/py-cmd.exp
> +++ b/gdb/testsuite/gdb.python/py-cmd.exp
> @@ -82,7 +82,7 @@ gdb_test_multiline "input command redefining itself" \
>    "  def invoke (self, arg, from_tty):" "" \
>    "    print (\"redefine_cmd output, msg = %s\" % self._msg)" "" \
>    "    self._msg = arg" "" \
> -  "    redefine_cmd (arg)" "" \
> +  "    #redefine_cmd (arg)" "" \
>    "redefine_cmd (\"XXX\")" "" \
>    "end" ""
> 
> the test start to pass (since it is not redefining itself).
> 
> HTH, Jan
> 
> -- >8 --
> Subject: [PATCH] gdb/testsuite: add test for Python commands redefining itself
> 
> This commit add test that creates a Python command that redefines

"add" -> "adds a"

> itself during its execution. This is to test use-after-free in
> execute_command ().
> 
> This test needs run with ASan enabled in order to fail when it
> should.
> ---
>  gdb/testsuite/gdb.python/py-cmd.exp | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
> index aa95a459f46..ce26f2d3040 100644
> --- a/gdb/testsuite/gdb.python/py-cmd.exp
> +++ b/gdb/testsuite/gdb.python/py-cmd.exp
> @@ -71,6 +71,29 @@ gdb_test_multiline "input subcommand" \
>  
>  gdb_test "prefix_cmd subcmd ugh" "subcmd output, arg = ugh" "call subcmd"
>  
> +# Test command redefining itself
> +
> +gdb_test_multiline "input command redefining itself" \
> +  "python" "" \
> +  "class redefine_cmd (gdb.Command):" "" \
> +  "  def __init__ (self, msg):" "" \
> +  "    super (redefine_cmd, self).__init__ (\"redefine_cmd\", gdb.COMMAND_OBSCURE)" "" \
> +  "    self._msg = msg" "" \
> +  "  def invoke (self, arg, from_tty):" "" \
> +  "    print (\"redefine_cmd output, msg = %s\" % self._msg)" "" \
> +  "    self._msg = arg" "" \

Is it needed to assign arg to self._msg here?

> +  "    redefine_cmd (arg)" "" \
> +  "redefine_cmd (\"XXX\")" "" \
> +  "end" ""
> +
> +gdb_test "redefine_cmd AAA" \
> +  "redefine_cmd output, msg = XXX" \
> +  "call command redefining itself 1"
> +
> +gdb_test "redefine_cmd BBB" \
> +  "redefine_cmd output, msg = AAA" \
> +  "call command redefining itself 2"
> +

Note that in TCL code, we use an indent of 4 columns (and just like with
C++ code, whole groups of 8 columns become a tab).

In order to isolate the new test from the other tests in the file, can
you put the new test into its own `proc_with_prefix` function, and start
with a fresh GDB?  That would mean calling clean_restart at the
beginning of the proc.

Simon

Re: [PATCH] gdb: fix possible use-after-free when executing commands

[Jan Vrany]

> > +  "  def invoke (self, arg, from_tty):" "" \
> > +  "    print (\"redefine_cmd output, msg = %s\" % self._msg)" "" \
> > +  "    self._msg = arg" "" \
> 
> Is it needed to assign arg to self._msg here?
> 

It is not, but found it usefull when testing the test.
This way, one may only comment the next line and test would
pass, without need to tweak following `gdb_test` lines.
Removed in new version (below)

> > +  "    redefine_cmd (arg)" "" \
> > +  "redefine_cmd (\"XXX\")" "" \
> > +  "end" ""
> > +
> > +gdb_test "redefine_cmd AAA" \
> > +  "redefine_cmd output, msg = XXX" \
> > +  "call command redefining itself 1"
> > +
> > +gdb_test "redefine_cmd BBB" \
> > +  "redefine_cmd output, msg = AAA" \
> > +  "call command redefining itself 2"
> > +
> 
> Note that in TCL code, we use an indent of 4 columns (and just like with
> C++ code, whole groups of 8 columns become a tab).
> 
> In order to isolate the new test from the other tests in the file, can
> you put the new test into its own `proc_with_prefix` function, and start
> with a fresh GDB?  That would mean calling clean_restart at the
> beginning of the proc.

Done, hopefully this is what you meant. Also I put the test to the end
of the file, as it is now in its own function.

-- >8 --
Subject: [PATCH v2] gdb/testsuite: add test for Python commands redefining
 itself

This commit adds a test that creates a Python command that redefines
itself during its execution. This is to test use-after-free in
execute_command ().

This test needs run with ASan enabled in order to fail when it
should.
---
 gdb/testsuite/gdb.python/py-cmd.exp | 30 +++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
index aa95a459f46..48c3e18f1cc 100644
--- a/gdb/testsuite/gdb.python/py-cmd.exp
+++ b/gdb/testsuite/gdb.python/py-cmd.exp
@@ -300,3 +300,33 @@ gdb_test_multiple "test_multiline" $test {
 	pass $test
     }
 }
+
+# Test command redefining itself
+
+proc_with_prefix test_command_redefining_itself {} {
+    # Start with a fresh gdb
+    clean_restart
+
+
+    gdb_test_multiline "input command redefining itself" \
+	"python" "" \
+	"class redefine_cmd (gdb.Command):" "" \
+	"  def __init__ (self, msg):" "" \
+	"    super (redefine_cmd, self).__init__ (\"redefine_cmd\", gdb.COMMAND_OBSCURE)" "" \
+	"    self._msg = msg" "" \
+	"  def invoke (self, arg, from_tty):" "" \
+	"    print (\"redefine_cmd output, msg = %s\" % self._msg)" "" \
+	"    redefine_cmd (arg)" "" \
+	"redefine_cmd (\"XXX\")" "" \
+	"end" ""
+
+    gdb_test "redefine_cmd AAA" \
+	"redefine_cmd output, msg = XXX" \
+	"call command redefining itself 1"
+
+    gdb_test "redefine_cmd BBB" \
+	"redefine_cmd output, msg = AAA" \
+	"call command redefining itself 2"
+}
+
+test_command_redefining_itself
-- 
2.35.1

Re: [PATCH] gdb: fix possible use-after-free when executing commands

[Simon Marchi]

On 12/15/22 09:51, Jan Vrany wrote:
>>> +  "  def invoke (self, arg, from_tty):" "" \
>>> +  "    print (\"redefine_cmd output, msg = %s\" % self._msg)" "" \
>>> +  "    self._msg = arg" "" \
>>
>> Is it needed to assign arg to self._msg here?
>>
> 
> It is not, but found it usefull when testing the test.
> This way, one may only comment the next line and test would
> pass, without need to tweak following `gdb_test` lines.
> Removed in new version (below)
> 
>>> +  "    redefine_cmd (arg)" "" \
>>> +  "redefine_cmd (\"XXX\")" "" \
>>> +  "end" ""
>>> +
>>> +gdb_test "redefine_cmd AAA" \
>>> +  "redefine_cmd output, msg = XXX" \
>>> +  "call command redefining itself 1"
>>> +
>>> +gdb_test "redefine_cmd BBB" \
>>> +  "redefine_cmd output, msg = AAA" \
>>> +  "call command redefining itself 2"
>>> +
>>
>> Note that in TCL code, we use an indent of 4 columns (and just like with
>> C++ code, whole groups of 8 columns become a tab).
>>
>> In order to isolate the new test from the other tests in the file, can
>> you put the new test into its own `proc_with_prefix` function, and start
>> with a fresh GDB?  That would mean calling clean_restart at the
>> beginning of the proc.
> 
> Done, hopefully this is what you meant. Also I put the test to the end
> of the file, as it is now in its own function.
> 
> -- >8 --
> Subject: [PATCH v2] gdb/testsuite: add test for Python commands redefining
>  itself
> 
> This commit adds a test that creates a Python command that redefines
> itself during its execution. This is to test use-after-free in
> execute_command ().
> 
> This test needs run with ASan enabled in order to fail when it
> should.
> ---
>  gdb/testsuite/gdb.python/py-cmd.exp | 30 +++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
> index aa95a459f46..48c3e18f1cc 100644
> --- a/gdb/testsuite/gdb.python/py-cmd.exp
> +++ b/gdb/testsuite/gdb.python/py-cmd.exp
> @@ -300,3 +300,33 @@ gdb_test_multiple "test_multiline" $test {
>  	pass $test
>      }
>  }
> +
> +# Test command redefining itself
> +
> +proc_with_prefix test_command_redefining_itself {} {
> +    # Start with a fresh gdb
> +    clean_restart
> +
> +
> +    gdb_test_multiline "input command redefining itself" \
> +	"python" "" \
> +	"class redefine_cmd (gdb.Command):" "" \
> +	"  def __init__ (self, msg):" "" \
> +	"    super (redefine_cmd, self).__init__ (\"redefine_cmd\", gdb.COMMAND_OBSCURE)" "" \
> +	"    self._msg = msg" "" \
> +	"  def invoke (self, arg, from_tty):" "" \
> +	"    print (\"redefine_cmd output, msg = %s\" % self._msg)" "" \
> +	"    redefine_cmd (arg)" "" \
> +	"redefine_cmd (\"XXX\")" "" \
> +	"end" ""
> +
> +    gdb_test "redefine_cmd AAA" \
> +	"redefine_cmd output, msg = XXX" \
> +	"call command redefining itself 1"
> +
> +    gdb_test "redefine_cmd BBB" \
> +	"redefine_cmd output, msg = AAA" \
> +	"call command redefining itself 2"
> +}
> +
> +test_command_redefining_itself

Thanks, this LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

This way, you know that this part of the test doesn't rely on anything
that comes before.  The proc prefix part makes it easy to jump directly
at the right place, if you investigate a failure.  And if everything is
in its own little independent proc like that, it makes it easy to
comment out all the other tests if you are investigating a failure in a
specific one.

Simon

> -- 
> 2.35.1
>