Message ID | 20221208142014.84759-1-jan.vrany@labware.com |
---|---|
State | New |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 6BE45388DD5F for <patchwork@sourceware.org>; Thu, 8 Dec 2022 14:20:52 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6BE45388DD5F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1670509252; bh=eThvUgUrl8ugqWJv3F6TKtXF2yCc2JZb9Bk7iVf7FGY=; h=To:CC:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=Qk2vitKLH23pglPFIwuh4iICKUCsLE2vEPeRXMaGuTlycdkZyf7NI7+PPV9VMlcPL YMkgX5T5oUc9bw7UrDyH+Hw8YWxjw4MtqcseIN0jWbhP1bYx1LUBfSmjgkbof6lrz7 Xp1bODsb8J73A2CrIe+hUxT2Mys3YJi1gT9QyuXM= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-114.mimecast.com (us-smtp-delivery-114.mimecast.com [170.10.133.114]) by sourceware.org (Postfix) with ESMTPS id EF15D3834872 for <gdb-patches@sourceware.org>; Thu, 8 Dec 2022 14:20:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EF15D3834872 Received: from NAM11-CO1-obe.outbound.protection.outlook.com (mail-co1nam11lp2173.outbound.protection.outlook.com [104.47.56.173]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-214-DasTkN8jMIyOiXr_uRTH6Q-2; Thu, 08 Dec 2022 09:20:24 -0500 X-MC-Unique: DasTkN8jMIyOiXr_uRTH6Q-2 Received: from DM6PR17MB3113.namprd17.prod.outlook.com (2603:10b6:5:6::10) by MN2PR17MB4080.namprd17.prod.outlook.com (2603:10b6:208:206::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5880.10; Thu, 8 Dec 2022 14:20:22 +0000 Received: from DM6PR17MB3113.namprd17.prod.outlook.com ([fe80::9df4:7ed9:aca6:322e]) by DM6PR17MB3113.namprd17.prod.outlook.com ([fe80::9df4:7ed9:aca6:322e%4]) with mapi id 15.20.5880.016; Thu, 8 Dec 2022 14:20:22 +0000 To: gdb-patches@sourceware.org CC: Jan Vrany <jan.vrany@labware.com> Subject: [PATCH] gdb: fix possible use-after-free when executing commands Date: Thu, 8 Dec 2022 14:20:14 +0000 Message-ID: <20221208142014.84759-1-jan.vrany@labware.com> X-Mailer: git-send-email 2.35.1 X-ClientProxiedBy: LO4P123CA0459.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600:1aa::14) To DM6PR17MB3113.namprd17.prod.outlook.com (2603:10b6:5:6::10) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: DM6PR17MB3113:EE_|MN2PR17MB4080:EE_ X-MS-Office365-Filtering-Correlation-Id: eeda1aaf-ae3a-49d7-7f4c-08dad927574b X-MS-Exchange-AtpMessageProperties: SA X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0 X-Microsoft-Antispam-Message-Info: yJPlnYdgoCNyMp2dtQDVC4MfOOSOY7JEZNDKQFkgXkBfEQeM++ReoxcK0tmEataoVnlBu6TilKtuNm/LehkiCo6SxYrv5/iv9pFkzr8lu2WNM8vhGSmBM8jzGDQ8yCwpI/kn0b/t6ZGj0INLnBHuRgvHw7updnn3UP8zGPemTwem2zdR4NbomDwg7ofoN45sm2dEHw6njTCNHJuSEbH3JFHhAfk9A0RHJ79rAQUKrmd4xu4zKHYvomkrKDTnaw1xtJFnHyxnvWqvFnwXMWRF7SNLw+yWFAb6ygtbqdZXwLbgaI0FcXPPF2djnZwKbBuLiWHT52VDM/IzZykFqYvQsCdEWT/xh9LXYV81jzXO2SvgNHPXeZPdsL35DbPM4ZY7FqprVGyKzmAtkj34SfmHcn2ZjufXDhiqfuoi2OYukqifJfot3Hr+zTBTxcHeLEwsu8fg7hNkXyjzkkhiDn7jzQ5xUkppKQV1ZUNmUDpcCl55rTV6YMHL1KpwW952gJxj3c4+PP+v7sAM+rNrRe/VM2Bhwan55/DhjxHEV6HYI8+c25VYRefss4OJE1LpeAs2M9plwExQE8QyVzSPNiYcpuLDqcLaIwlSW9qQDV56fuh9FmS08sgWCoNwRpWck7DEB+wCR7w4FevBMOA4wDA2bALkSn7hv34JLJK41TVNls/f1tbld1dgIVZR2Nh5Wm6+PRSv4di5vpvJG43RZJ8U3w== X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DM6PR17MB3113.namprd17.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230022)(4636009)(396003)(366004)(136003)(346002)(39850400004)(376002)(451199015)(36756003)(2906002)(38350700002)(38100700002)(86362001)(8936002)(44832011)(83380400001)(1076003)(2616005)(186003)(316002)(6916009)(66899015)(5660300002)(6486002)(478600001)(6512007)(26005)(52116002)(4326008)(41300700001)(8676002)(66556008)(6666004)(66476007)(107886003)(66946007)(6506007); DIR:OUT; SFP:1101 X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: rjr0MFuk6D2AHmLNiwdbODd9YdoCJNK4JeJkJBZ/uHF/Ut+VPkWpkjxrdPxhHcbRH0YKCYzOyr2JWanPpJuthENXT4m9CG3nfs176asptBWS0o7PNvA1HTv38Fp86KNvYNKgHmhKHsXMktH0v1Yj5kpZaDriE9y/BqEOvVItNEzNXPXaGnnsZYNJbvrfSDWBn+jyUBhkrefsdBgXu2FTMRYCff/lGb/vQozZd4wid6cac/JyauPK7eBD414pwbrw7wK20Eehz+Puh5sBYepznQGtIceOpT40eKYziFSJgtA1RN0UIPkTB5aO/4UbcuUDjUo88f0Tr4UQpz+7ZVaKinKOr0yFf9SOSGCg2ifH1oSgtah6ANkbmmBcfXHL4OFR/TGrNoDo8m+qDIwnHB9/ghJOgk/iRG0QLoZLbQCr5g4j6XkFD8wbuFTWtYJdyt5LILz2DHPLYDAnmmw5wehcHENMsNQZ6v39Lz2NG0p1sfSZGS0tTufXPt2W8rsFNQBWYHs8Q6EtohA7+jrg/XfDJ5B4G8jS0EBupaYntrkoqh024cFL75vvBzEdIUq2IBTs4uHdvCEnQTs3mUKgnOVbhkxb4ocaCJ5RvaUoWibDN0iXxpHxu2/krdmLGSdXkd+zmHHu7fuApgPY4HT7r3XbP5NhInHv1mD4WFLhktVjxCuHq5ToNRCjkJDUulDjgmHLAUKFfd7oY3TIb6m0urq4YIBh9Nmr2S75kIcVseQxJFmJN84S5T3dDVY5dwn9Zj+9w5yld9Nn0hZ2msu1sAuMeygq2GkwwAM0V3sO6T7+3x4xtrop+Jkl3BwJYU0aCs/56EkrG+zsyEQH+XGKcr0FU3lqtKlissZX9iAgA7p1PFzG0jX3IUjAQn3nXuySgp5rt/4ZypycCYFQH537tJny1uiK8kyyXyAK4UBu7uzuuyubYUI73P2Xafsvy6we2ic5cy1KeX6XvsOm+mtS7GYQeEEgcEPr1ilmxWrZSj1AG+WEsM9l8l+i6rxA/UKID3poYSBG0r/ebY8/p6z49f14ZwhzDlyZuqWw9fSyCsa804LUIDz1wym4WXYxPKNOvlCk0Vjt861YB0JAK/gIblhr8lMG12v/BXdKRMUlbU481wdqsXTY3zMwHlsdI6KBrr7o33jpCYiWxxKPYsEndF9l1myPMsrKjk0sBYLhw7IPToJeB4dSROOmV8XJ8mN93h0L0m9QRK0wLGjhwLlc687YczC7I88ikZ/fE3OysX67D+rT+FbQv9/xpr2qn5s8bONPy2Xpaios1AsKQEF53XgSJlr9n/2CKSgRuX9gL0mJH9BOgH2fmP50YQeKbSqVKbkLxGaxoU2uHfJYgd3C2WgNVyYhqt2ra1DRUOWXn6pykWapMDL+DBbUvVZOeR7rFRDRfApz+kiNbScxFL44CE+vLkPeF0kypIP6tTkSFcShj0rsbS/uFkA/uxmDngerkEeq53t6eoUhE63O+HcxZ12z/UlVL72uKeSUJ+9GCvmoWDlvuwyqYy3TAYHma4I2ReRcUhGyI4FNz1q0/NiwH+4T+MTdwv6wWhvN7dWSKgLfDTuYpxjEow/1CPO63KbJpq7Z X-OriginatorOrg: labware.com X-MS-Exchange-CrossTenant-Network-Message-Id: eeda1aaf-ae3a-49d7-7f4c-08dad927574b X-MS-Exchange-CrossTenant-AuthSource: DM6PR17MB3113.namprd17.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2022 14:20:21.9582 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: b5db0322-1aa0-4c0a-859c-ad0f96966f4c X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: MFmfTvXqokVrA20++8b/P1v57HTVoiWyLF5YGGGdkUU564pHmySaIFA1JLUkahehfZ7UlNLwtb8uzmkZEfOMyg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR17MB4080 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: labware.com Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=WINDOWS-1252 X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Jan Vrany <jan.vrany@labware.com> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
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
>>>>> "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
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
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 >
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.
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
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 >
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
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" \
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
> > + " 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
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 >
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) {