From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id A0AF33858CDA for ; Tue, 10 Jan 2023 12:16:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A0AF33858CDA Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673353019; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ZHQDx3H6pB6VYW9mhPh0Sxv47I7xEnrlW+Xu4DR+NUA=; b=Zk59ekrAU0gtCUv3ehOBwqEaaUzRAvZi9G2p7CxZ/T3q3QcCwzVwLF0ZFE9zURCmIKPU3K KR45q5WzvEbCCPlfwT3flSLogj5CwUtUU4bP0AyQxaK3kL4C/zvvZlTWeDd/MjRsywsSqR qOJcTXg52LzPzv5gImYo6ZHXg9PIX3E= Received: from mail-vk1-f198.google.com (mail-vk1-f198.google.com [209.85.221.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-328-ViKVHWbFPwm99yId2NCHaw-1; Tue, 10 Jan 2023 07:16:58 -0500 X-MC-Unique: ViKVHWbFPwm99yId2NCHaw-1 Received: by mail-vk1-f198.google.com with SMTP id g191-20020a1fb6c8000000b003d5f1d80ae2so3654066vkf.1 for ; Tue, 10 Jan 2023 04:16:58 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ZHQDx3H6pB6VYW9mhPh0Sxv47I7xEnrlW+Xu4DR+NUA=; b=G+Sb8Mr2g7lsELmcrRxBd51j2lUv7cQKy8yLD2EMOcwLbmbTDFUIWC973K1isaXkB2 Z1BPPav/mYnZStWN1lwmyzxMNu4OP5ZJITzXmS71n/blMwWMRk8gEDr0RoHrA1D7ZMYy HxFln8RJy8F2YQ09HGlAA58lMRi9cx/lv1GRQcihZwro0/yy1IBKcvRF/CGGgrTf3/Am Uey/A74z6HXzbzoCeZnL7VbGX8sZqURpGFR3Oh3LgEqdoeW1xajRinZ1b253NkmupBfM NaPKCjzF6hBn6aaGG+w4jvGwrwHhtujj2QLFECFnjiDjqCAM/DhyNKFM33UI/QiAqUtu YF2A== X-Gm-Message-State: AFqh2kqlnxxr6uAe9NF9dhsHeO2IAv8FTMzu9tZqh6NVKpofk8/d6NML xFq9GN6gXJO6x+Z5OiCet+fSYTEBPW9g62VdqHGry91OL0X21uO2g9VofMSijB2UiM+Fr4yqbRq 9P/TQIz4cr6GRi54MYCxIGm2mvJwfypOfN/yUBW9wA4x9dsCxSjWOH5gV764RxhUoJfBJ/ET4pg == X-Received: by 2002:a05:6102:ccf:b0:3d0:d20f:acff with SMTP id g15-20020a0561020ccf00b003d0d20facffmr246850vst.7.1673353017518; Tue, 10 Jan 2023 04:16:57 -0800 (PST) X-Google-Smtp-Source: AMrXdXtJ5R3JTy3WaAs6sllYBCuPPmd1Ba3osH8BdwEoLiP31xN+S3n3CRoUmaJrVuN1JDsLvzGB9Q== X-Received: by 2002:a05:6102:ccf:b0:3d0:d20f:acff with SMTP id g15-20020a0561020ccf00b003d0d20facffmr246823vst.7.1673353017129; Tue, 10 Jan 2023 04:16:57 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id y9-20020a37f609000000b006fa2b1c3c1esm6921070qkj.58.2023.01.10.04.16.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 10 Jan 2023 04:16:56 -0800 (PST) From: Andrew Burgess To: Philippe Waroquiers via Gdb-patches , gdb-patches@sourceware.org Cc: Philippe Waroquiers Subject: Re: [RFA] Fix use after free caused by redefining a python prefix cmd that has aliases. In-Reply-To: <20221226170823.2744157-1-philippe.waroquiers@skynet.be> References: <20221226170823.2744157-1-philippe.waroquiers@skynet.be> Date: Tue, 10 Jan 2023 12:16:55 +0000 Message-ID: <87fscih9e0.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Philippe Waroquiers via Gdb-patches 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