diff mbox series

[RFA] Fix use after free caused by redefining a python prefix cmd that has aliases.

Message ID 20221226170823.2744157-1-philippe.waroquiers@skynet.be
State New
Headers
Series [RFA] Fix use after free caused by redefining a python prefix cmd that has aliases. |

Commit Message

Philippe Waroquiers Dec. 26, 2022, 5:08 p.m. UTC 
  GDB gets a segfault when redefining a python prefix command that has aliases.
This is reproduced by running the modified py-cmd.exp (see SEGV stacktrace
below and some valgring reported errors).

Fix the problem by ensuring the aliases of the redefined python command get a
copy of all the new member values of the redefined command.

Tested on amd64/debian. Also run all tests under valgrind.

Fatal signal: Segmentation fault
  ----- Backtrace -----
  0x564d0f2cff7e gdb_internal_backtrace_1
          ../../termours/gdb/bt-utils.c:122
  0x564d0f2d0023 _Z22gdb_internal_backtracev
          ../../termours/gdb/bt-utils.c:168
  0x564d0f4bc969 handle_fatal_signal
          ../../termours/gdb/event-top.c:956
  0x564d0f4bcae4 handle_sigsegv
          ../../termours/gdb/event-top.c:1029
  0x7fb82fe0613f ???
          ./nptl/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0
  0x564d0f3290a5 lookup_cmd_with_subcommands
          ../../termours/gdb/cli/cli-decode.c:80
  0x564d0f3290b6 lookup_cmd_with_subcommands
          ../../termours/gdb/cli/cli-decode.c:80
  0x564d0f329647 do_add_cmd
          ../../termours/gdb/cli/cli-decode.c:225
  0x564d0f32968f _Z7add_cmdPKc13command_classS0_PP16cmd_list_element
          ../../termours/gdb/cli/cli-decode.c:236
  0x564d0f6965e4 cmdpy_init
          ../../termours/gdb/python/py-cmd.c:520
  0x564d0fb4a87b wrap_init

Valgrind reports:
  ==1309561== Invalid read of size 8
  ==1309561==    at 0x48F12E: lookup_cmd_with_subcommands (cli-decode.c:80)
  ==1309561==    by 0x48F12E: do_add_cmd(char const*, command_class, char const*, cmd_list_element**) (cli-decode.c:225)
  ==1309561==    by 0x4906D9: add_cmd (cli-decode.c:247)
  ==1309561==    by 0x4906D9: add_prefix_cmd(char const*, command_class, void (*)(char const*, int), char const*, cmd_list_element**, int, cmd_list_element**) (cli-decode.c:362)
  ==1309561==    by 0x6C04DD: cmdpy_init(_object*, _object*, _object*) (py-cmd.c:514)
  ==1309561==    by 0xA14D2B: wrap_init (typeobject.c:5993)
  ==1309561==    by 0x9C663F: _PyObject_MakeTpCall (call.c:159)
  ==1309561==    by 0x3B4052: _PyObject_Vectorcall (abstract.h:125)
  ==1309561==    by 0x3B4052: _PyObject_Vectorcall (abstract.h:115)
  ==1309561==    by 0x3B4052: call_function (ceval.c:4987)
  ...
  ==1309561==  Address 0x7808f38 is 40 bytes inside a block of size 64 free'd
  ==1309561==    at 0x483BF4C: free (vg_replace_malloc.c:884)
  ==1309561==    by 0xA155AA: subtype_dealloc (typeobject.c:1287)
  ==1309561==    by 0x6BF4E7: _Py_DECREF (object.h:478)
  ==1309561==    by 0x6BF4E7: decref (py-ref.h:36)
  ==1309561==    by 0x6BF4E7: ~ref_ptr (gdb_ref_ptr.h:91)
  ==1309561==    by 0x6BF4E7: cmdpy_destroyer(cmd_list_element*, void*) (py-cmd.c:96)
  ==1309561==    by 0x48EEFF: delete_cmd (cli-decode.c:1253)
  ==1309561==    by 0x48EEFF: do_add_cmd(char const*, command_class, char const*, cmd_list_element**) (cli-decode.c:190)
  ==1309561==    by 0x4906D9: add_cmd (cli-decode.c:247)
  ==1309561==    by 0x4906D9: add_prefix_cmd(char const*, command_class, void (*)(char const*, int), char const*, cmd_list_element**, int, cmd_list_element**) (cli-decode.c:362)
  ==1309561==    by 0x6C04DD: cmdpy_init(_object*, _object*, _object*) (py-cmd.c:514)
  ==1309561==    by 0xA14D2B: wrap_init (typeobject.c:5993)
  ==1309561==    by 0x9C663F: _PyObject_MakeTpCall (call.c:159)
  ==1309561==    by 0x3B4052: _PyObject_Vectorcall (abstract.h:125)
  ...
  ==1309561==  Block was alloc'd at
  ==1309561==    at 0x483979B: malloc (vg_replace_malloc.c:393)
  ==1309561==    by 0xAD2C4D: _PyObject_GC_Alloc (gcmodule.c:1964)
  ==1309561==    by 0xAD2C4D: _PyObject_GC_Malloc (gcmodule.c:1987)
  ==1309561==    by 0xA16DE9: PyType_GenericAlloc (typeobject.c:1010)
  ==1309561==    by 0xA18F44: type_call (typeobject.c:969)
  ==1309561==    by 0x9C663F: _PyObject_MakeTpCall (call.c:159)
  ==1309561==    by 0x3B069F: _PyObject_Vectorcall (abstract.h:125)
  ==1309561==    by 0x3B069F: _PyObject_Vectorcall (abstract.h:115)
  ==1309561==    by 0x3B069F: call_function (ceval.c:4987)
  ==1309561==    by 0x3B069F: _PyEval_EvalFrameDefault (ceval.c:3500)
  ==1309561==    by 0xA75253: PyEval_EvalFrameEx (ceval.c:741)
  ==1309561==    by 0xA75253: _PyEval_EvalCodeWithName (ceval.c:4298)
  ==1309561==    by 0xA755B2: PyEval_EvalCodeEx (ceval.c:4327)
  ==1309561==    by 0xA755B2: PyEval_EvalCode (ceval.c:718)
  ==1309561==    by 0xAB1402: run_eval_code_obj (pythonrun.c:1117)
  ==1309561==    by 0xAB1402: run_mod (pythonrun.c:1139)
  ==1309561==    by 0xAB27F0: PyRun_StringFlags (pythonrun.c:1026)
  ==1309561==    by 0xAB27F0: PyRun_SimpleStringFlags (pythonrun.c:460)
  ==1309561==    by 0x6F689B: gdbpy_eval_from_control_command(extension_language_defn const*, command_line*) (python.c:434)
  ==1309561==    by 0x49F54C: execute_control_command_1(command_line*, int) (cli-script.c:683)
