From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.134]) by sourceware.org (Postfix) with ESMTPS id 63114386103A for ; Tue, 9 Feb 2021 09:47:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 63114386103A Received: from calimero.vinschen.de ([24.134.7.25]) by mrelayeu.kundenserver.de (mreue009 [212.227.15.167]) with ESMTPSA (Nemesis) id 1MC2sH-1l4Dbi3K85-00CV0x for ; Tue, 09 Feb 2021 10:47:23 +0100 Received: by calimero.vinschen.de (Postfix, from userid 500) id 24893A8093F; Tue, 9 Feb 2021 10:47:23 +0100 (CET) Date: Tue, 9 Feb 2021 10:47:23 +0100 From: Corinna Vinschen To: cygwin-developers@cygwin.com Subject: Re: Potential handle leaks in dup_worker Message-ID: <20210209094723.GQ4251@calimero.vinschen.de> Reply-To: cygwin-developers@cygwin.com Mail-Followup-To: cygwin-developers@cygwin.com References: <2fef1107-005d-9a44-fd4a-79fa5904d436@cornell.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <2fef1107-005d-9a44-fd4a-79fa5904d436@cornell.edu> X-Provags-ID: V03:K1:LSwr3FlogLMNSgbtjVeZF9rzuzAqrT2NWVgNJc2ZrbJ32Kfl3P9 QHuZBvXUn28sW7zEE+WWkNEM+2p8vQH/OpW4iXHcFm6byy0Fh2/1ceK/9avaPhMINVGMbQs S5rJGPEkaTKhbob6aTIyHi9wVImHgmTOkWT9WrEoEtgERli5CIx045Vp+XQe742Bu6xMiKY 2Dv4O1+Q9GZpfof6vi3sg== X-UI-Out-Filterresults: notjunk:1;V03:K0:D9g2MswUH2Y=:++d4GCFz0U39anFzzUDgbo MQc39bQQKz9j9rLQtIU9hwlE+SG1TEoU6TVHI1vy8Y6voZZJZ6blx99bnXI8orsQNsx31dInd Fb9uRBKia8/h/GpmBAUCBgJkxjdn8N9Z5BFAOCRBVvsHF8aaPCRfvAERvoOTN30ydc2ATKYwe 6vP+VlEanduR6aBAnhW/McrXvJbNU2IwtXMx/EJtN7f39v7IefqKa3wltMhNy2XhUreXU/T2K wpfkr4emIHDu1bQ2aExvRbl8fTxzsibnLT4+snRAU+DExTGnFpgGYUGUTarvzn2tM0lYTcEk2 KiZt6cqbHNkoetw0D1SDUGAF9DqgkcizzTFPXKOVi+aMzY4cWPECmE0EXPWJ3sHJqB4gXhqEp O2syg272tlGCNfjgcSjSf+WNoGJNGwhgNRbr3Komk+F7XKGnje0m4mOlXwuwIqD1BwjENpsw8 8jN1glWtVQ== X-Spam-Status: No, score=-107.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, 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 09:47:30 -0000 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: diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc index 52a020f07d5e..58e993b66c42 100644 --- a/winsup/cygwin/syscalls.cc +++ b/winsup/cygwin/syscalls.cc @@ -1475,6 +1475,10 @@ open (const char *unix_path, int flags, ...) int opt = PC_OPEN | PC_SYM_NOFOLLOW_PROCFD; opt |= (flags & (O_NOFOLLOW | O_EXCL)) ? PC_SYM_NOFOLLOW : PC_SYM_FOLLOW; + + if (flags & O_NOATIME) + opt |= PC_KEEP_HANDLE; + /* This is a temporary kludge until all utilities can catch up with a change in behavior that implements linux functionality: opening a tty should not automatically cause it to become the And then create an STC like this: #define _GNU_SOURCE 1 #include #include #include int main (int argc, char **argv) { int fd, fd2; fd = open (argv[1], O_RDONLY | O_NOATIME); dup (fd); } > 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? Thanks, Corinna