From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM11-CO1-obe.outbound.protection.outlook.com (mail-co1nam11on2098.outbound.protection.outlook.com [40.107.220.98]) by sourceware.org (Postfix) with ESMTPS id 0A4F2387086B for ; Tue, 9 Feb 2021 19:12:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0A4F2387086B ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=CnY09KY4hKCkytZLtZuTSF76hC+gyjLnkISDcRFQ6A7KFR83s/Fdr+1sWPPi/wCDwzJvenYPoTHVOPfPNcWF2SchKkX7Mha/Shv/uMD+QVB8pPUGcZf+gVOlYX2AJA33PJMjZT3Uu5A/V2OxDGSY7EdphzLhX88HXSaEfPpsu+ZM4ROjI4POA6GoOvtxQcGyr5Gyn3m6YDPWWJl+Yak/FtLtQeZW3ySlDeNaGV+oe6lLZbEzG9/TPDigfn7MWMgLTrzgwk+3jg+ZXp+qUgV9aeterbbxc4OnzntFzoIXPTKKIm3Nif+GFzQnNhgFlT8g7xVEUyBmnS2Xz9kP5nYQUA== 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=5XypbvaLwO3ggsko3ec9YMr41FV1SrmGoz2daf9uHEA=; b=PpVl/DFeFGRRLjMKaM8YLO+7YUEp0r4kMJE5yZ9xBAItIIU196rXjfVkiwyM3Kyq/Ohh8y34xvayLbmB5NR8XtW7lNFOOY+6vjLCts3C+JXx3foCLaSUEoDSEpyIEbMEePdFiSmSXngg6Og7NOLVQG7j6oc+4ta68CIfvD975rjArHssMTAA7rPGu9/zJwybL+9rpuI7XbVCsPotmQbxgtDfUXAplAWRFY/VvZEzyrpM8ilPZn1i4TEsQYlzxv2alOdddfnLMPiqv90dcKCxJGWh1awOmCNjxZIdQsMN5j7OTqQFVWwnx3rx9AcokqtCTTbDh6eNfT8bndv8gC0PKA== 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 BN8PR04MB5475.namprd04.prod.outlook.com (2603:10b6:408:50::28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3825.30; Tue, 9 Feb 2021 19:12:12 +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 19:12:12 +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> From: Ken Brown Message-ID: <0745b429-d297-c424-8c60-540ef9593a37@cornell.edu> Date: Tue, 9 Feb 2021 14:12:03 -0500 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 In-Reply-To: <16299cda-4d87-cd94-d4a6-6677438fd4d6@cornell.edu> 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: BN9PR03CA1039.namprd03.prod.outlook.com (2603:10b6:408:10b::24) 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 BN9PR03CA1039.namprd03.prod.outlook.com (2603:10b6:408:10b::24) 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 19:12:12 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 7aa805ba-3733-4327-33d5-08d8cd2e9ad1 X-MS-TrafficTypeDiagnostic: BN8PR04MB5475: X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:6108; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: FQezsZLa7kVENOU7ibnRzpWN9BYxMXnuo1KzyrYPHiBqcNhfImYZDQKmoRHzGIBtGHJiPniASw+UQyumzS5zo/JIRSD78YoAy31EivShM4PtwCtmlLOa+PRD5PQ7I9jBHmpUanOSw420UPApn89I1Az/JacGNCo9YS5GN+DW+IhE7iqmal9Z2B+jtSuCHXUvDH4Nmo2A7BdIuBtq0O9SAPQA+SRuNxnci0d3ydrnEvIfgwV/L6GUnjnyGwrt5lfEKy5UREC196TLJta7g6tdT22mQ8MH0pUFensw9Z35dZ9pqA1bqMnN3EL1wC7AhuvBBpHtiCKc93uPb1DjXfQ1Wcrp5pEMHhd/BrPZqf05iJfkK4cBG8c6FhYygzaq/pXCQqTw3J9R/BSw660A1oVaZXUlCUXJm4uvRnhC/zzNACj/6livq4Fu7BlN86wUzs7zZZbBaKYgsvQvFfezibEFmLOqKl1bYPItHDXYgWPiMPsR6HJrjxdOFjGZZi4hDNHburmf2sLrdxFLVt2kFLmOFRtnJD1DkdWNfXZWxCNlkVumTsgKuStyNjUeYAgx30mJqBTPhRwzPurJ3mHgpjFtU0E/kTO8eOMx1A9/+YO/aR4= 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)(376002)(39860400002)(366004)(346002)(136003)(8936002)(26005)(6666004)(786003)(66556008)(186003)(36756003)(83380400001)(53546011)(6916009)(16526019)(316002)(52116002)(478600001)(956004)(86362001)(75432002)(31696002)(8676002)(16576012)(31686004)(5660300002)(2616005)(66946007)(2906002)(6486002)(66476007)(43740500002)(45980500001); DIR:OUT; SFP:1102; X-MS-Exchange-AntiSpam-MessageData: =?Windows-1252?Q?pKAYTttpSDVavdKAuCH/vd/6SQsO15TrrnZaC+NwlR3z9TabLLAarvPG?= =?Windows-1252?Q?y0DpiJrG2js7rBRrXnaueRa4azvL5WDgNAKeCgtn6UUFwBoHETHqaERy?= =?Windows-1252?Q?aYhuxwc5VJyjxWJoCY3KYkBZiTEG3lhrFWK36QtWvTNW02PWuJ2FeVeb?= =?Windows-1252?Q?tkFiWRomEI+0SRca9sHEEUUcIIGVRUFLONks/5c3xL3Rp7LzoaYay81K?= =?Windows-1252?Q?oTvU9jJNvU9BYVP6ckYMFcgAGlPIDcBZZ7CZ9smYXu1uowXvTdDSkXPb?= =?Windows-1252?Q?OBvuveSNc8HtDp+W2TzyA6p7IXMymcCFhqKesY3MY9+ymzNoKfEcBRl8?= =?Windows-1252?Q?spZGVQotmEhDZFb/8S/kUegJWuYPjWs6DeYhSuDbyMUyYz1BgL+o1RaU?= =?Windows-1252?Q?FbDtFfjKaXq2lLu+M44NXghl5jrpE3O0V8dcdZh5ApRfRLbf2BJTpUao?= =?Windows-1252?Q?eFdM/XGVx+6PfKuV9+A5SmGtDDtM310iBqsl6fUVyvyIFpluCH9QyY7x?= =?Windows-1252?Q?O4RWu2i1q/kAr+K6Lkdt4Bsc00l3U6nvQHCJ73tRW4ENUi1+ZSJ/6iY1?= =?Windows-1252?Q?yq/hGDzmVCEdh6OYL9JZ2wqXql+E40+8NHGy/NIMCfcMzQpOWBkGUC/W?= =?Windows-1252?Q?ytQBo4X1WYt8d/Hg9P9YksY1CokibT1yN78pnCUiE06uVUAV2jhVLHbn?= =?Windows-1252?Q?5U7I2D0GREm8KErIgbTv0SPk8c5mc91aeWENzXxu59tgrUvlz69ym3BK?= =?Windows-1252?Q?eguHLPVt4GYnqiNDX+SreoxXT3k+FerRfFfomlt9IHAlNwug9uPFtCTc?= =?Windows-1252?Q?F4yysPCjyyVYGKf/LhDVQZL19Bp3M6+xeC3GKG3FqUppuL37gkPIqzjO?= =?Windows-1252?Q?e6G90P6r0NoR4ioP9DItNWYi346XsR04RiFcGi7PzHFr1366MgEHx7Ag?= =?Windows-1252?Q?BWs5coRA8mgnG70BIYVkcRiMwz4utgm1VYfzxWYQ4LAkI98qG23HX8gp?= =?Windows-1252?Q?W+H9Rr5Bxz2sb9Sec13tVolHt35EmMurVBbLbiI2o1uk1rQbHYxzpDLH?= =?Windows-1252?Q?I6LaLU8lB+hC2J8VP0HVYEets/q+9pzttH9ZbLgZ+OOMevYRK32/hatT?= =?Windows-1252?Q?SSxihmw0JCCJ0XMoMdkObKYuKqvfGuvsj/XfG55sGjEXwr2sp5Vl4QHA?= =?Windows-1252?Q?uNv6FljQK3J21w1icSQP/Eb0oUt0CgLa7wc2pBnNkYaQcjp1w9CEbcwY?= =?Windows-1252?Q?UknpTn9w7EnaFByf9q404u6D1nFm0H/AU5kUAuF7uYieCTywLiWAn9lQ?= =?Windows-1252?Q?AZ9NfnSMIBWTkycZbq7KQnsMv02GZTzmqeiqHnnIqk3HmU68IcyGCVhG?= =?Windows-1252?Q?DLjq40Qi33yTUT5m2GpEOwQUgTcwA+bR24IoeBgOa5F3Yhr+gkws/EPN?= X-OriginatorOrg: cornell.edu X-MS-Exchange-CrossTenant-Network-Message-Id: 7aa805ba-3733-4327-33d5-08d8cd2e9ad1 X-MS-Exchange-CrossTenant-AuthSource: BN7PR04MB4388.namprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2021 19:12:12.4405 (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: GfDgf5Y1Z9ZuQneVujYSUM49/QCQUe9u3gutFxLY450eOQWlY3m27jGWZtIDgcvM4OojXIkD9OWs/3qUVDcndw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR04MB5475 X-Spam-Status: No, score=-1.0 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, 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 19:12:16 -0000 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.  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=. >> >> 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(). >> >> Are you going to create the patch or shall I? > > I'll do it. 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? Ken