From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.1 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id 9AAB52CCDE for ; Thu, 22 Aug 2024 16:49:06 +0200 (CEST) Received: (qmail 7844 invoked by uid 550); 22 Aug 2024 14:49:02 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 7799 invoked from network); 22 Aug 2024 14:49:01 -0000 Date: Thu, 22 Aug 2024 10:48:53 -0400 From: Rich Felker To: Markus Wichmann Cc: musl@lists.openwall.com Message-ID: <20240822144852.GA10433@brightrain.aerifal.cx> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] fcntl: Purpose of second DUPFD_CLOEXEC? On Thu, Aug 22, 2024 at 03:52:58PM +0200, Markus Wichmann wrote: > Hi all, > > I just stumbled upon the code to emulate F_DUPFD_CLOEXEC on old kernels. > At the moment, it looks like this: > > |int ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, arg); > |if (ret != -EINVAL) { > | if (ret >= 0) > | __syscall(SYS_fcntl, ret, F_SETFD, FD_CLOEXEC); > | return __syscall_ret(ret); > |} > |ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, 0); > |if (ret != -EINVAL) { > | if (ret >= 0) __syscall(SYS_close, ret); > | return __syscall_ret(-EINVAL); > |} > |ret = __syscall(SYS_fcntl, fd, F_DUPFD, arg); > |if (ret >= 0) __syscall(SYS_fcntl, ret, F_SETFD, FD_CLOEXEC); > |return __syscall_ret(ret); > > And I'm wondering what the point of the second call to F_DUPFD_CLOEXEC > is. If it is just to test that the argument is valid, then why not just > get rid of the block? It's to test if F_DUPFD_CLOEXEC failed with EINVAL because it's an unknown fcntl command or because arg exceeds the fd limit. Unfortunately, the error is ambiguous. > F_DUPFD has the same constraints on the argument. That's a good point -- F_DUPFD can be expected to fail (thus not introduce any fd leak race condition) as long as the limit does not change concurrently with the fcntl call. But if the limit does change concurrently, the test result may be outdated by the time it's used. So I think it may make sense to try the fallback directly. > Getting rid of that block also has the advantage of being able to factor > out the F_SETFD call. Then it would simply be > > int ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, arg); > if (ret == -EINVAL) ret = __syscall(SYS_fcntl, fd, F_DUPFD, arg); > if (ret >= 0) __syscall(SYS_fcntl, fd, F_SETFL, FD_CLOEXEC); > return __syscall_ret(ret); > > Much nicer, isn't it? And it doesn't even allocate a file descriptor > uselessly. Seems like it. I'll wait for further feedback but this might be the right thing to do. Note that this factoring-out by inverting the conditions can be done even without dropping the arg=0 second check. Rich