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

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

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