From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/15059 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] coresignt: stdio: modify the fdopen interface Date: Wed, 25 Dec 2019 14:19:32 -0500 Message-ID: <20191225191932.GB30412@brightrain.aerifal.cx> References: <20191225103150.23251-1-liujie1@huawei.com> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="79173"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-15075-gllmg-musl=m.gmane.org@lists.openwall.com Wed Dec 25 20:19:48 2019 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.89) (envelope-from ) id 1ikCCC-000KVP-59 for gllmg-musl@m.gmane.org; Wed, 25 Dec 2019 20:19:48 +0100 Original-Received: (qmail 31899 invoked by uid 550); 25 Dec 2019 19:19:45 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 31881 invoked from network); 25 Dec 2019 19:19:45 -0000 Content-Disposition: inline In-Reply-To: <20191225103150.23251-1-liujie1@huawei.com> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:15059 Archived-At: On Wed, Dec 25, 2019 at 06:31:50PM +0800, Liu Jie wrote: > For fdopen function, its RETURN VALUE is defined in posix as "Upon > successful completion, fdopen() shall return a pointer to a stream; > otherwise, a null pointer shall be returned and errno set to > indicate the error.". This patch made some changes in __fdopen.c > for error types: EBADF, EINVAL, EMFILE and ENOMEM. POSIX defines EBADF for fd being invalid is a "may fail", not a "shall fail" error. For the most part musl ignores "may fail" conditions unless there is a demonstrable safety benefit to detecting them; I don't think there's one here. The other errors mentioned are not even possible for the places you're checking them in; see comments below: > Signed-off-by: Liu Jie > --- > src/stdio/__fdopen.c | 40 ++++++++++++++++++++++++++++++++++++---- > 1 file changed, 36 insertions(+), 4 deletions(-) > > diff --git a/src/stdio/__fdopen.c b/src/stdio/__fdopen.c > index 116e78e5..c9469b07 100644 > --- a/src/stdio/__fdopen.c > +++ b/src/stdio/__fdopen.c > @@ -5,11 +5,13 @@ > #include > #include > #include "libc.h" > +#include > > FILE *__fdopen(int fd, const char *mode) > { > FILE *f; > struct winsize wsz; > + int err; > > /* Check for valid initial mode character */ > if (!strchr("rwa", *mode)) { > @@ -27,13 +29,35 @@ FILE *__fdopen(int fd, const char *mode) > if (!strchr(mode, '+')) f->flags = (*mode == 'r') ? F_NOWR : F_NORD; > > /* Apply close-on-exec flag */ > - if (strchr(mode, 'e')) __syscall(SYS_fcntl, fd, F_SETFD, FD_CLOEXEC); > + if (strchr(mode, 'e')) { > + err = __syscall(SYS_fcntl, fd, F_SETFD, FD_CLOEXEC); > + if (err == -EBADF || err == -EINVAL || > + err == -EMFILE || err == -ENOMEM) { The only one of these errors that can actually happen is EBADF, for an invalid fd. > /* Set append mode on fd if opened for append */ > if (*mode == 'a') { > int flags = __syscall(SYS_fcntl, fd, F_GETFL); > - if (!(flags & O_APPEND)) > - __syscall(SYS_fcntl, fd, F_SETFL, flags | O_APPEND); > + err = flags; > + if (err == -EBADF || err == -EINVAL || > + err == -EMFILE || err == -ENOMEM) { > + free(f); > + errno = -err; > + return NULL; > + } Same here. > + if (!(flags & O_APPEND)) { > + err = __syscall(SYS_fcntl, fd, F_SETFL, flags | O_APPEND); > + if (err == -EBADF || err == -EINVAL || > + err == -EMFILE || err == -ENOMEM) { > + free(f); > + errno = -err; > + return NULL; > + } > + } And here. > @@ -43,7 +67,15 @@ FILE *__fdopen(int fd, const char *mode) > > /* Activate line buffered mode for terminals */ > f->lbf = EOF; > - if (!(f->flags & F_NOWR) && !__syscall(SYS_ioctl, fd, TIOCGWINSZ, &wsz)) > + err = __syscall(SYS_ioctl, fd, TIOCGWINSZ, &wsz); > + if (err == -EBADF || err == -EINVAL || > + err == -EMFILE || err == -ENOMEM) { > + free(f); > + errno = -err; > + return NULL; > + } > + > + if (!(f->flags & F_NOWR) && !err) > f->lbf = '\n'; The only use of the result here is to determine if the fd refers to a tty. It will succeed if so, fail if not (including the case where it's not valid). Aside from that, introduction of free() in this file is a linking properties regression; previously fdopen and fopen intentionally did not depend on free. If you feel like there is some safety/hardening advantage to checking for EBADF and still want to do it, the right way would be moving the tty check to the top of the function so it happens first, just before the malloc, so there's a common (always-executed) code path the check takes place in. After that, we've ruled out EBADF, so all the remaining syscalls can be assumed to succed. But I'm doubtful that this change is beneficial anyway. Rich