---
 gdb/cli/cli-decode.c                     | 19 ++++++++++------
 gdb/python/py-cmd.c                      |  9 +++++++-
 gdb/testsuite/gdb.python/py-cmd-alias.py | 28 ++++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-cmd.exp      | 17 ++++++++++++++
 4 files changed, 65 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.python/py-cmd-alias.py

Comments

Philippe Waroquiers Jan. 9, 2023, 11:37 p.m. UTC | #1 
Ping ?
Thanks
Philippe

On Mon, 2022-12-26 at 18:08 +0100, Philippe Waroquiers wrote:
> GDB gets a segfault when redefining a python prefix command that has aliases.
> This is reproduced by running the modified py-cmd.exp (see SEGV stacktrace
> below and some valgring reported errors).
> 
> Fix the problem by ensuring the aliases of the redefined python command get a
> copy of all the new member values of the redefined command.
> 
> Tested on amd64/debian. Also run all tests under valgrind.
> 
> Fatal signal: Segmentation fault
>   ----- Backtrace -----
>   0x564d0f2cff7e gdb_internal_backtrace_1
>           ../../termours/gdb/bt-utils.c:122
>   0x564d0f2d0023 _Z22gdb_internal_backtracev
>           ../../termours/gdb/bt-utils.c:168
>   0x564d0f4bc969 handle_fatal_signal
>           ../../termours/gdb/event-top.c:956
>   0x564d0f4bcae4 handle_sigsegv
>           ../../termours/gdb/event-top.c:1029
>   0x7fb82fe0613f ???
>           ./nptl/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0
>   0x564d0f3290a5 lookup_cmd_with_subcommands
>           ../../termours/gdb/cli/cli-decode.c:80
>   0x564d0f3290b6 lookup_cmd_with_subcommands
>           ../../termours/gdb/cli/cli-decode.c:80
>   0x564d0f329647 do_add_cmd
>           ../../termours/gdb/cli/cli-decode.c:225
>   0x564d0f32968f _Z7add_cmdPKc13command_classS0_PP16cmd_list_element
>           ../../termours/gdb/cli/cli-decode.c:236
>   0x564d0f6965e4 cmdpy_init
>           ../../termours/gdb/python/py-cmd.c:520
>   0x564d0fb4a87b wrap_init
> 
> Valgrind reports:
>   ==1309561== Invalid read of size 8
>   ==1309561==    at 0x48F12E: lookup_cmd_with_subcommands (cli-decode.c:80)
>   ==1309561==    by 0x48F12E: do_add_cmd(char const*, command_class, char const*, cmd_list_element**) (cli-decode.c:225)
>   ==1309561==    by 0x4906D9: add_cmd (cli-decode.c:247)
>   ==1309561==    by 0x4906D9: add_prefix_cmd(char const*, command_class, void (*)(char const*, int), char const*, cmd_list_element**, int, cmd_list_element**) (cli-decode.c:362)
>   ==1309561==    by 0x6C04DD: cmdpy_init(_object*, _object*, _object*) (py-cmd.c:514)
>   ==1309561==    by 0xA14D2B: wrap_init (typeobject.c:5993)
>   ==1309561==    by 0x9C663F: _PyObject_MakeTpCall (call.c:159)
>   ==1309561==    by 0x3B4052: _PyObject_Vectorcall (abstract.h:125)
>   ==1309561==    by 0x3B4052: _PyObject_Vectorcall (abstract.h:115)
>   ==1309561==    by 0x3B4052: call_function (ceval.c:4987)
>   ...
>   ==1309561==  Address 0x7808f38 is 40 bytes inside a block of size 64 free'd
>   ==1309561==    at 0x483BF4C: free (vg_replace_malloc.c:884)
>   ==1309561==    by 0xA155AA: subtype_dealloc (typeobject.c:1287)
>   ==1309561==    by 0x6BF4E7: _Py_DECREF (object.h:478)
>   ==1309561==    by 0x6BF4E7: decref (py-ref.h:36)
>   ==1309561==    by 0x6BF4E7: ~ref_ptr (gdb_ref_ptr.h:91)
>   ==1309561==    by 0x6BF4E7: cmdpy_destroyer(cmd_list_element*, void*) (py-cmd.c:96)
>   ==1309561==    by 0x48EEFF: delete_cmd (cli-decode.c:1253)
>   ==1309561==    by 0x48EEFF: do_add_cmd(char const*, command_class, char const*, cmd_list_element**) (cli-decode.c:190)
>   ==1309561==    by 0x4906D9: add_cmd (cli-decode.c:247)
>   ==1309561==    by 0x4906D9: add_prefix_cmd(char const*, command_class, void (*)(char const*, int), char const*, cmd_list_element**, int, cmd_list_element**) (cli-decode.c:362)
>   ==1309561==    by 0x6C04DD: cmdpy_init(_object*, _object*, _object*) (py-cmd.c:514)
>   ==1309561==    by 0xA14D2B: wrap_init (typeobject.c:5993)
>   ==1309561==    by 0x9C663F: _PyObject_MakeTpCall (call.c:159)
>   ==1309561==    by 0x3B4052: _PyObject_Vectorcall (abstract.h:125)
>   ...
>   ==1309561==  Block was alloc'd at
>   ==1309561==    at 0x483979B: malloc (vg_replace_malloc.c:393)
>   ==1309561==    by 0xAD2C4D: _PyObject_GC_Alloc (gcmodule.c:1964)
>   ==1309561==    by 0xAD2C4D: _PyObject_GC_Malloc (gcmodule.c:1987)
>   ==1309561==    by 0xA16DE9: PyType_GenericAlloc (typeobject.c:1010)
>   ==1309561==    by 0xA18F44: type_call (typeobject.c:969)
>   ==1309561==    by 0x9C663F: _PyObject_MakeTpCall (call.c:159)
>   ==1309561==    by 0x3B069F: _PyObject_Vectorcall (abstract.h:125)
>   ==1309561==    by 0x3B069F: _PyObject_Vectorcall (abstract.h:115)
>   ==1309561==    by 0x3B069F: call_function (ceval.c:4987)
>   ==1309561==    by 0x3B069F: _PyEval_EvalFrameDefault (ceval.c:3500)
>   ==1309561==    by 0xA75253: PyEval_EvalFrameEx (ceval.c:741)
>   ==1309561==    by 0xA75253: _PyEval_EvalCodeWithName (ceval.c:4298)
>   ==1309561==    by 0xA755B2: PyEval_EvalCodeEx (ceval.c:4327)
>   ==1309561==    by 0xA755B2: PyEval_EvalCode (ceval.c:718)
>   ==1309561==    by 0xAB1402: run_eval_code_obj (pythonrun.c:1117)
>   ==1309561==    by 0xAB1402: run_mod (pythonrun.c:1139)
>   ==1309561==    by 0xAB27F0: PyRun_StringFlags (pythonrun.c:1026)
>   ==1309561==    by 0xAB27F0: PyRun_SimpleStringFlags (pythonrun.c:460)
>   ==1309561==    by 0x6F689B: gdbpy_eval_from_control_command(extension_language_defn const*, command_line*) (python.c:434)
>   ==1309561==    by 0x49F54C: execute_control_command_1(command_line*, int) (cli-script.c:683)
> ---
>  gdb/cli/cli-decode.c                     | 19 ++++++++++------
>  gdb/python/py-cmd.c                      |  9 +++++++-
>  gdb/testsuite/gdb.python/py-cmd-alias.py | 28 ++++++++++++++++++++++++
>  gdb/testsuite/gdb.python/py-cmd.exp      | 17 ++++++++++++++
>  4 files changed, 65 insertions(+), 8 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.python/py-cmd-alias.py
> 
> diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
> index 7c98029f9f4..132b0ca659a 100644
> --- a/gdb/cli/cli-decode.c
> +++ b/gdb/cli/cli-decode.c
> @@ -179,18 +179,26 @@ cmd_list_element::command_components () const
>  
> 
>  static struct cmd_list_element *
>  do_add_cmd (const char *name, enum command_class theclass,
> +	    cmd_simple_func_ftype *fun,
>  	    const char *doc, struct cmd_list_element **list)
>  {
>    struct cmd_list_element *c = new struct cmd_list_element (name, theclass,
>  							    doc);
>  
> 
> +  set_cmd_simple_func (c, fun);
> +
>    /* Turn each alias of the old command into an alias of the new
>       command.  */
>    c->aliases = delete_cmd (name, list, &c->hook_pre, &c->hookee_pre,
>  			   &c->hook_post, &c->hookee_post);
> -
>    for (cmd_list_element &alias : c->aliases)
> -    alias.alias_target = c;
> +    {
> +      alias.func = c->func;
> +      alias.function = c->function;
> +      alias.subcommands = c->subcommands;
> +      alias.allow_unknown = c->allow_unknown;
> +      alias.alias_target = c;
> +    }
>  
> 
>    if (c->hook_pre)
>      c->hook_pre->hookee_pre = c;
> @@ -233,9 +241,7 @@ struct cmd_list_element *
>  add_cmd (const char *name, enum command_class theclass,
>  	 const char *doc, struct cmd_list_element **list)
>  {
> -  cmd_list_element *result = do_add_cmd (name, theclass, doc, list);
> -  result->func = NULL;
> -  result->function.simple_func = NULL;
> +  cmd_list_element *result = do_add_cmd (name, theclass, NULL, doc, list);
>    return result;
>  }
>  
> 
> @@ -244,8 +250,7 @@ add_cmd (const char *name, enum command_class theclass,
>  	 cmd_simple_func_ftype *fun,
>  	 const char *doc, struct cmd_list_element **list)
>  {
> -  cmd_list_element *result = do_add_cmd (name, theclass, doc, list);
> -  set_cmd_simple_func (result, fun);
> +  cmd_list_element *result = do_add_cmd (name, theclass, fun, doc, list);
>    return result;
>  }
>  
> 
> diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
> index 5cc392af175..d66311d623b 100644
> --- a/gdb/python/py-cmd.c
> +++ b/gdb/python/py-cmd.c
> @@ -524,8 +524,15 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
>           name_allocated, so release it.  */
>        cmd_name.release ();
>  
> 
> -      /* There appears to be no API to set this.  */
> +      /* There appears to be no API to set the below members.
> +	 In particular, we cannot pass cdmpy_function to add_prefix_cmd and
> +	 add_cmd above.  This means that if cmd has aliases, its aliases have
> +	 been re-targeted to this freshly built CMD with a null func and
> +	 function.simple_func (see cli-decode.c do_add_cmd).  So, we need to set
> +	 the 'func' of the aliases of CMD.  */
>        cmd->func = cmdpy_function;
> +      for (cmd_list_element &alias : cmd->aliases)
> +	alias.func = cmd->func;
>        cmd->destroyer = cmdpy_destroyer;
>        cmd->doc_allocated = 1;
>        cmd->name_allocated = 1;
> diff --git a/gdb/testsuite/gdb.python/py-cmd-alias.py b/gdb/testsuite/gdb.python/py-cmd-alias.py
> new file mode 100644
> index 00000000000..d62628841f4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-cmd-alias.py
> @@ -0,0 +1,28 @@
> +class some_prefix_cmd (gdb.Command):
> +  def __init__ (self):
> +    super (some_prefix_cmd, self).__init__ ("some_prefix_cmd", gdb.COMMAND_OBSCURE, gdb.COMPLETE_NONE, True)
> +  def invoke (self, arg, from_tty):
> +    print ("some_prefix_cmd output, arg = %s" % arg)
> +
> +some_prefix_cmd ()
> +
> +def def_alias(alias : str, command_name : str) -> None:
> +    """Defines an alias -a ALIAS = COMMAND_NAME.
> +Traps the error if ALIAS is already defined (so as to be able to source
> +this file again)."""
> +    d = "alias " + alias + ' = ' + command_name
> +    try:
> +        gdb.execute (d)
> +    except Exception as inst:
> +        print(str(inst))
> +
> +
> +def_alias("alias_of_some_prefix_cmd", "some_prefix_cmd")
> +def_alias("other_alias_of_some_prefix_cmd", "some_prefix_cmd")
> +
> +class some_subcmd (gdb.Command):
> +  def __init__ (self):
> +    super (some_subcmd, self).__init__ ("some_prefix_cmd some_subcmd", gdb.COMMAND_OBSCURE)
> +  def invoke (self, arg, from_tty):
> +    print ("some_subcmd output, arg = %s" % arg)
> +some_subcmd ()
> diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
> index 48c3e18f1cc..c7398889fd4 100644
> --- a/gdb/testsuite/gdb.python/py-cmd.exp
> +++ b/gdb/testsuite/gdb.python/py-cmd.exp
> @@ -111,6 +111,23 @@ gdb_test_multiline "input new subcommand" \
>  
> 
>  gdb_test "info newsubcmd ugh" "newsubcmd output, arg = ugh" "call newsubcmd"
>  
> 
> +# Test sourcing twice a file that redefines a prefix command with aliases.
> +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/py-cmd-alias.py]
> +foreach initial_load {1 0} {
> +    if $initial_load {
> +	gdb_test_no_output "source ${pyfile}" "load python file"
> +    } else {
> +	gdb_test "source ${pyfile}" \
> +	    "Alias already exists: alias_of_some_prefix_cmd\r\nAlias already exists: other_alias_of_some_prefix_cmd" \
> +	    "load again python file"
> +    }
> +    foreach cmd {"some_prefix_cmd" "alias_of_some_prefix_cmd" "other_alias_of_some_prefix_cmd"} {
> +	gdb_test "$cmd ugh $initial_load" \
> +	    "some_prefix_cmd output, arg = ugh $initial_load" \
> +	    "call $cmd $initial_load"
> +    }
> +}
> +
>  # Test a command that throws gdb.GdbError.
>  
> 
>  gdb_test_multiline "input command to throw error" \
Andrew Burgess Jan. 10, 2023, 12:16 p.m. UTC | #2 
Philippe Waroquiers via Gdb-patches <gdb-patches@sourceware.org> writes:

