gdb: fix possible use-after-free when executing commands

Message ID 20221208142014.84759-1-jan.vrany@labware.com
State New
Headers
Series gdb: fix possible use-after-free when executing commands |

Commit Message

Jan Vrany Dec. 8, 2022, 2:20 p.m. UTC
  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(-)
  

Comments

Tom Tromey Dec. 9, 2022, 5:55 p.m. UTC | #1
>>>>> "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
  
Luis Machado Dec. 12, 2022, 3:05 p.m. UTC | #2
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
  
Jan Vrany Dec. 12, 2022, 3:08 p.m. UTC | #3
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

>
  
Luis Machado Dec. 12, 2022, 3:09 p.m. UTC | #4
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.
  
Simon Marchi Dec. 14, 2022, 7:52 p.m. UTC | #5
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
  
Jan Vrany Dec. 14, 2022, 8:39 p.m. UTC | #6
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
>
  
Simon Marchi Dec. 14, 2022, 8:42 p.m. UTC | #7
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
  
Jan Vrany Dec. 15, 2022, 12:57 p.m. UTC | #8
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" \
  
Simon Marchi Dec. 15, 2022, 1:53 p.m. UTC | #9
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
  
Jan Vrany Dec. 15, 2022, 2:51 p.m. UTC | #10
> > +  "  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
  
Simon Marchi Dec. 15, 2022, 4 p.m. UTC | #11
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
>
  

Patch

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