From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM10-DM6-obe.outbound.protection.outlook.com (mail-dm6nam10on2090.outbound.protection.outlook.com [40.107.93.90]) by sourceware.org (Postfix) with ESMTPS id 9BEAF38708C8 for ; Tue, 9 Feb 2021 15:31:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9BEAF38708C8 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=OVqDjhQUpMNeqX7Q93ojnTjk/oNjXMD0pOtupgNbOym3wuldycRCUr6jXRRlO9U/Hc3lHyHILdzkID77g0GxJPbT2Vu7F+OWV6gHot6HNokckG9oF8ctFaxqB/OmUMmEoUsqTwUin7Huvrz8YYKp/gvOGVoEFElAswrj1haej+ohl0B5J7yO7ExDmjNnKz2fqbA+fo6u4kyzdW9FqwDcn0H/kscI9NK9mC1/v+oDfgYTJaluLmt29n0vF8bZyy2FcMpKtapZWwqfix+Fe+0TxZNZhO4Cy8PsXXNEz8gdxBLboM3QUIGLnZiKn8CuUzZQQIFGzHCGqK3Zm94h2tYV+A== 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=XrkxVA+qqX0jlaAKan4vDHPpb4s1/xez7kT9r0k0EUE=; b=Cd88SRU5YN2JdemXcZBWcvfozHSjT2PDM/w42+Yq71IL+hXRvhxfEZqDh/zGqA3/k5n9QmWtxfGZuWaz5MvwnJwa/Nn1ccz+SBHUU8RgWIcTdzjAgsR2x+f8RY2HMIX/LxXgoctL8wC0lqDQ5XyxUacvX5pS614cPG37kI/y3mtlt78tH2+V8Ai5dwZUH5Mrpj4SiTqsXZlPxcH7rAeD5zypoMiHPlpdtuCkW7FD7tLa/6pa+1Q2mS67icRfeLJgrI9C7s4bkKDMntpRotg/fYZ2uWUlEmr5bS5If7rVMqsmqgwkGBNWAxrNz2iQmG6kO8UXhXgBbNTFHBVNrkzVFg== 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 BN8PR04MB6385.namprd04.prod.outlook.com (2603:10b6:408:d6::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3846.26; Tue, 9 Feb 2021 15: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 15: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> From: Ken Brown Message-ID: <4b8c6b7a-07af-2080-e105-2f22374e5bd8@cornell.edu> Date: Tue, 9 Feb 2021 10: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: <20210209150249.GT4251@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: BN9PR03CA0336.namprd03.prod.outlook.com (2603:10b6:408:f6::11) 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 BN9PR03CA0336.namprd03.prod.outlook.com (2603:10b6:408:f6::11) 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 15:31:17 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 6b60bd4e-a475-4b45-5a5a-08d8cd0fbe47 X-MS-TrafficTypeDiagnostic: BN8PR04MB6385: X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:7691; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: ab+pPSwmPC9JKc4ArL4TQdDODVHsroYvJcoE7V/G1F017V3ofLEjdjHjr/36FpwzQzQskCf05gCwzjUGYD13DeFEJ1x8N+56bVpdvB9D8Zorvu1wSF91ellFpFnysBFyrWSgHLZbfF+ipG/nVmvXlLhKXx2xSCstrnf4qMqrDFxU9EViH52mcAiF8aDEZdcQRwIjrOrd6a+p8aDFJG8CshL2+vtYlLKRiYdm4f/+EQHZO0VuB93bSzFnhXZKoBuNE7dY+4bRkb7modLVlWsGUiqdxABVw2SILabJYVJscR28wpfL2aDDJGjsQeXONXpCr01YWnaZ1Zu9uYXCvR0+D42r8jmKateWa4cMiP2oQMrdrA5GrKS5m/uqB6HJGUyart409NEBV8DVsrkkOc9WZF7yJ3q8CrPjzmmhSUPmiEOmxOofiPb7u4jKjlG3tWKWwG2LuzVQJDE/7SdN/hwbTFvyMDaFzdmU73+jPsMDDFOmdVCHlFkokG1L0n2Do+ImOl7ch6fuWOFbePtSDkPQI5Q2MmqmldnAXRPOYjBf22o1+OOk/vB/wJVWKSFfW8pCzJD5E8XvyX7ORfO/3JFz6nqV6oA9TsygMW5xevyO+TY= 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)(396003)(366004)(39860400002)(136003)(346002)(376002)(31686004)(6486002)(5660300002)(16576012)(2616005)(53546011)(8936002)(86362001)(16526019)(83380400001)(186003)(26005)(52116002)(2906002)(316002)(786003)(478600001)(956004)(36756003)(8676002)(66946007)(66476007)(66556008)(31696002)(75432002)(6916009)(45980500001)(43740500002); DIR:OUT; SFP:1102; X-MS-Exchange-AntiSpam-MessageData: =?Windows-1252?Q?dcEUoK1CwT+2YRsRWGjySFAKjG/gihhhKJyDk77EZIARrM5Tp18pdITs?= =?Windows-1252?Q?J+bC7bFhjPkQptyHjLNuKyUNPL5yW93VOgrzz2yUuUh1nK+LRD67ckTa?= =?Windows-1252?Q?31sei609md+lqnrkFi6lf+TQrhGJVBh1hTWATpXjfuFaNcbTqz7xT5SE?= =?Windows-1252?Q?HV57Zff8KwVZjf7NzLsV+bbfbMhdt+xG4q+82admO1ptqw5OGFXeSwv1?= =?Windows-1252?Q?9vuHzhyItTVl1FPjguw/JvYlJXH33MNPRIj+yjlpY/M9sgUeGEW6Qm3Q?= =?Windows-1252?Q?nVr04kOcNAiuWCGoJp/9m4g32KCO50TpIBx5e0L4DZwBLlwr685art1m?= =?Windows-1252?Q?6ugFB7drA8mgz55CC6VnfF7f1rhb5lxc+MIJjX6zN71OFDuTlceB+d/P?= =?Windows-1252?Q?IH4Oa/ESddUGXuj0XE7UMovKLmkWHlTMH0o4B2Sg5rpGPcvC10X+bnsE?= =?Windows-1252?Q?NXqrFqWOMkv6Atrs/2gsqE7gLNlj8L8Eo9WTGPXA+54WjkMKnT3G77xp?= =?Windows-1252?Q?HJo92R+wnrRMcPErOQOoWi+K6jtXNzZHKHilUq8UignjtxHlG4el26+F?= =?Windows-1252?Q?GpqYFHMauO9mhF6nz0KIx9aBnuSbI5E8Q0yYyDKXCg1xwFrPEgNT0nZy?= =?Windows-1252?Q?oncOf/U8KLnXJrg2NA4ahBvYRSheKGTgGNelEGwML/aTTHLZJ6AZxTfE?= =?Windows-1252?Q?c4LalVnSN7mO7uEzpcUtTiX05W/zyenKwjm7DNAA3Os5wdYHow3Pk1yj?= =?Windows-1252?Q?ko+EH31ogLhNNjxOrvzzBmJKHmacuvrw1SE8e1kD76t520wiOFrZdrC4?= =?Windows-1252?Q?XKUUbuZg3EVz7m2Sd67RGZR7c4ruB2qPjmTYWE8glWm5PHCPhe6Qcctq?= =?Windows-1252?Q?jOTwdoJpU0Vx3PJm4mO0+MYSIbefJDc4nnLgdKx4udsEnIaP00L5PAIH?= =?Windows-1252?Q?kaZW8s9sYG1TeCcYNJOm/KjouawiWbEopHMsC366FBXfLm52+/fmTHwi?= =?Windows-1252?Q?073KKoUhkOOyEE7CDULcpStj39VAPXAvLNPZpujQ+DBima/FaPH9IMC/?= =?Windows-1252?Q?IAr8Z2viy0woVOFrc+uxhHxBFh5ha9act7KnNAJc8Wf1TI7lsv3z05Iy?= =?Windows-1252?Q?FZtJFitZUr/1CzdlrQanJfnf9sySZs2rkW24GmX27nkkT1DujvnviYxG?= =?Windows-1252?Q?Yp+TlNSwOrCah3WbVlnV7jxfgS5ZM8lkb1Yen9dL3A6VUqBZVsZnneAQ?= =?Windows-1252?Q?xeGriq2d/Bf+w7JbRHNoxbEpZ0QaLG58O649prOSKWfOP239lvdPrPsL?= =?Windows-1252?Q?tsu0g34eKQQcJDcWqlXWtDujdrHOVTEkHLkBFMbpZ86AZA7HCJdwfpfj?= =?Windows-1252?Q?hKMqXE3umrBZK+dac61qYpuAO1LkrOSkiapsNGDLfsViU0AhlyoLwH+j?= X-OriginatorOrg: cornell.edu X-MS-Exchange-CrossTenant-Network-Message-Id: 6b60bd4e-a475-4b45-5a5a-08d8cd0fbe47 X-MS-Exchange-CrossTenant-AuthSource: BN7PR04MB4388.namprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2021 15:31:17.5015 (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: f2BFF3Psh/loeczK/rBM6/ENsLuPLYhpDP5NNPlii6BqnlyC0PnDIgTm95PpTra8t8zi5indLqlf4/U8f5hqvQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR04MB6385 X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, 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 15:31:21 -0000 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. 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: >>> [...] >>>> 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. > > 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=. Ken