From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.135]) by sourceware.org (Postfix) with ESMTPS id 838453870914 for ; Tue, 9 Feb 2021 15:02:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 838453870914 Received: from calimero.vinschen.de ([24.134.7.25]) by mrelayeu.kundenserver.de (mreue012 [212.227.15.167]) with ESMTPSA (Nemesis) id 1N94FT-1lwcxB1Kyd-0165tR for ; Tue, 09 Feb 2021 16:02:50 +0100 Received: by calimero.vinschen.de (Postfix, from userid 500) id B5BC8A8093F; Tue, 9 Feb 2021 16:02:49 +0100 (CET) Date: Tue, 9 Feb 2021 16:02:49 +0100 From: Corinna Vinschen To: cygwin-developers@cygwin.com Subject: Re: Potential handle leaks in dup_worker Message-ID: <20210209150249.GT4251@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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <4b56fffd-5401-bd8a-0444-4b8b7a8da4f5@cornell.edu> X-Provags-ID: V03:K1:DCEWZVskUvlY+9PJcTF8yzvI4SFpCpMhaGiAgRgYRFED7gg5Guh 9chdlO1HCTawgNZ/nOSWuQtGNSlM5z7RU82hFAw+tQ0d6Bf3nZhuV+4y9yIu7hcHZbXsIQr FUR8/HGS3dhss2atRZU1N2fSWrloWojL3R/onFqnbCA1SPEYvp6mikvDmv5HEJed7S1ZWUL wODOA5J/G7t3xqeA7skuQ== X-UI-Out-Filterresults: notjunk:1;V03:K0:HED8eg97R80=:6P/bg7+F30KrtrMhosN9Nv WT36qzsAQtONFfEDR3VeCJ5Hd3pxTNY2iEbvLqMvnOLOHrKhmTljiWOX4niV1BAriYvq0IfWs 8Osx7DHX+2beOYr/rmtZ6VSdrIcBu2QjHw9tNeMVhDfDev+yTHBaLIRj1dG5b3erhFzbIBOc/ CvY9vKeqTI40jOpvGhHfed2VJg97srtzDXlay/VDq0uF/KK0l6seQYZ61jnAsX/ftlBD9oR/a jD714Vb+XiR/EfgsybdM8W2PyqgX3tDdwW6wcqqBdchQUdLTKv0VLumD6NkwWr4CJJRY3AYL5 wYjDqPob48GDFhJuiI7uJZNq78z1PxYHqMKBCdzWs2+Fu0m4f2nYuv9lLnDn9JQ42rm2Gii0D xObIFJ46xHq0ORJFmpdCV29OYDuUPCdklgha/XobR92yG3+J5rRZA9HyDu9Vm/lF2/EnFFaHb TP2ki3elcQ== 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 15:02:56 -0000 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. Corinna