> GDB gets a segfault when redefining a python prefix command that has aliases.
> This is reproduced by running the modified py-cmd.exp (see SEGV stacktrace
> below and some valgring reported errors).
>
> Fix the problem by ensuring the aliases of the redefined python command get a
> copy of all the new member values of the redefined command.

Our command/alias handling is a real mess :/

>
> Tested on amd64/debian. Also run all tests under valgrind.
>
> Fatal signal: Segmentation fault
>   ----- Backtrace -----
>   0x564d0f2cff7e gdb_internal_backtrace_1
>           ../../termours/gdb/bt-utils.c:122
>   0x564d0f2d0023 _Z22gdb_internal_backtracev
>           ../../termours/gdb/bt-utils.c:168
>   0x564d0f4bc969 handle_fatal_signal
>           ../../termours/gdb/event-top.c:956
>   0x564d0f4bcae4 handle_sigsegv
>           ../../termours/gdb/event-top.c:1029
>   0x7fb82fe0613f ???
>           ./nptl/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0
>   0x564d0f3290a5 lookup_cmd_with_subcommands
>           ../../termours/gdb/cli/cli-decode.c:80
>   0x564d0f3290b6 lookup_cmd_with_subcommands
>           ../../termours/gdb/cli/cli-decode.c:80
>   0x564d0f329647 do_add_cmd
>           ../../termours/gdb/cli/cli-decode.c:225
>   0x564d0f32968f _Z7add_cmdPKc13command_classS0_PP16cmd_list_element
>           ../../termours/gdb/cli/cli-decode.c:236
>   0x564d0f6965e4 cmdpy_init
>           ../../termours/gdb/python/py-cmd.c:520
>   0x564d0fb4a87b wrap_init
>
> Valgrind reports:
>   ==1309561== Invalid read of size 8
>   ==1309561==    at 0x48F12E: lookup_cmd_with_subcommands (cli-decode.c:80)
>   ==1309561==    by 0x48F12E: do_add_cmd(char const*, command_class, char const*, cmd_list_element**) (cli-decode.c:225)
>   ==1309561==    by 0x4906D9: add_cmd (cli-decode.c:247)
>   ==1309561==    by 0x4906D9: add_prefix_cmd(char const*, command_class, void (*)(char const*, int), char const*, cmd_list_element**, int, cmd_list_element**) (cli-decode.c:362)
>   ==1309561==    by 0x6C04DD: cmdpy_init(_object*, _object*, _object*) (py-cmd.c:514)
>   ==1309561==    by 0xA14D2B: wrap_init (typeobject.c:5993)
>   ==1309561==    by 0x9C663F: _PyObject_MakeTpCall (call.c:159)
>   ==1309561==    by 0x3B4052: _PyObject_Vectorcall (abstract.h:125)
>   ==1309561==    by 0x3B4052: _PyObject_Vectorcall (abstract.h:115)
>   ==1309561==    by 0x3B4052: call_function (ceval.c:4987)
>   ...
>   ==1309561==  Address 0x7808f38 is 40 bytes inside a block of size 64 free'd
>   ==1309561==    at 0x483BF4C: free (vg_replace_malloc.c:884)
>   ==1309561==    by 0xA155AA: subtype_dealloc (typeobject.c:1287)
>   ==1309561==    by 0x6BF4E7: _Py_DECREF (object.h:478)
>   ==1309561==    by 0x6BF4E7: decref (py-ref.h:36)
>   ==1309561==    by 0x6BF4E7: ~ref_ptr (gdb_ref_ptr.h:91)
>   ==1309561==    by 0x6BF4E7: cmdpy_destroyer(cmd_list_element*, void*) (py-cmd.c:96)
>   ==1309561==    by 0x48EEFF: delete_cmd (cli-decode.c:1253)
>   ==1309561==    by 0x48EEFF: do_add_cmd(char const*, command_class, char const*, cmd_list_element**) (cli-decode.c:190)
>   ==1309561==    by 0x4906D9: add_cmd (cli-decode.c:247)
>   ==1309561==    by 0x4906D9: add_prefix_cmd(char const*, command_class, void (*)(char const*, int), char const*, cmd_list_element**, int, cmd_list_element**) (cli-decode.c:362)
>   ==1309561==    by 0x6C04DD: cmdpy_init(_object*, _object*, _object*) (py-cmd.c:514)
>   ==1309561==    by 0xA14D2B: wrap_init (typeobject.c:5993)
>   ==1309561==    by 0x9C663F: _PyObject_MakeTpCall (call.c:159)
>   ==1309561==    by 0x3B4052: _PyObject_Vectorcall (abstract.h:125)
>   ...
>   ==1309561==  Block was alloc'd at
>   ==1309561==    at 0x483979B: malloc (vg_replace_malloc.c:393)
>   ==1309561==    by 0xAD2C4D: _PyObject_GC_Alloc (gcmodule.c:1964)
>   ==1309561==    by 0xAD2C4D: _PyObject_GC_Malloc (gcmodule.c:1987)
>   ==1309561==    by 0xA16DE9: PyType_GenericAlloc (typeobject.c:1010)
>   ==1309561==    by 0xA18F44: type_call (typeobject.c:969)
>   ==1309561==    by 0x9C663F: _PyObject_MakeTpCall (call.c:159)
>   ==1309561==    by 0x3B069F: _PyObject_Vectorcall (abstract.h:125)
>   ==1309561==    by 0x3B069F: _PyObject_Vectorcall (abstract.h:115)
>   ==1309561==    by 0x3B069F: call_function (ceval.c:4987)
>   ==1309561==    by 0x3B069F: _PyEval_EvalFrameDefault (ceval.c:3500)
>   ==1309561==    by 0xA75253: PyEval_EvalFrameEx (ceval.c:741)
>   ==1309561==    by 0xA75253: _PyEval_EvalCodeWithName (ceval.c:4298)
>   ==1309561==    by 0xA755B2: PyEval_EvalCodeEx (ceval.c:4327)
>   ==1309561==    by 0xA755B2: PyEval_EvalCode (ceval.c:718)
>   ==1309561==    by 0xAB1402: run_eval_code_obj (pythonrun.c:1117)
>   ==1309561==    by 0xAB1402: run_mod (pythonrun.c:1139)
>   ==1309561==    by 0xAB27F0: PyRun_StringFlags (pythonrun.c:1026)
>   ==1309561==    by 0xAB27F0: PyRun_SimpleStringFlags (pythonrun.c:460)
>   ==1309561==    by 0x6F689B: gdbpy_eval_from_control_command(extension_language_defn const*, command_line*) (python.c:434)
>   ==1309561==    by 0x49F54C: execute_control_command_1(command_line*, int) (cli-script.c:683)
> ---
>  gdb/cli/cli-decode.c                     | 19 ++++++++++------
>  gdb/python/py-cmd.c                      |  9 +++++++-
>  gdb/testsuite/gdb.python/py-cmd-alias.py | 28 ++++++++++++++++++++++++
>  gdb/testsuite/gdb.python/py-cmd.exp      | 17 ++++++++++++++
>  4 files changed, 65 insertions(+), 8 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.python/py-cmd-alias.py
>
> diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
> index 7c98029f9f4..132b0ca659a 100644
> --- a/gdb/cli/cli-decode.c
> +++ b/gdb/cli/cli-decode.c
> @@ -179,18 +179,26 @@ cmd_list_element::command_components () const
>  
>  static struct cmd_list_element *
>  do_add_cmd (const char *name, enum command_class theclass,
> +	    cmd_simple_func_ftype *fun,

A brief description of what FUN is for should be added to the comment
above do_add_cmd.

>  	    const char *doc, struct cmd_list_element **list)
>  {
>    struct cmd_list_element *c = new struct cmd_list_element (name, theclass,
>  							    doc);
>  
> +  set_cmd_simple_func (c, fun);
> +
>    /* Turn each alias of the old command into an alias of the new
>       command.  */
>    c->aliases = delete_cmd (name, list, &c->hook_pre, &c->hookee_pre,
>  			   &c->hook_post, &c->hookee_post);
> -
>    for (cmd_list_element &alias : c->aliases)
> -    alias.alias_target = c;
> +    {
> +      alias.func = c->func;
> +      alias.function = c->function;
> +      alias.subcommands = c->subcommands;
> +      alias.allow_unknown = c->allow_unknown;
> +      alias.alias_target = c;

I think it might be worth adding a comment here explaining why alias.doc
is not updated at this point.

> +    }
>  
>    if (c->hook_pre)
>      c->hook_pre->hookee_pre = c;
> @@ -233,9 +241,7 @@ struct cmd_list_element *
>  add_cmd (const char *name, enum command_class theclass,
>  	 const char *doc, struct cmd_list_element **list)
>  {
> -  cmd_list_element *result = do_add_cmd (name, theclass, doc, list);
> -  result->func = NULL;
> -  result->function.simple_func = NULL;
> +  cmd_list_element *result = do_add_cmd (name, theclass, NULL, doc, list);
>    return result;
>  }
>  
> @@ -244,8 +250,7 @@ add_cmd (const char *name, enum command_class theclass,
>  	 cmd_simple_func_ftype *fun,
>  	 const char *doc, struct cmd_list_element **list)
>  {
> -  cmd_list_element *result = do_add_cmd (name, theclass, doc, list);
> -  set_cmd_simple_func (result, fun);
> +  cmd_list_element *result = do_add_cmd (name, theclass, fun, doc, list);
>    return result;
>  }
>  
> diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
> index 5cc392af175..d66311d623b 100644
> --- a/gdb/python/py-cmd.c
> +++ b/gdb/python/py-cmd.c
> @@ -524,8 +524,15 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
>           name_allocated, so release it.  */
>        cmd_name.release ();
>  
> -      /* There appears to be no API to set this.  */
> +      /* There appears to be no API to set the below members.
> +	 In particular, we cannot pass cdmpy_function to add_prefix_cmd and
> +	 add_cmd above.  This means that if cmd has aliases, its aliases have
> +	 been re-targeted to this freshly built CMD with a null func and
> +	 function.simple_func (see cli-decode.c do_add_cmd).  So, we need to set
> +	 the 'func' of the aliases of CMD.  */
>        cmd->func = cmdpy_function;
> +      for (cmd_list_element &alias : cmd->aliases)
> +	alias.func = cmd->func;

I don't think this is sufficient.  In add_alias_cmd we also setup the
doc and doc_allocated fields (in some cases), and those cases are
relevant for Python.  I think what we need is to (conditionally) free
the doc field on the alias, and then recreate it from the new command.

With all this extra work, I'm wondering if we should add a new function
in the cli/ directory, something like refresh_alias_cmd() or maybe
refresh_all_aliases(), then call this from cmdpy_init, but only once
we've finished messing with the new cmd_list_element object for the new
command.

Additionally, there is similar code in guile/scm-cmd.c, it would be
great if this could be updated as part of this commit, I don't think
there's anything significantly different that would need doing there, so
this should be pretty easy.

For bonus points there is also gdb.guile/scm-cmd.exp, it would be great
if there was a matching test added here too.  But maybe that's asking a
little too much?

>        cmd->destroyer = cmdpy_destroyer;
>        cmd->doc_allocated = 1;
>        cmd->name_allocated = 1;
> diff --git a/gdb/testsuite/gdb.python/py-cmd-alias.py b/gdb/testsuite/gdb.python/py-cmd-alias.py
> new file mode 100644
> index 00000000000..d62628841f4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-cmd-alias.py

This file should have a copyright notice at the start.

Also, you should run black, the Python code formatter, against this file
to bring the code into line with GDB's Python coding standard.

> @@ -0,0 +1,28 @@
> +class some_prefix_cmd (gdb.Command):
> +  def __init__ (self):
> +    super (some_prefix_cmd, self).__init__ ("some_prefix_cmd", gdb.COMMAND_OBSCURE, gdb.COMPLETE_NONE, True)

GDB only supports Python 3 these days.  I believe I'm correct saying
that for Python 3 the arguments to super are not needed, and should be
dropped.

> +  def invoke (self, arg, from_tty):
> +    print ("some_prefix_cmd output, arg = %s" % arg)
> +
> +some_prefix_cmd ()
> +
> +def def_alias(alias : str, command_name : str) -> None:
> +    """Defines an alias -a ALIAS = COMMAND_NAME.
> +Traps the error if ALIAS is already defined (so as to be able to source
> +this file again)."""
> +    d = "alias " + alias + ' = ' + command_name
> +    try:
> +        gdb.execute (d)
> +    except Exception as inst:
> +        print(str(inst))
> +
> +
> +def_alias("alias_of_some_prefix_cmd", "some_prefix_cmd")
> +def_alias("other_alias_of_some_prefix_cmd", "some_prefix_cmd")
> +
> +class some_subcmd (gdb.Command):
> +  def __init__ (self):
> +    super (some_subcmd, self).__init__ ("some_prefix_cmd some_subcmd", gdb.COMMAND_OBSCURE)
> +  def invoke (self, arg, from_tty):
> +    print ("some_subcmd output, arg = %s" % arg)
> +some_subcmd ()
> diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
> index 48c3e18f1cc..c7398889fd4 100644
> --- a/gdb/testsuite/gdb.python/py-cmd.exp
> +++ b/gdb/testsuite/gdb.python/py-cmd.exp
> @@ -111,6 +111,23 @@ gdb_test_multiline "input new subcommand" \
>  
>  gdb_test "info newsubcmd ugh" "newsubcmd output, arg = ugh" "call newsubcmd"
>  
> +# Test sourcing twice a file that redefines a prefix command with aliases.
> +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/py-cmd-alias.py]
> +foreach initial_load {1 0} {
> +    if $initial_load {
> +	gdb_test_no_output "source ${pyfile}" "load python file"
> +    } else {
> +	gdb_test "source ${pyfile}" \
> +	    "Alias already exists: alias_of_some_prefix_cmd\r\nAlias already exists: other_alias_of_some_prefix_cmd" \

You can make multi-line patterns like this easier to read with:

  [multi_line \
     "Alias already exists: alias_of_some_prefix_cmd" \
     "Alias already exists: other_alias_of_some_prefix_cmd"]\

though this is a suggestion, not a requirement.

Thanks,
Andrew

> +	    "load again python file"
> +    }
> +    foreach cmd {"some_prefix_cmd" "alias_of_some_prefix_cmd" "other_alias_of_some_prefix_cmd"} {
> +	gdb_test "$cmd ugh $initial_load" \
> +	    "some_prefix_cmd output, arg = ugh $initial_load" \
> +	    "call $cmd $initial_load"
> +    }
> +}
> +
>  # Test a command that throws gdb.GdbError.
>  
>  gdb_test_multiline "input command to throw error" \
> -- 
> 2.30.2
Philippe Waroquiers Jan. 11, 2023, 10:58 a.m. UTC | #3 
Thanks for the comments, I will handle them (time permitting, this is an evening/weekend
activity).

