Re: Add login_tty

[Rich Felker <dalias@libc.org>]

On Thu, Sep 04, 2014 at 11:22:00PM +0200, Szabolcs Nagy wrote:
> * Felix Janda <felix.janda@posteo.de> [2014-08-26 18:56:28 +0200]:
> > --- /dev/null
> > +++ b/src/misc/login_tty.c
> > @@ -0,0 +1,14 @@
> > +#include <utmp.h>
> > +#include <sys/ioctl.h>
> > +#include <unistd.h>
> > +
> > +int login_tty(int fd)
> > +{
> > +	setsid();
> > +	if (ioctl(fd, TIOCSCTTY, (char *)0)) return -1;
> > +	dup2(fd, 0);
> > +	dup2(fd, 1);
> > +	dup2(fd, 2);
> > +	if (fd>2) close(fd);
> > +	return 0;
> > +}
> 
> i recently came across this:
> http://lxr.free-electrons.com/source/fs/file.c#L751
> 
> so dup2 may spuriously fail with EBUSY on linux

This can only happen when you're already invoking UB via a call to
dup2 where you don't know the dest fd number is already open, and
where it might race with open.

It's actually not 100% clear to me that this is UB, but I base my
claim on the allowance for the implementation to make internal use of
file descriptors, explained here:

http://austingroupbugs.net/view.php?id=149

Using dup2 where the application does not know it "owns" the dest fd
already seems equivalent to calling close on an fd you don't own.

In any case, it should not be able to happen in correct programs.

Details on the topic may be found here:

http://stackoverflow.com/a/24012015/379897

> the current forkpty does not check dup2 either, but i
> wonder if it should be
> 
>  while(dup2(fd,0)==-1 && errno==EBUSY);
> 
> instead

Actually, musl's dup2 already accounts for the issue by looping
internally, but I'm thinking we should remove that. POSIX does not
forbid dup2 from failing when you do something idiotic like this
(actually, like I said, I think it's morally UB), but it does demand
that open and dup2 be atomic with respect to each other for regular
files, whereas the loop would delay indefinitely a thread calling dup2
on a file descriptor for which another thread is stuck in
uninterruptible sleep trying to open (e.g. slow/dead NFS).

Any thoughts on whether/how this should be changed?

Rich