Re: [PATCH] coresignt: stdio: modify the fdopen interface

[Rich Felker <dalias@libc.org>]

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