[PATCH] return EBADF from ttyname_r

[PATCH] return EBADF from ttyname_r

[Benjamin Peterson]

POSIX allows ttyname(_r) to return EBADF if passed file descriptor is invalid.
---
 src/unistd/ttyname_r.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/unistd/ttyname_r.c b/src/unistd/ttyname_r.c
index 33aa4ae1..e208b3c3 100644
--- a/src/unistd/ttyname_r.c
+++ b/src/unistd/ttyname_r.c
@@ -10,7 +10,10 @@ int ttyname_r(int fd, char *name, size_t size)
 	char procname[sizeof "/proc/self/fd/" + 3*sizeof(int) + 2];
 	ssize_t l;
 
-	if (!isatty(fd)) return ENOTTY;
+	if (!isatty(fd)) {
+		if (errno == EBADF) return EBADF;
+		return ENOTTY;
+	}
 
 	__procfdname(procname, fd);
 	l = readlink(procname, name, size);
-- 
2.17.1

Re: [PATCH] return EBADF from ttyname_r

[Rich Felker]

On Wed, Sep 12, 2018 at 05:34:24PM -0700, Benjamin Peterson wrote:
> POSIX allows ttyname(_r) to return EBADF if passed file descriptor is invalid.
> ---
>  src/unistd/ttyname_r.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/unistd/ttyname_r.c b/src/unistd/ttyname_r.c
> index 33aa4ae1..e208b3c3 100644
> --- a/src/unistd/ttyname_r.c
> +++ b/src/unistd/ttyname_r.c
> @@ -10,7 +10,10 @@ int ttyname_r(int fd, char *name, size_t size)
>  	char procname[sizeof "/proc/self/fd/" + 3*sizeof(int) + 2];
>  	ssize_t l;
>  
> -	if (!isatty(fd)) return ENOTTY;
> +	if (!isatty(fd)) {
> +		if (errno == EBADF) return EBADF;
> +		return ENOTTY;
> +	}
>  
>  	__procfdname(procname, fd);
>  	l = readlink(procname, name, size);
> -- 
> 2.17.1

The EBADF error for isatty is optional and musl's does not set it, so
this patch does not work as-is. I think returning ENOTTY for cases for
which it does not apply in ttyname_r is non-conforming though, so
some change similar to this is probably needed. If isatty were
modified to set errno, I think we could just return errno here.

Rich

Re: [PATCH] return EBADF from ttyname_r

[A. Wilcox]