Just a small initial feedback on the addition of FUN argument:

On Tue, 2023-01-10 at 12:16 +0000, Andrew Burgess wrote:
> >  static struct cmd_list_element *
> >  do_add_cmd (const char *name, enum command_class theclass,
> > +	    cmd_simple_func_ftype *fun,
> 
> A brief description of what FUN is for should be added to the comment
> above do_add_cmd.
In fact, I should have indicated in the commit msg that I have just added back
the FUN argument that was there in the past, but when it was removed, the comment
was left there.
I will dig a little bit more in depth to see when/why FUN was removed
and double check that the existing comment for FUN still correctly describes
what it now does (again?).

> 

Thanks
Philippe
diff mbox series

Patch

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 7c98029f9f4..132b0ca659a 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -179,18 +179,26 @@  cmd_list_element::command_components () const
 
 static struct cmd_list_element *
 do_add_cmd (const char *name, enum command_class theclass,
+	    cmd_simple_func_ftype *fun,
 	    const char *doc, struct cmd_list_element **list)
 {
   struct cmd_list_element *c = new struct cmd_list_element (name, theclass,
 							    doc);
 
+  set_cmd_simple_func (c, fun);
+
   /* Turn each alias of the old command into an alias of the new
      command.  */
   c->aliases = delete_cmd (name, list, &c->hook_pre, &c->hookee_pre,
 			   &c->hook_post, &c->hookee_post);
-
   for (cmd_list_element &alias : c->aliases)
-    alias.alias_target = c;
+    {
+      alias.func = c->func;
+      alias.function = c->function;
+      alias.subcommands = c->subcommands;
+      alias.allow_unknown = c->allow_unknown;
+      alias.alias_target = c;
+    }
 
   if (c->hook_pre)
     c->hook_pre->hookee_pre = c;
@@ -233,9 +241,7 @@  struct cmd_list_element *
 add_cmd (const char *name, enum command_class theclass,
 	 const char *doc, struct cmd_list_element **list)
 {
-  cmd_list_element *result = do_add_cmd (name, theclass, doc, list);
-  result->func = NULL;
-  result->function.simple_func = NULL;
+  cmd_list_element *result = do_add_cmd (name, theclass, NULL, doc, list);
   return result;
 }
 
@@ -244,8 +250,7 @@  add_cmd (const char *name, enum command_class theclass,
 	 cmd_simple_func_ftype *fun,
 	 const char *doc, struct cmd_list_element **list)
 {
-  cmd_list_element *result = do_add_cmd (name, theclass, doc, list);
-  set_cmd_simple_func (result, fun);
+  cmd_list_element *result = do_add_cmd (name, theclass, fun, doc, list);
   return result;
 }
 
diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
index 5cc392af175..d66311d623b 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -524,8 +524,15 @@  cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
          name_allocated, so release it.  */
       cmd_name.release ();
 
-      /* There appears to be no API to set this.  */
+      /* There appears to be no API to set the below members.
+	 In particular, we cannot pass cdmpy_function to add_prefix_cmd and
+	 add_cmd above.  This means that if cmd has aliases, its aliases have
+	 been re-targeted to this freshly built CMD with a null func and
+	 function.simple_func (see cli-decode.c do_add_cmd).  So, we need to set
+	 the 'func' of the aliases of CMD.  */
       cmd->func = cmdpy_function;
+      for (cmd_list_element &alias : cmd->aliases)
+	alias.func = cmd->func;
       cmd->destroyer = cmdpy_destroyer;
       cmd->doc_allocated = 1;
       cmd->name_allocated = 1;
diff --git a/gdb/testsuite/gdb.python/py-cmd-alias.py b/gdb/testsuite/gdb.python/py-cmd-alias.py
new file mode 100644
index 00000000000..d62628841f4
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-cmd-alias.py
@@ -0,0 +1,28 @@ 
+class some_prefix_cmd (gdb.Command):
+  def __init__ (self):
+    super (some_prefix_cmd, self).__init__ ("some_prefix_cmd", gdb.COMMAND_OBSCURE, gdb.COMPLETE_NONE, True)
+  def invoke (self, arg, from_tty):
+    print ("some_prefix_cmd output, arg = %s" % arg)
+
+some_prefix_cmd ()
+
+def def_alias(alias : str, command_name : str) -> None:
+    """Defines an alias -a ALIAS = COMMAND_NAME.
+Traps the error if ALIAS is already defined (so as to be able to source
+this file again)."""
+    d = "alias " + alias + ' = ' + command_name
+    try:
+        gdb.execute (d)
+    except Exception as inst:
+        print(str(inst))
+
+
+def_alias("alias_of_some_prefix_cmd", "some_prefix_cmd")
+def_alias("other_alias_of_some_prefix_cmd", "some_prefix_cmd")
+
+class some_subcmd (gdb.Command):
+  def __init__ (self):
+    super (some_subcmd, self).__init__ ("some_prefix_cmd some_subcmd", gdb.COMMAND_OBSCURE)
+  def invoke (self, arg, from_tty):
+    print ("some_subcmd output, arg = %s" % arg)
+some_subcmd ()
diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
index 48c3e18f1cc..c7398889fd4 100644
--- a/gdb/testsuite/gdb.python/py-cmd.exp
+++ b/gdb/testsuite/gdb.python/py-cmd.exp
@@ -111,6 +111,23 @@  gdb_test_multiline "input new subcommand" \
 
 gdb_test "info newsubcmd ugh" "newsubcmd output, arg = ugh" "call newsubcmd"
 
+# Test sourcing twice a file that redefines a prefix command with aliases.
+set pyfile [gdb_remote_download host ${srcdir}/${subdir}/py-cmd-alias.py]
+foreach initial_load {1 0} {
+    if $initial_load {
+	gdb_test_no_output "source ${pyfile}" "load python file"
+    } else {
+	gdb_test "source ${pyfile}" \
+	    "Alias already exists: alias_of_some_prefix_cmd\r\nAlias already exists: other_alias_of_some_prefix_cmd" \
+	    "load again python file"
+    }
+    foreach cmd {"some_prefix_cmd" "alias_of_some_prefix_cmd" "other_alias_of_some_prefix_cmd"} {
+	gdb_test "$cmd ugh $initial_load" \
+	    "some_prefix_cmd output, arg = ugh $initial_load" \
+	    "call $cmd $initial_load"
+    }
+}
+
 # Test a command that throws gdb.GdbError.
 
 gdb_test_multiline "input command to throw error" \