From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de [217.72.192.73]) by sourceware.org (Postfix) with ESMTPS id 502373851C35 for ; Wed, 10 Feb 2021 09:52:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 502373851C35 Received: from calimero.vinschen.de ([24.134.7.25]) by mrelayeu.kundenserver.de (mreue107 [212.227.15.183]) with ESMTPSA (Nemesis) id 1MKszj-1lUSMR461N-00LDln for ; Wed, 10 Feb 2021 10:52:51 +0100 Received: by calimero.vinschen.de (Postfix, from userid 500) id 59171A80994; Wed, 10 Feb 2021 10:52:50 +0100 (CET) Date: Wed, 10 Feb 2021 10:52:50 +0100 From: Corinna Vinschen To: cygwin-developers@cygwin.com Subject: Re: Potential handle leaks in dup_worker Message-ID: <20210210095250.GZ4251@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> <20210209161223.GX4251@calimero.vinschen.de> <16299cda-4d87-cd94-d4a6-6677438fd4d6@cornell.edu> <0745b429-d297-c424-8c60-540ef9593a37@cornell.edu> <20210209205257.GY4251@calimero.vinschen.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Provags-ID: V03:K1:FHD7K6lZnLBj7PrJ6OpBB9Ym1Wu0hg37YUtjYW8g6wcOyPqd6aq Z1kZRbXe//EUIblpPvsxUvisq/0rsPkRiv+GRGMbSJyra6pH2AxShX5pvuth+PYaPFu0Uod 5D6SA/xfSG+K2o10N3Exzwzw4eObAVOUicmF4LH0vgBqHVExJCNWeXjIjedR1+FW8nSyXiJ MHw/k8Rh780ZWnazKxWAQ== X-UI-Out-Filterresults: notjunk:1;V03:K0:KMldzrH/9D4=:aBeUk6MNeOv3qgzblqIbwR qN1eRW1rtBKYnJFif3ALHaUDEM4fRlwxKleahWOrGG33s8IKbpeQ4V3dwOj6S92eWViZ3pkBU yOlNgX53TaSU6QinGm5sYsu6OxoLc6xVuQA9Eq89YYZDMyMDrRMR3wQjJfSqpSpFsRSgj091J tL20Aa853jxpFz1xl1ZfAwRmCpIdhVAQy+l5gun1vTUksPC/zWETwtOsiMzJBJlIMn4zapVEM GwsITezNN0H9nREZcUsrvS1J5TCtgj8hkvv8LyCssccc7srYfUNkg4/xY9S+PHCYM+hTlXD+8 n8XO1Qh+gv3G6GMrYPL5jJ9U6pmp49gkzMYrERY9ooc7zhtd7/N7Ut1fYvQN/nTjvt4oEcCZY ujMDnGH/nmPEEn+tq2pmXjdFbpnZFjAzfo/np3+RsSZyqsb93Uzemt1Met6Lf1BeLqI3Hh6e8 CAKQlhowMA== 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: Wed, 10 Feb 2021 09:52:53 -0000 On Feb 9 17:31, Ken Brown via Cygwin-developers wrote: > On 2/9/2021 3:52 PM, Corinna Vinschen via Cygwin-developers wrote: > > On Feb 9 14:12, Ken Brown via Cygwin-developers wrote: > > > Trying to make _copyto_reset_helper() protected leads to errors like this: > > > > > > In file included from ../../../../newlib-cygwin/winsup/cygwin/spawn.cc:22: > > > ../../../../newlib-cygwin/winsup/cygwin/fhandler.h: In member function > > > ‘virtual void fhandler_pty_master::copyto(fhandler_base*)’: > > > ../../../../newlib-cygwin/winsup/cygwin/fhandler.h:2461:30: error: ‘void > > > fhandler_base::_copyto_reset_helper()’ is protected within this context > > > 2461 | x->_copyto_reset_helper (); > > > > > > As usual, my C++ knowledge is limited, but I guess the issue is that we're > > > calling _copyto_reset_helper() on behalf of x. As an experiment, I removed > > > 'x->', and the error message went away. I don't fully understand this. Do > > > you see an easy way around it, or should I just leave _copyto_reset_helper() > > > public? > > > > There's no easy way as such. I think the right approach is to turn the > > copy-to method into a copy-from method, modifying "this", rather than > > another object given as parameter. This is a more object-oriented > > approach, IMHO. This also automagically fixes the problem that a > > protected method can't be called in this context. > > > > Here's a patch fixing it this way. Can you please review it and see > > if I missed something? > > > > Actually, on second thought, this should be split into two patches, one > > removing the call to path_conv::operator<<, and the other one > > reshuffling the code. If the patch is ok, I'll do that before pushing > > it. > > LGTM. I agree that this is a better approach. My only other suggestion is > that you consider removing path_conv::reset_conv_handle and replacing its > two uses by close_conv_handle. Again this is to avoid the appearance of a > handle leak, even though I'm pretty sure that the handle is always NULL when > this is called. Good idea. Done. Thanks, Corinna