[-- Attachment #1.1: Type: text/plain, Size: 879 bytes --]

On 09/12/18 21:07, Rich Felker wrote:
> The EBADF error for isatty is optional and musl's does not set it, so
> this patch does not work as-is. I think returning ENOTTY for cases for
> which it does not apply in ttyname_r is non-conforming though, so
> some change similar to this is probably needed. If isatty were
> modified to set errno, I think we could just return errno here.
> 
> Rich
> 


Please do feel free to work on isatty's error handling, though, Benjamin
(or others).

It is non-conformant as it stands; it returns 0 for /dev/null and it
does not error on a closed fd.

We're tracking this and other issues at the following URL (but note,
it's a little bit behind and a few have been fixed since):
https://wiki.adelielinux.org/wiki/POSIX

Best,
--arw


-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
https://www.adelielinux.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] return EBADF from ttyname_r

[Rich Felker]

On Wed, Sep 12, 2018 at 10:23:37PM -0500, A. Wilcox wrote:
> On 09/12/18 21:07, Rich Felker wrote:
> > The EBADF error for isatty is optional and musl's does not set it, so
> > this patch does not work as-is. I think returning ENOTTY for cases for
> > which it does not apply in ttyname_r is non-conforming though, so
> > some change similar to this is probably needed. If isatty were
> > modified to set errno, I think we could just return errno here.
> 
> Please do feel free to work on isatty's error handling, though, Benjamin
> (or others).
> 
> It is non-conformant as it stands; it returns 0 for /dev/null and it
> does not error on a closed fd.

These are "may fail" errors. The only mandate is that isatty return 1
if the argument is a fd for a tty and 0 otherwise. But the related
ttyname_r issue reported here makes it clear that choosing to follow
that "may" is the right choice for a simple implementation, I think.

Rich

Re: [PATCH] return EBADF from ttyname_r

[Szabolcs Nagy]

* Benjamin Peterson <benjamin@python.org> [2018-09-12 17:34:24 -0700]:
> POSIX allows ttyname(_r) to return EBADF if passed file descriptor is invalid.

i think EBADF is always a 'may fail' in posix, so not strictly required.

> -	if (!isatty(fd)) return ENOTTY;
> +	if (!isatty(fd)) {
> +		if (errno == EBADF) return EBADF;
> +		return ENOTTY;
> +	}

musl isatty uses __syscall which does not set errno so this is wrong.

note that on glibc isatty sets errno according what the kernel returns
however linux has different code paths in ioctl for different type of
fds and in some cases it can fail in interesting ways (iirc on a socket
fd it will fail with EINVAL or EFAULT at least on some linux versions
and it can even spuriously succeed on non-tty fds because the TCGETS
ioctl command was reused on some audio device to do different things)

this means users cannot rely on errno value being sane,
so there is not much point trying to do something fancy here.

Re: [PATCH] return EBADF from ttyname_r

[Rich Felker]

On Thu, Sep 13, 2018 at 10:53:14AM +0200, Szabolcs Nagy wrote:
> * Benjamin Peterson <benjamin@python.org> [2018-09-12 17:34:24 -0700]:
> > POSIX allows ttyname(_r) to return EBADF if passed file descriptor is invalid.
> 
> i think EBADF is always a 'may fail' in posix, so not strictly required.
> 
> > -	if (!isatty(fd)) return ENOTTY;
> > +	if (!isatty(fd)) {
> > +		if (errno == EBADF) return EBADF;
> > +		return ENOTTY;
> > +	}
> 
> musl isatty uses __syscall which does not set errno so this is wrong.
> 
> note that on glibc isatty sets errno according what the kernel returns
> however linux has different code paths in ioctl for different type of
> fds and in some cases it can fail in interesting ways (iirc on a socket
> fd it will fail with EINVAL or EFAULT at least on some linux versions
> and it can even spuriously succeed on non-tty fds because the TCGETS
> ioctl command was reused on some audio device to do different things)

That's why we no longer use TCGETS but rather TIOCGWINSZ.

> this means users cannot rely on errno value being sane,
> so there is not much point trying to do something fancy here.

I'm not sure this is actually an issue anymore, but if it is, we
should simply translate anything other than EBADF to ENOTTY. There is
no other meaningful error. Either the fd is valid or it's not, and if
it is valid, either it is a tty or it's not.

Rich

Re: [PATCH] return EBADF from ttyname_r

[Benjamin Peterson]

Thank you for the feedback.

On Thu, Sep 13, 2018, at 08:29, Rich Felker wrote:
> On Thu, Sep 13, 2018 at 10:53:14AM +0200, Szabolcs Nagy wrote:
> > * Benjamin Peterson <benjamin@python.org> [2018-09-12 17:34:24 -0700]:
> > > POSIX allows ttyname(_r) to return EBADF if passed file descriptor is invalid.
> > 
> > i think EBADF is always a 'may fail' in posix, so not strictly required.

Right, I don't claim this patch fixes a bug; just makes the error reporting more precise.

> > 
> > > -	if (!isatty(fd)) return ENOTTY;
> > > +	if (!isatty(fd)) {
> > > +		if (errno == EBADF) return EBADF;
> > > +		return ENOTTY;
> > > +	}
> > 
> > musl isatty uses __syscall which does not set errno so this is wrong.

Good point.

> > 
> > note that on glibc isatty sets errno according what the kernel returns
> > however linux has different code paths in ioctl for different type of
> > fds and in some cases it can fail in interesting ways (iirc on a socket
> > fd it will fail with EINVAL or EFAULT at least on some linux versions
> > and it can even spuriously succeed on non-tty fds because the TCGETS
> > ioctl command was reused on some audio device to do different things)
> 
> That's why we no longer use TCGETS but rather TIOCGWINSZ.
> 
> > this means users cannot rely on errno value being sane,
> > so there is not much point trying to do something fancy here.
> 
> I'm not sure this is actually an issue anymore, but if it is, we
> should simply translate anything other than EBADF to ENOTTY. There is
> no other meaningful error. Either the fd is valid or it's not, and if
> it is valid, either it is a tty or it's not.

I will send another patch to this effect.

> 
> Rich