From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM10-DM6-obe.outbound.protection.outlook.com (mail-dm6nam10on2095.outbound.protection.outlook.com [40.107.93.95]) by sourceware.org (Postfix) with ESMTPS id F006B387086B for ; Tue, 9 Feb 2021 22:31:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F006B387086B ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=MMjiBq3W46C9W+M6ur739COt2ZJCQ/9CHkYh6HRgkaISQAFpo0AZKhv4YteA21BfyA7/R4w9NSi3jNahl6gTjJDc6NhAOTuvIpG7MYvvgbLIfJ6+asNJ2Ei4iAKhVlXo6jmet4LKMOVd2pKgzcfTffIH3RczrI0LZaiOOPKDoE9+OanBUwLMjRb4u5CXlBtR4BwZdOeH8wDI3STfJY0ft1XQdVCg4L86YA4FmarL6NT5DBSRptGTF3QCM9MkaopJYKtQzzXqRAdPmJ1VpaEeEvy48luDdYwwKdfD3eKylakk1sYhdB5uSlnhIDV+dSIzL6f5THzoYUjxEyzoL2fooQ== 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=9j01LKfdkbLHcROyOUAoZFPdUbTEinQLl38uulrwq7A=; b=VpoupfEnDqUyU7lib1POw72bBu41y/Y3/g5Vo4iPd9OCMCx6nLHikRRPy462Cmilw6JeeCOWtnHLlK9WBfrbIKbqvhuy+cbxCN1kXy57QrrLeluuyxnmLYCru4LGVyeUPAyoQcp5RkDd6gGg6+uSg+ApcBgb0RbrkyU0iFuvI+gUKttTkHkESWFlP56GyZXIxsZo14Byqx4zbUm/oI0UK6f7TsFqzWQWAyozwA5WHLCeDZmzvU6nP63hXrhWkWT/ABWMkB+LEaXMsJbkznBedBGwPa+/N5D33byNB+QJgNfnYqa745J7z+JV19HqPgk4FBoZNXbuhWFMuaVhi/MwXw== 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 BN7PR04MB4052.namprd04.prod.outlook.com (2603:10b6:406:c0::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3825.27; Tue, 9 Feb 2021 22:31:17 +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 22:31:17 +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> <4b56fffd-5401-bd8a-0444-4b8b7a8da4f5@cornell.edu> <20210209150249.GT4251@calimero.vinschen.de> <4b8c6b7a-07af-2080-e105-2f22374e5bd8@cornell.edu> <20210209161223.GX4251@calimero.vinschen.de> <16299cda-4d87-cd94-d4a6-6677438fd4d6@cornell.edu> <0745b429-d297-c424-8c60-540ef9593a37@cornell.edu> <20210209205257.GY4251@calimero.vinschen.de> From: Ken Brown Message-ID: Date: Tue, 9 Feb 2021 17:31:16 -0500 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 In-Reply-To: <20210209205257.GY4251@calimero.vinschen.de> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [65.112.130.200] X-ClientProxiedBy: BN6PR19CA0119.namprd19.prod.outlook.com (2603:10b6:404:a0::33) 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 BN6PR19CA0119.namprd19.prod.outlook.com (2603:10b6:404:a0::33) 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 22:31:16 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 8d9b1e7f-6edb-495e-ddc4-08d8cd4a6a75 X-MS-TrafficTypeDiagnostic: BN7PR04MB4052: X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:8273; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: xlPvZo9Rd1Eh2w6P7F/onb+dmXWoKtOLwUKr+RG0o/zTc5LHJArjQ8+Qgszh/3EcTvnnE0UGQc97Bx5ztILtioVK1CJSVCkV8X1IH7rqWVOHv/IshdZKFprU7867CZMGEhFhEFGQdI0wIcGVpiWVDYpSd/eg8JJdxQ8reTh/7YymgIYTYVRSoHnOvkL6tx3sfnIsIXBl7dfy5zYUMV0CsH1YGUoKNUBSbd9Kvkznns6rw3k05iON2fanOfU1fOeIo3RLpzKhbwtJCILKYbb2hiM9KSU7cAj8u1jDHJ70YNNRRV2d5+En8VRVgKSjhMpvUnsB1x+7nBlJ6Ul1iRw+3G9LV2n90xMwX4O0cPkr6T5QJkAHP5PDS7ldgJcFo/Xrlnde1AzkZ1BVQcZKo50S3W458f1WOEIuj+PP2ROdWrQXHn/wnVZNeP+CGJSkhYTdYYqPX3T0W7YUwE1jKlly0R3z7DpW+Lm/CozEsjQHqqKnxSBvtX35bZKo52U4Cn8/UqB6UgTbO+rkwrDN7fQKctBRJDAkShmGBz/S6duq8PEiZ9SXTLGF02OyZV2FV4+s8RmW5bnNkeuVethSaE1Ugp3YUYdT5XDb3Ph3HD5YOCA= 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)(376002)(136003)(346002)(396003)(366004)(39860400002)(6916009)(86362001)(478600001)(31696002)(6486002)(66476007)(8676002)(83380400001)(16526019)(52116002)(53546011)(66556008)(5660300002)(66946007)(26005)(16576012)(2906002)(8936002)(2616005)(31686004)(786003)(75432002)(956004)(186003)(316002)(36756003)(45980500001)(43740500002); DIR:OUT; SFP:1102; X-MS-Exchange-AntiSpam-MessageData: =?Windows-1252?Q?q8t5HOXd5b2CbJ1zpDCgocwhjYA2G/eEiIsatH94ePBvl3Y46CfU01Xs?= =?Windows-1252?Q?D2bTT11Ylc7SVB2yNi3XRj5ejmy9n3BkgZgFmHXK26rPG9xuXgt5Iv42?= =?Windows-1252?Q?Hhrg0BCHn0nZV3GJq+JRknrxrxmJixOGP1G2mF7ksCw8LBrMb2SQRple?= =?Windows-1252?Q?cOBPmmrqodkmYENvehwlSFL9KAglCtb96humKRyDbawumu/Ol0auE/TM?= =?Windows-1252?Q?fsFZ3wUHO2SHkl/srlTBDy+9OlCFgTDYAvffDZdVhyl9l1mhhbQh07wN?= =?Windows-1252?Q?lV3sn3idIZaNJ3IUVR8XcCMd5XQxhAaBusQIjKVKjuvGa96P4pGi5tEZ?= =?Windows-1252?Q?shZWg++uG+A4hN4yKQfcU1Kzbuefn7Ullj2wJsX7FwegzGDKmpdhHUib?= =?Windows-1252?Q?xhipjYRP7VbG5cIWdjkHKlTLRW0FPDVywy96Joe1P5E09sYFXDuruta0?= =?Windows-1252?Q?NtHJCbDuuH+ObZALw/FqMnGvwvL0fUDILZF46S+TrHw7Ju0MIa2QnQ2v?= =?Windows-1252?Q?S9Zm2kkaX2EmhhU6C+GKUCCpRL0x6CnDPgjrxSrlDLsyMeryO7bm31La?= =?Windows-1252?Q?bWBZd6XhjbSLcUJJmwpYSiIoFXzyGhWfE+pntw6VN4xTRHVxulo9EZzG?= =?Windows-1252?Q?s2JOy+Ieavcf8u19EGBTXDlRaANKDvbtxztQJccoakey6dUbWRtG2LZK?= =?Windows-1252?Q?e6bgF4HzuAZc7SbZA0rRWsamPUTt35N41/Xjg26bC9bjuH4UqqTwutNO?= =?Windows-1252?Q?zbSVieRQXglRFdGjU6rJN97PR2OFed3pWnu1nrPxVO7nSkbHcgOEFG6Z?= =?Windows-1252?Q?vQMMfwpfIRykKdZctk/J2wBZM3YMAK3nqKVn9ZKbOPfq12aQgUS9Qh0j?= =?Windows-1252?Q?1EoYnrmlRlTxp/favOGPkqtznlZIpvU5UC848c+HUhNBnG9KY0KmadRm?= =?Windows-1252?Q?Y7gdpvdGtpvwVe23lG/vuCUFBH2EmMXCwA+vM/X/rnOiECesAVMYWugJ?= =?Windows-1252?Q?zcEAKyPKUHKM69HVTwnCgm3uPZF1rFb3BhKNTM+pzOgX/PyVUEOSxlmV?= =?Windows-1252?Q?1EbBf2u45xEa+QYIb0Dh9r25eShBN9LnMVl2SjCvqwNlAfLAgTETee7P?= =?Windows-1252?Q?vm9/w4s80/8EAtKtMwh5L9rf/0t3bf6Cf4dP5wtwcKiASUpIC6P+5p2O?= =?Windows-1252?Q?gP/gdxwjaql61l711TER4sGUE9e4ic/We30yyD/9jXtOGIYF9n3Siipw?= =?Windows-1252?Q?T6CR67zCN/bIeomwsX3gJoCP+kVoqJxf2I03voANuZATYNahEBKeY3W6?= =?Windows-1252?Q?Klt1yzgDGYVohQ9iDGqjQnOHemYhuxVtXzt82TD+3onGHVcUi2pXyIS1?= =?Windows-1252?Q?lqbsh3HtM4JkB6Ev8rEEqkMca3XHVpmzAubbbZ2V5R0zJbtpnkvt7kBz?= X-OriginatorOrg: cornell.edu X-MS-Exchange-CrossTenant-Network-Message-Id: 8d9b1e7f-6edb-495e-ddc4-08d8cd4a6a75 X-MS-Exchange-CrossTenant-AuthSource: BN7PR04MB4388.namprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2021 22:31:17.2737 (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: lB0feGC9eZbi8uvoTwTY0EH2qmu186zqcSZxr6TZF5JvRv8/vah0T+qH3L7HXJyUUkRvKdiBNBGg54O2ZGlsaQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN7PR04MB4052 X-Spam-Status: No, score=0.0 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, MSGID_FROM_MTA_HEADER, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=no 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 22:31:20 -0000 On 2/9/2021 3:52 PM, Corinna Vinschen via Cygwin-developers wrote: > On Feb 9 14:12, Ken Brown via Cygwin-developers wrote: >> On 2/9/2021 12:13 PM, Ken Brown via Cygwin-developers wrote: >>> On 2/9/2021 11:12 AM, Corinna Vinschen via Cygwin-developers wrote: >>>> On Feb  9 10:31, Ken Brown via Cygwin-developers wrote: >>>>> On 2/9/2021 10:02 AM, Corinna Vinschen via Cygwin-developers wrote: >>>>>> On Feb  9 09:19, Ken Brown via Cygwin-developers wrote: >>>>>>> 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. >>>>>>>> [...] >>>>>>>> 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? >>>>>>> I've come across one place where I think it's needed. >>>>>>> [...] >>>>>> Indeed, you're right.  I just found that the fhandler_base::reset method >>>>>> is only called from copyto.  Given that fhandler::operator= already >>>>>> calls path_conv::operator=, and that duplicates the conv handle, why >>>>>> call path_conv::operator<< from fhandler_base::reset at all?  It looks >>>>>> like this is only duplicating what already has been done. >>>>> >>>>> I think that's right.  It looks like operator<< differs from operator= only >>>>> in being careful not to overwrite an existing path.  So I can't see that it >>>>> ever makes sense to call operator<< right after calling operator=. >>>> >>>> It might be helpful not only to move reset to a protected inline method, >>>> but also to rename it, making entirely clear that this is just a copyto >>>> helper and nothing else.  I. e., something like _copyto_reset_helper(). >> [...] >> Trying to make _copyto_reset_helper() protected leads to errors like this: >> >> In file included from ../../../../newlib-cygwin/winsup/cygwin/spawn.cc:22: >> ../../../../newlib-cygwin/winsup/cygwin/fhandler.h: In member function >> ‘virtual void fhandler_pty_master::copyto(fhandler_base*)’: >> ../../../../newlib-cygwin/winsup/cygwin/fhandler.h:2461:30: error: ‘void >> fhandler_base::_copyto_reset_helper()’ is protected within this context >> 2461 | x->_copyto_reset_helper (); >> >> As usual, my C++ knowledge is limited, but I guess the issue is that we're >> calling _copyto_reset_helper() on behalf of x. As an experiment, I removed >> 'x->', and the error message went away. I don't fully understand this. Do >> you see an easy way around it, or should I just leave _copyto_reset_helper() >> public? > > There's no easy way as such. I think the right approach is to turn the > copy-to method into a copy-from method, modifying "this", rather than > another object given as parameter. This is a more object-oriented > approach, IMHO. This also automagically fixes the problem that a > protected method can't be called in this context. > > Here's a patch fixing it this way. Can you please review it and see > if I missed something? > > Actually, on second thought, this should be split into two patches, one > removing the call to path_conv::operator<<, and the other one > reshuffling the code. If the patch is ok, I'll do that before pushing > it. LGTM. I agree that this is a better approach. My only other suggestion is that you consider removing path_conv::reset_conv_handle and replacing its two uses by close_conv_handle. Again this is to avoid the appearance of a handle leak, even though I'm pretty sure that the handle is always NULL when this is called. Ken