mailing list of musl libc
 help / color / Atom feed
* [PATCH] coresignt: stdio: modify the fdopen interface
@ 2019-12-25 10:31 Liu Jie
  2019-12-25 19:19 ` Rich Felker
  0 siblings, 1 reply; 2+ messages in thread
From: Liu Jie @ 2019-12-25 10:31 UTC (permalink / raw)
  To: musl; +Cc: yunlong.song, liujie1

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.

Signed-off-by: Liu Jie <liujie1@huawei.com>
---
 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 <errno.h>
 #include <string.h>
 #include "libc.h"
+#include <stdio.h>
 
 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) {
+			free(f);
+			errno = -err;
+			return NULL;
+		}
+	}
 
 	/* 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;
+		}
+		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;
+			}
+		}
 		f->flags |= F_APP;
 	}
 
@@ -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';
 
 	/* Initialize op ptrs. No problem if some are unneeded. */
-- 
2.17.1



^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] coresignt: stdio: modify the fdopen interface
  2019-12-25 10:31 [PATCH] coresignt: stdio: modify the fdopen interface Liu Jie
@ 2019-12-25 19:19 ` Rich Felker
  0 siblings, 0 replies; 2+ messages in thread
From: Rich Felker @ 2019-12-25 19:19 UTC (permalink / raw)
  To: musl

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 <liujie1@huawei.com>
> ---
>  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 <errno.h>
>  #include <string.h>
>  #include "libc.h"
> +#include <stdio.h>
>  
>  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


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-25 10:31 [PATCH] coresignt: stdio: modify the fdopen interface Liu Jie
2019-12-25 19:19 ` Rich Felker

mailing list of musl libc

Archives are clonable: git clone --mirror http://inbox.vuxu.org/musl

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.musl


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git