From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM10-BN7-obe.outbound.protection.outlook.com (mail-bn7nam10on2092.outbound.protection.outlook.com [40.107.92.92]) by sourceware.org (Postfix) with ESMTPS id A25903835417 for ; Tue, 9 Feb 2021 17:13:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A25903835417 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=BA9FkFFxrpGVchzlDygih9pmTj/vAoi7UEx8lapuGplTeWkHJCt+Em75EdbXJEUl+dtRijZucEfuraGJ1Q0S9P557IQP+OK1nJMk0WHJebjZvLqZJ7PVWNe8gQ3xpNRugEicx1WrluET4eVXJpfap5g0Apn5tZ0ANM4zR+24/B/+HZ7DWKRnKFbdVrd5EQAFu94iw5YZnv75/WyiTqjOQQ3ie50W5X4TGXD0LTOn1v7C3wdu4a+YVZlomKyNKOhR21s2cT1Wc3eXjWSZvMy6+/ZpBWBNpM/WB7LJ93RsdsL0OsPJ7nfjA6KeVRwDaVRnzO+ZHxV9+gZMreE3e9wVLw== 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=E7q4TtrCyFR1eEQajRSGVyWwWLxD9tljgcivad7fSTA=; b=iuNglL90Mbx9vWm05MKbrFeajX9c3DcAMKPV0JbAkKLjJoBGCn3NPv7aJQsu0H1JpqKrdkRefHSkIKu3jYrKGfnnkLbg2uCLBwHmz5nl28fFRxSE0CGNxbA60JQTjiUnzSTEO8iNIC1yUF0w+iNBj2kggAyoxDwBQGf6QkPg8pznd65YepvmiMZLFhZaLdAjmZe2EjZzr4YOmm2xGSmU1o8XbqHeBzPr+PY598yBAFpQEJPYuAK0XOZZHGhszgqBewMWUVu9zpYKFNPfTqpBTKKmkloJqwR9UXURv5ykozI9jYHHVNYaAxZuqQP6+FxGfm4z8xeWJ7+e6sQSFzFrwA== 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 BN8PR04MB6370.namprd04.prod.outlook.com (2603:10b6:408:d5::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3846.25; Tue, 9 Feb 2021 17:13:35 +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 17:13:34 +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> From: Ken Brown Message-ID: <16299cda-4d87-cd94-d4a6-6677438fd4d6@cornell.edu> Date: Tue, 9 Feb 2021 12:13:33 -0500 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 In-Reply-To: <20210209161223.GX4251@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: BN6PR1701CA0003.namprd17.prod.outlook.com (2603:10b6:405:15::13) 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 BN6PR1701CA0003.namprd17.prod.outlook.com (2603:10b6:405:15::13) 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 17:13:34 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: a6896a5e-7c22-48e6-ac11-08d8cd1e085a X-MS-TrafficTypeDiagnostic: BN8PR04MB6370: 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: eQEQhFGEm/Va5XbmZyLzpp7JrqqTsHP12nvLsG7Jo0sW5xRYMPunIc71MPgN8GbjKIDlaWSHOwjieU2N6Xcm8oMeer32TmcG/hgc/GQA2WuelCoiSAgMvFTCh2ID2kl+b1OEj2obhzQKoiFwLJbOgsvHBX817hl7FizV7kcx3IHUy2NNeNeYFWb98esYeCffrSJijK9n0UTPQqu7LSmLS9XnG+lJx4fxQlY2hS5QMb6Le20ejSHTauRZK9UH1+oDW9MEnrAJ+LHQW86IUgvDmkxqePiNWs/HHeZFqNbgoLQ+I+6T/z4HwXhFky4In9+sVQ7oxwH+RUDqYdfFYBNJq5wlufHCVLp9mCzj4GoBBU9+sIhmTspS9oFJJdTv+d3Eo9Yc6BtM9+7cOGfPEeDLGHC67JWns0wIH5+D+yncKHBegJ8xQZIoL7tPAiLofJnDpwzwSpGkKOz+4ZTeOojiC3lZuoPm4OcYu+g6WQxA4xHHlC43D53+zPeFoZne2Yz3eclNUNTAbP0PRTFWjXv17EqsgPPyx/03rq5bGT62E9VtrN9dWrdyZ8SR7MghAGCu+wH55icsw5zvXXdNse7xFvGwXPCvpc+6Za3IMBzrALA= 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)(39860400002)(346002)(376002)(396003)(366004)(6916009)(53546011)(2616005)(31696002)(86362001)(956004)(16526019)(186003)(26005)(31686004)(36756003)(8676002)(83380400001)(6486002)(786003)(316002)(16576012)(478600001)(5660300002)(2906002)(52116002)(66476007)(66556008)(8936002)(66946007)(75432002)(45980500001)(43740500002); DIR:OUT; SFP:1102; X-MS-Exchange-AntiSpam-MessageData: =?Windows-1252?Q?Abt5SOsECixxO70OSR9UVzCnY4dFbBkMxhsIrEzA0TLwITIFyqpz9sQG?= =?Windows-1252?Q?m8pO4L/FU+lK6ODlZdhVKRoor0MHHBiscPXYjue8wI6HWyrDfcDO0FHy?= =?Windows-1252?Q?neKROGuN/QEcLAcmi1eirGmI+MkCUA5EoNpX4h3Fl4rWqgovv7zxjIon?= =?Windows-1252?Q?4rpPd8y+/EJ3BSKzKTWyPURWlVDmkj98LuDiaTCQHuhj9/IxI8cbyWzs?= =?Windows-1252?Q?3lK4gRJpB/viYvoGae02dSK+g44RUtDljBfC7blWevCe8P0kHAyr6L5C?= =?Windows-1252?Q?kx/B8/KaTnUQ0UB350iod5Q2Kdel/o3fH9NCAafaLKsJ1918IGwDiyqe?= =?Windows-1252?Q?0YYEdnce2iApvO7/7XYGujMaaotN2tyJTyDFqckVt4JdKh0C6EgZnJvA?= =?Windows-1252?Q?2mhn4vQoerpUn9GllQQufZJBxeBtO6YHrJwh2XAN1ilzxrj6X9oJf/zC?= =?Windows-1252?Q?TCqgjSCPaJuxYiEuHglH8H2yz6XlZtNzySxuQFcJNRCxzpt7AVFMKQqK?= =?Windows-1252?Q?BsYx5DOkEI48a7gL2TO71U2owmvxUkD/dYjtNwT7zMuqry+j7RvEfBvb?= =?Windows-1252?Q?ijecE209QKYgv+NgLNO9mpOcDbqFJYH+hjcxwM/BGk+PbpouQu9QkMmL?= =?Windows-1252?Q?5qNKYb1FcsTxl0IawByAAp79/FIO22sepjOc7S1dV9RztAxazKUi8uGj?= =?Windows-1252?Q?+7JXulUSzDr8oIoqwtZLE8cZqieCxTKRh3AygUResDK1idHYe8UjPzmO?= =?Windows-1252?Q?QiVtHtaMu+bC1ekCH7OPJpExrP5bWQRWVbBHrHKi97qMtX6poISjZojW?= =?Windows-1252?Q?X6yJfbF0Hj6HOBIOYLsWJsJPCTlgrmgcDEqyl5vVhy01AELoRexl5ech?= =?Windows-1252?Q?3BqgPQxz3pxtixwL4A4Pow6uLF8+X96r+cyywXxVDgEW7SLZ9f9kH3/y?= =?Windows-1252?Q?oi9GN/iilQyOvLgIk7CRlxw2wvcfE8UP67PLojfqJHNbOuCib3ebwR+X?= =?Windows-1252?Q?aDlwxz0FSPW0Sanb1epb0vVr0BZTAelFZgWWSBd77CAo9429bLKlihy0?= =?Windows-1252?Q?13DtK/d78ts84DD8T+MuiQcPFD4JWKFv8t8B0WongZiLezF5V3mSre+G?= =?Windows-1252?Q?M/Fqwm703TL8Ex/xY8CueJHD4pQAhzULUZVNCF19ZyzCTkNeTbChshkk?= =?Windows-1252?Q?nwlJofpDZE5IZKD3erKa1WDmGOORKlhgL+ZnKLKBwm2b4qoqt21DtYd9?= =?Windows-1252?Q?G81kN7ayUngAGCs0Nf5H2GjNhJCh53xfUCDPRlEbPz3wvHAT2vJsoCK6?= =?Windows-1252?Q?N6gdMyiBsiWryyaHXiJlHgthoYzYK+PHNKB1rTDHAnk44zeA0q960FvP?= =?Windows-1252?Q?r7a9fR0mJdCi4UuLR7xu4EckCg9J5hmoB/TwSz+sO9tpWq8D6GcB8+bT?= X-OriginatorOrg: cornell.edu X-MS-Exchange-CrossTenant-Network-Message-Id: a6896a5e-7c22-48e6-ac11-08d8cd1e085a X-MS-Exchange-CrossTenant-AuthSource: BN7PR04MB4388.namprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2021 17:13:34.8476 (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: n3J3MTO13GPGdDGvUpRY6gK0cT4DNChX6o2PNr42jGH7uhrdRV6xh73+TfPM7mJWP8d8jYXK/BI4P0P6PZTd6g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR04MB6370 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 17:13:38 -0000 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. Ken