Re: Add login_tty

[Rich Felker <dalias@libc.org>]

On Sat, Nov 01, 2014 at 11:56:43PM +0100, Felix Janda wrote:
> Rich Felker wrote:
> > One more issue:
> > 
> > On Sat, Nov 01, 2014 at 11:27:29PM +0100, Felix Janda wrote:
> > >  int forkpty(int *m, char *name, const struct termios *tio, const struct winsize *ws)
> > >  {
> > > @@ -13,12 +12,7 @@ int forkpty(int *m, char *name, const struct termios *tio, const struct winsize
> > >  	pid = fork();
> > >  	if (!pid) {
> > >  		close(*m);
> > > -		setsid();
> > > -		ioctl(s, TIOCSCTTY, (char *)0);
> > > -		dup2(s, 0);
> > > -		dup2(s, 1);
> > > -		dup2(s, 2);
> > > -		if (s>2) close(s);
> > > +		login_tty(s);
> > 
> > Here there's no checking for failure of login_tty. Note that checking
> > for failure would not be useful, because there's no valid response,
> > but:
> > 
> > > diff --git a/src/misc/login_tty.c b/src/misc/login_tty.c
> > > new file mode 100644
> > > index 0000000..f0be0a0
> > > --- /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;
> > > +}
> > 
> > Unlike the code moved out of forkpty, this code errors out early if
> > the ioctl fails, thereby skipping the dup2's and close. In the case of
> > forkpty, this leaves the child process in a very messed-up state with
> > regard to its file descriptors; it will clobber the parent's
> > stdin/out/err without knowing anything is wrong.
> > 
> > In the case of forkpty, the error just needs to be ignored, I think,
> > like it is now. I'm not sure what login_tty should do, though.
> > Reporting the error is good, but leaving the process and its file
> > descriptors in an unpredictable state is not good. Is there any
> > documentation for how they're left on error?
> 
> I have taken from the man page that if the ioctl fails login_tty will
> also fail. So how about

Yes, it documents this failure, but not the side effects on failure:

- Whether a new session has been created.
- Whether file descriptors 0-2 have been replaced.
- Whether fd has been closed.

This kind of lack of documentation is a big problem in duplicating
nonstandard functions that we run into again and again. :(

> int login_tty(int fd)
> {
> 	int ret;
> 	setsid();
> 	ret = ioctl(fd, TIOCSCTTY, (char *)0);
> 	dup2(fd, 0);
> 	dup2(fd, 1);
> 	dup2(fd, 2);
> 	if (fd>2) close(fd);
> 	return ret;
> }

This behavior seems preferable in itself, but it's inconsistent with
what glibc and probably the BSDs do, so it's probably not a good idea.
glibc's behavior seems to match your previous version. This is leading
me to think maybe the code in forkpty should just stay separate. Do
you have other ideas?

Rich