From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM02-BL2-obe.outbound.protection.outlook.com (mail-eopbgr750113.outbound.protection.outlook.com [40.107.75.113]) by sourceware.org (Postfix) with ESMTPS id 295E338708FA for ; Tue, 9 Feb 2021 14:19:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 295E338708FA ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=J8PX7NWVC3EQbPxa35llajsxIFXWwYMzOAnAghxrxDcv7rngf1zL8TzPY5xFg7+BQazTbwDkY7x+lb0d8kTXv/dR28XeT9L2kOvxnxUx2tEEpEjTkov0QYdlE52otGQCW+7CbvWuF3mwvQaM91fQaLytp+inj2ex38wQRlsPXBwa+fzOIb/piXRoRCrjn6zR7DQsWFNZumx1EBOpj8z1/SRXsQBdcWVvO1HH43fqcJ0IaljvvoBFYkTsmeaKl81DLbz2Onup9zzqd0/IYJoWIPBscOQAKbrBNCeEO14u83UdqN6oCb78mNHs9o5KjsT0KYM4F7nA/TgMNLWRf6VjGw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=FZ2O/MVk4U1kSclPpggLjL6gIWd7I9DRXLLyE9XcyHE=; b=kU4Jskrkl/oeuyWxwjEvPEFbkVZpjMqDa/dDD6or7pX2T1mGyXLozyz0IdqlUEUC+QBpYziJY8g6NxUP1zDN3gjaO24ctngULgLjQN68fQqtOy1YDh45pqFxvnd/PCZ3pl3iwh6k047FThvnB/huztkbJBCpqUGM3HefPRiCJb2bwoXDrtk4TIyA1GWT/dYOMRFrg5QoYxtprqKn+j9V9IaBX5uH4FZGFVbjtJ6OMYjzibg0Wbu7vkaRkCyZ2liLEnOC7fhHu/BZ40qVIRUWow8dXKNk7WrDu/wo8jYcRQRmHThpJ35/wOdKKU82TYbZQhKbVAQHBhBdREmJ496+eA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=cornell.edu; dmarc=pass action=none header.from=cornell.edu; dkim=pass header.d=cornell.edu; arc=none Received: from BN7PR04MB4388.namprd04.prod.outlook.com (2603:10b6:406:f8::19) by BN6PR04MB1027.namprd04.prod.outlook.com (2603:10b6:405:44::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3825.23; Tue, 9 Feb 2021 14:19:13 +0000 Received: from BN7PR04MB4388.namprd04.prod.outlook.com ([fe80::f071:e174:ef12:375c]) by BN7PR04MB4388.namprd04.prod.outlook.com ([fe80::f071:e174:ef12:375c%6]) with mapi id 15.20.3825.030; Tue, 9 Feb 2021 14:19:13 +0000 Subject: Re: Potential handle leaks in dup_worker To: cygwin-developers@cygwin.com References: <2fef1107-005d-9a44-fd4a-79fa5904d436@cornell.edu> <20210209094723.GQ4251@calimero.vinschen.de> From: Ken Brown Message-ID: <4b56fffd-5401-bd8a-0444-4b8b7a8da4f5@cornell.edu> Date: Tue, 9 Feb 2021 09:19:12 -0500 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 In-Reply-To: <20210209094723.GQ4251@calimero.vinschen.de> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [65.112.130.200] X-ClientProxiedBy: BN6PR14CA0032.namprd14.prod.outlook.com (2603:10b6:404:13f::18) To BN7PR04MB4388.namprd04.prod.outlook.com (2603:10b6:406:f8::19) MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [10.13.22.4] (65.112.130.200) by BN6PR14CA0032.namprd14.prod.outlook.com (2603:10b6:404:13f::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3846.25 via Frontend Transport; Tue, 9 Feb 2021 14:19:13 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: c5c8acbd-b744-475f-a41e-08d8cd05ad25 X-MS-TrafficTypeDiagnostic: BN6PR04MB1027: X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:8882; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: S2lBUqpTELXpp0Upjrgdn43jKK83927JWmcoPhIPAbCfvVVnvy5qsEwYoNPsMJLbTeHDYQsATaXmpg4LzwiHrLN0QGkvLsctzVwM6qhG9IJ/Lgnwr+xz1PrTMFd1kHxrvJGmH21XYV+Mk+Y/fcqpy0cIPontrc9DKfp0SBOhP1qXR7tNayrkzSYBTxY52TespHrO+2Y4uxYy3ypozSvMdMLJ1D5zXy6QSNVKwkc4LZohRQa08KTdwpVFgBzWKFi1A1+HvBHLFYtaumgjSQ7JDw2GOanHb0hhgHuC/olTWgoPwG7s1bVStdqQDtV2veTk5CvAItu1l4m5MQwjVBPIXDROGj4fZjV0jjVhDbxGoUn+xEWxuQed4L/mJdNwT3vp+IN9dA1wh5Azk5v4C3P6buby3ktkRj7CmdNct6W7otyb/OLzef9td8q5HmACGEftRo+3Rj7ys9Pf7Ntmza5PHX8la0mB4g2Bxu5+zUMsI1E+UdY90Y/7o8sLaDUrT+3KUocBKZoZgsqkXMjlxjSMpKvyEf8pPpt0Y46oxv9fUnJ8kWCjkA2OE1+F03SiJWDs6uM29oqp1Jh39BZjZNEzG/9uazoB9pyB6UqZ+vTsv+Y= X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BN7PR04MB4388.namprd04.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(136003)(366004)(39860400002)(396003)(376002)(346002)(316002)(2616005)(786003)(31696002)(2906002)(83380400001)(6486002)(956004)(8676002)(16526019)(31686004)(66946007)(52116002)(53546011)(186003)(36756003)(16576012)(86362001)(6916009)(75432002)(26005)(5660300002)(478600001)(66556008)(66476007)(8936002)(45980500001)(43740500002); DIR:OUT; SFP:1102; X-MS-Exchange-AntiSpam-MessageData: =?Windows-1252?Q?hGZLwXz4yAzrKGckbr1Ue67mfZpHJfFMXJ7bdt4xAcs2Wq5VyYTwjWxq?= =?Windows-1252?Q?+tdwyCRWMHwmpcAhhlbOzfT7clEVj+KaKWi6Swz8SK88Jc/3YLQnb1vE?= =?Windows-1252?Q?kHMgjARgiss8tIrpYUcfFrF1EBL0sbssHbnIXldtXoLs5nzIEqq5qUgr?= =?Windows-1252?Q?og5SSPS3/IvGYRgYsZPufR2twOk8JMh9CgGDwvNMFzzXvOhOFQ/O5IQO?= =?Windows-1252?Q?uwKS6lvKr1P182JXvTF/QpF3d1u8EUplewC9AzvA+xE7L6yCDDTkrxbh?= =?Windows-1252?Q?JoUOVS5a6TQIpXfOvTrZ61FwcNGWrKrTbNhxOx79FX7c4owqaQgJ0TBH?= =?Windows-1252?Q?ncM9W4UOHl27ax6XpVaSRPm860OtCVZMleVqrnZb7XShwFBjTIAPsHtq?= =?Windows-1252?Q?odunSCWc4OqKgeQqp9M79O2yCldID/S4r7Y3RBKDMbhM1TGx7Bnfs4am?= =?Windows-1252?Q?vSIfTGCCRAnmfmyptsRNa4FC3Wp+vFkJft8ESmBwYZxAtnaTFVAf4Bb3?= =?Windows-1252?Q?fcA9k/+OA0tBV2Eh/7d5yYV1NepNxZuGyx6jzOcHgLZxTv+zKvFm5Jcl?= =?Windows-1252?Q?7KyYbdvgWhGQFohCPgBy7sL/LRedoZiA3w9dhGRyhFUetMtioni2NdiP?= =?Windows-1252?Q?dhT6i5qrwblrrW4VI+z+ru7Vkc9EwTi9m0SPp6PnTAtZeppFuDz+ZvZM?= =?Windows-1252?Q?uw0V5fIb7RBnvxdvxDvMJ5mog6WyyVdh2uVdtHYNo/gDJrTqS+vBW1A7?= =?Windows-1252?Q?gLgMIbOvROPshxHjeu9GF3rSv1ruUK4cXrtLp45TRLu0cM41xuNmg7EV?= =?Windows-1252?Q?pNTJkVSqx+mt9NBd5G57XyzvUNkNL7pk2hqsvF1U8GLmdKpe5OawZN9E?= =?Windows-1252?Q?ivav5OlSyGmMU3D8iWyue3rFvyNexOkMQMrAHjXnNZYhUFG3rVAbEPM2?= =?Windows-1252?Q?WusUxMB/E/Ble3OAW0x+Avm0JhLjF7Cu/c0/xQ7g26kGm8aHG5vxhYrY?= =?Windows-1252?Q?G0k0zGg0zmTtu+iKkvHy2b8WAIk/4Y5XZCofY9FVZ6QOhnGpK/nnIRZC?= =?Windows-1252?Q?HxXp1fuEROQ3OEx0lFWkINGM03aSjaRBVj60yULV000SfMFU87Szdx4z?= =?Windows-1252?Q?iMP7Hwb4QCWzhqgUycDu+pPfksYUXAGj9TFT4eZUWcnCghXQUvX4S/nm?= =?Windows-1252?Q?oBA1oTvsPylr4R2KiZn5/M8Sj+ciL2tRyNvul1WlgmzlCnMGHc3Z8ozP?= =?Windows-1252?Q?tK6y4bFtPGqW7oPTJTjbQ1CSVFNKVbWPPsnfQ2p40zrmbT2SADU+U0iS?= =?Windows-1252?Q?Ha1rJGOcqH92QQqWaI8f1Hl3lY271Q6Ni29nIg/WSebWzfdtriC6QACI?= =?Windows-1252?Q?btcwAVrAL9fjWOlh6qnvXkamrnccKfaOFxx2mJ60dLYEGXgLb+yWyv8x?= X-OriginatorOrg: cornell.edu X-MS-Exchange-CrossTenant-Network-Message-Id: c5c8acbd-b744-475f-a41e-08d8cd05ad25 X-MS-Exchange-CrossTenant-AuthSource: BN7PR04MB4388.namprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2021 14:19:13.8369 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 5d7e4366-1b9b-45cf-8e79-b14b27df46e1 X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: Fv3V1NlF8AMe0lr7KYNU5SYkQPD6nEA9uex0aDdAeSfaW0yC8vCsX6Uyer67enBcCOiDkHAG7dcTXu0VJwhh5A== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR04MB1027 X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, MSGID_FROM_MTA_HEADER, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: cygwin-developers@cygwin.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Cygwin core component developers mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Feb 2021 14:19:16 -0000 On 2/9/2021 4:47 AM, Corinna Vinschen via Cygwin-developers wrote: > On Feb 8 12:39, Ken Brown via Cygwin-developers wrote: >> I've had occasion to work through dtable::dup_worker, and I'm seeing the >> potential for leaks of path_conv handles. I haven't seen any evidence that >> the leaks actually occur, but the code should probably be cleaned up if I'm >> right. >> >> dup_worker calls clone to create newfh from oldfh. clone calls copyto, >> which calls operator=, which calls path_conv::operator=, which duplicates >> the path_conv handle from oldfh to newfh. Then copyto calls reset, which >> calls path_conv::operator<<, which again duplicates the path_conv handle >> from oldfh to newfh without first closing the previous one. That's the >> first leak. >> >> Further on, dup_worker calls newfh->pc.reset_conv_handle (), which sets the >> path_conv handle of newfh to NULL without closing the existing handle. So >> that's a second leak. This one is easily fixed by calling close_conv_handle >> instead of reset_conv_handle. > > Nice detective work, you're right. For fun, this is easily testable. > Apply this patch to Cygwin: > > diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc > index 52a020f07d5e..58e993b66c42 100644 > --- a/winsup/cygwin/syscalls.cc > +++ b/winsup/cygwin/syscalls.cc > @@ -1475,6 +1475,10 @@ open (const char *unix_path, int flags, ...) > int opt = PC_OPEN | PC_SYM_NOFOLLOW_PROCFD; > opt |= (flags & (O_NOFOLLOW | O_EXCL)) ? PC_SYM_NOFOLLOW > : PC_SYM_FOLLOW; > + > + if (flags & O_NOATIME) > + opt |= PC_KEEP_HANDLE; > + > /* This is a temporary kludge until all utilities can catch up > with a change in behavior that implements linux functionality: > opening a tty should not automatically cause it to become the > > And then create an STC like this: > > #define _GNU_SOURCE 1 > #include > #include > #include > > int > main (int argc, char **argv) > { > int fd, fd2; > > fd = open (argv[1], O_RDONLY | O_NOATIME); > dup (fd); > } > >> As a practical matter, I think the path_conv handle of oldfh is always NULL >> when dup_worker is called, so there's no actual leak. > > Right, because conv_handle should only be non-NULL in calls to stat(2) > and friends. > > Nevertheless, it's a bad idea to keep this code. So the question is > this: Do we actually *need* to duplicate the conv_handle at all? > It doesn't look like this is ever needed. Perhaps the code should > just never duplicate conv_handle and just always reset it to NULL > instead? I've come across one place where I think it's needed. Suppose build_fh_name is called with PC_KEEP_HANDLE. It calls build_fh_pc, which calls set_name, which calls path_conv::operator<<. I think we need to duplicate conv_handle here. If this is the only place where duplication is needed, we could just do it in build_fh_pc or fhandler_base::set_name. We would probably want to add a path_conv::dup_conv_handle method: inline void dup_conv_handle (path_conv& pc) { conv_handle.close (); conv_handle.dup (pc.conv_handle); } Ken