From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.17.13]) by sourceware.org (Postfix) with ESMTPS id 1F9213835431 for ; Tue, 9 Feb 2021 15:04:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1F9213835431 Received: from calimero.vinschen.de ([24.134.7.25]) by mrelayeu.kundenserver.de (mreue108 [212.227.15.183]) with ESMTPSA (Nemesis) id 1MlwFp-1lZRmJ3JlD-00j0GH for ; Tue, 09 Feb 2021 16:04:20 +0100 Received: by calimero.vinschen.de (Postfix, from userid 500) id CE4EEA806FF; Tue, 9 Feb 2021 16:04:19 +0100 (CET) Date: Tue, 9 Feb 2021 16:04:19 +0100 From: Corinna Vinschen To: cygwin-developers@cygwin.com Subject: Re: Potential handle leaks in dup_worker Message-ID: <20210209150419.GU4251@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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210209150249.GT4251@calimero.vinschen.de> X-Provags-ID: V03:K1:k2tXcFayE2f3HpPfj1a0O/Bjn1vhSV7NUVbFWbFppTDgtQdwgFx 45c9GCcU77e3Ikw3SdQ59826nc1WwLkPXMJxEabdzBxgIsAy56yNUyHbPyhtAOEGfBBIedK 06bN/1sNIVtP/W7L51FbXc7ujokS+EayseYnoXBXzIzPnR1zbX+MSoMIH1kJmG+Itvf+XiG e8ID9SWbXHMQLS+fGroHA== X-UI-Out-Filterresults: notjunk:1;V03:K0:7YRthO0IsvE=:q/LDLn3Mt6Fz471uBQ7Ync N1rGL22Kw1PVlvx1OMaq9c7VBZOa8PmLLqc2HFgkHxkKcxKF+gjq4QphrKS5IQk2tJzPww7FB 1el0f8SHWUBAD7FMp8EV/QTdBKLXbV6eSTE+CtBGfQZh3S4CbhIKILuBimwTT4gurHlgQmXx+ MeYkkAfEQCFDoeDb4GSWFHf4zyudFeWYYPcmo7qdjU07jnrLeGbFN8GMyQMY26C/fAgpkHAlb uQx6PQQQ80HhZWBVHVG/MVs/yI4smNMB2G3KX4Dj2HrdLQZaOGXN3KQGYV2LCJWHf6LQef1bO b/4QqMMF4v0ep6EVdcJF0WoU+POLNvAlnS4WswlfI4QXVILcq3P1Aks4HbWXjPHac2f1Myvkj msAA9ADc8ql3r6OG/wwt1JfgTzDccWx/1UOkZ80vjFBM3BDUm3XOSrIsBVBxgfO5dsDeyWZAj 8YxYnadf5g== X-Spam-Status: No, score=-101.0 required=5.0 tests=BAYES_00, GOOD_FROM_CORINNA_CYGWIN, JMQ_SPF_NEUTRAL, 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:04:23 -0000 On Feb 9 16:02, 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. ...and maybe we should just convert the remaining reset method to an inline method then... Corinna