Re: Add login_tty

[Felix Janda <felix.janda@posteo.de>]

Rich Felker wrote:
[..]
> > >From f1d88438a6d00defcf96562ef536a4af71827ee7 Mon Sep 17 00:00:00 2001
> > From: Felix Janda <felix.janda@posteo.de>
> > Date: Tue, 26 Aug 2014 18:36:23 +0200
> > Subject: [PATCH] split off login_tty() from forkpty() and clean up the latter
> > 
> > since after calling openpty() no new fds are needed, an fd limit
> > causes no problems.
> 
> I assume this remark is about the other code removals in forkpty. Even
> if these were correct, they should not be part of an unrelated patch.

Ok.

> But in this case they're not correct:
> 
> >  int forkpty(int *m, char *name, const struct termios *tio, const struct winsize *ws)
> >  {
> > -	int s, t, i, istmp[3]={0};
> > +	int s;
> >  	pid_t pid;
> >  
> >  	if (openpty(m, &s, name, tio, ws) < 0) return -1;
> >  
> > -	/* Ensure before forking that we don't exceed fd limit */
> > -	for (i=0; i<3; i++) {
> > -		if (fcntl(i, F_GETFL) < 0) {
> > -			t = fcntl(s, F_DUPFD, i);
> > -			if (t<0) break;
> > -			else if (t!=i) close(t);
> > -			else istmp[i] = 1;
> > -		}
> > -	}
> 
> This loop is checking whether fd 0/1/2 are already open in the parent,
> and if not, temporarily allocating them prior to fork to detect an
> error before fork, since we can't handle errors after fork. The idea
> is that dup2 might fail when dup'ing onto an unallocated fd, but
> should never fail when atomically replacing an existing one. I'm not
> 100% sure this is correct -- the kernel might deallocate some resource
> then reallocate, rather than using in-place, in which case there would
> be a resource exhaustion leak -- but that's at least the intent of the
> code.

I still don't understand how dup2 can fail when fd 0/1/2 are not open in
the parent. AFAIU, limits on the number of open fds are imposed by an
upper bound on the value of any fd. For the dup2 calls we know that the
newfds are certainly within the limits.

> > 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;
> > +}
> 
> Is login_tty supposed to close the fd passed to it?

The man page says so.

Felix