From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 6D485384F952 for ; Thu, 15 Dec 2022 16:00:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6D485384F952 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.0.64] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id BDB891E0CD; Thu, 15 Dec 2022 11:00:00 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1671120001; bh=9V9Zu+IIuxkyGMVC8S08cQz9nA5A6NOnNVTGWcIxtvU=; h=Date:Subject:To:References:From:In-Reply-To:From; b=k8BQszDL93ES7nll6B74FRoEsnEdOgTISLXocDC/ZsDEpAdQIcwnkLyZmhoQ0tkIv hbOYQfyw24txqyqt3B7Y/pVKYDWcyZDhDwh0zZQ2vwCXasQue0q3FS8gEnxD+EqXmB b3WI+nW4eGmdPILCTDoXnd4SpaKzkYQKvbDrq1o4= Message-ID: <2f491de8-59aa-5b2b-3c53-1ec950ef1eeb@simark.ca> Date: Thu, 15 Dec 2022 11:00:00 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 Subject: Re: [PATCH] gdb: fix possible use-after-free when executing commands To: Jan Vrany , gdb-patches@sourceware.org References: <7ced24a4-19d5-293e-b7d6-88d77d3aea7a@simark.ca> <20221215145140.39092-1-jan.vrany@labware.com> Content-Language: fr From: Simon Marchi In-Reply-To: <20221215145140.39092-1-jan.vrany@labware.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,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: 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 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 >