From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de [217.72.192.75]) by sourceware.org (Postfix) with ESMTPS id 10F7C3874C38 for ; Tue, 9 Feb 2021 16:12:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 10F7C3874C38 Received: from calimero.vinschen.de ([24.134.7.25]) by mrelayeu.kundenserver.de (mreue108 [212.227.15.183]) with ESMTPSA (Nemesis) id 1MJEEf-1lT74m2xpL-00KhBU for ; Tue, 09 Feb 2021 17:12:23 +0100 Received: by calimero.vinschen.de (Postfix, from userid 500) id 4DB45A806FF; Tue, 9 Feb 2021 17:12:23 +0100 (CET) Date: Tue, 9 Feb 2021 17:12:23 +0100 From: Corinna Vinschen To: cygwin-developers@cygwin.com Subject: Re: Potential handle leaks in dup_worker Message-ID: <20210209161223.GX4251@calimero.vinschen.de> Reply-To: cygwin-developers@cygwin.com Mail-Followup-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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <4b8c6b7a-07af-2080-e105-2f22374e5bd8@cornell.edu> X-Provags-ID: V03:K1:p6R7vPFj31brxPU3jx+dD3pBWkwBoZic5DuPGpMwm3jjvRxYHMd vRO0VS9Mm8NKava8S0Llq6/YrWCrE+enGUC4NKUK5KyM4BqkjwRqEbS66Hsu3rGzPxlHojf X4LxIPS9ugYlOuDoGkVeCsCCTBHDpXr7QuInPkSId4IHYYkicvd7SyXpZiJT3EDCswS9zdZ uDE2ALdDQ5EftaZuSNK4w== X-UI-Out-Filterresults: notjunk:1;V03:K0:UWt/yYB7tw8=:9/8oi3qN4ntdHTbpZdbnTb UqS4UZH6L85TqMAtGrhssa81MKcbybaxfKFHJ2+wSVuGCOp5TwKNdyGnfTQvPhj84+W4MbJI4 D0DkL3eqBYxe+gp4yO73wSODv7pr3sUhIstpla41sbY8v1mOTAPMflqoJSRqNkV8e5lyn/6kU akSDM3e3+v4N3uX/5lZNTngmAkma+Hr0RCtqxs7gZv9JL3xCvU55DMMqOKuMtwMW+VwN18Cyn NCaRY9gOwUNukvUedoN0sq3dvhqWStEp/+GnMBr9w0OWhGbeD0j5+w5bxIpkibAuO9DQ3mfFq b2C3l1MFdGEVPp1A0eWGtdTx/Uur/EZWVPCm+ELc0+Mt/0Nu3hgxjmWOHVK/BfTMMROhjsbe7 Ka5qn1ET6ZFqY0NBb7efrv2/cLIBZvN+/vRK7CsweYpmuqr4/nKkQuR5afhpVMZqin1eoof5z clBfDiqxxg== X-Spam-Status: No, score=-101.3 required=5.0 tests=BAYES_00, GOOD_FROM_CORINNA_CYGWIN, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NEUTRAL, 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 16:12:26 -0000 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? Thanks, Corinna