Add login_tty

Add login_tty

[Felix Janda]

Hi,

could "int login_tty(int fd)" be added? It is essentially the part of
forkpty() executed by the forked child. (apart from closing the master)

For example, uim(-fep) uses it for a modified forkpty(). Some other
programs might be pulling in the corresponding gnulib module.

Regards,
Felix

Re: Add login_tty

[Rich Felker]

On Mon, Aug 25, 2014 at 08:57:57PM +0200, Felix Janda wrote:
> Hi,
> 
> could "int login_tty(int fd)" be added? It is essentially the part of
> forkpty() executed by the forked child. (apart from closing the master)
> 
> For example, uim(-fep) uses it for a modified forkpty(). Some other
> programs might be pulling in the corresponding gnulib module.

I don't have any fundamental objection to this. It might be nice to
review the forkpty code for errors it should be checking and make
these improvements at the same time, though.

Rich

Re: Add login_tty

[Felix Janda]

[-- Attachment #1: Type: text/plain, Size: 248 bytes --]

Rich Felker wrote:
[..]
> I don't have any fundamental objection to this. It might be nice to
> review the forkpty code for errors it should be checking and make
> these improvements at the same time, though.

Ok, attached a proposed patch.

Felix

[-- Attachment #2: 0001-split-off-login_tty-from-forkpty-and-clean-up-the-la.patch --]
[-- Type: text/x-patch, Size: 2255 bytes --]

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.
---
 include/utmp.h       |  2 ++
 src/misc/forkpty.c   | 25 ++++---------------------
 src/misc/login_tty.c | 14 ++++++++++++++
 3 files changed, 20 insertions(+), 21 deletions(-)
 create mode 100644 src/misc/login_tty.c

diff --git a/include/utmp.h b/include/utmp.h
index e9ba23e..d3dcee7 100644
--- a/include/utmp.h
+++ b/include/utmp.h
@@ -43,6 +43,8 @@ void updwtmp(const char *, const struct utmp *);
 #define UTMP_FILENAME _PATH_UTMP
 #define WTMP_FILENAME _PATH_WTMP
 
+int login_tty(int);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/misc/forkpty.c b/src/misc/forkpty.c
index 07f8d01..007b369 100644
--- a/src/misc/forkpty.c
+++ b/src/misc/forkpty.c
@@ -1,37 +1,20 @@
 #include <pty.h>
+#include <utmp.h>
 #include <unistd.h>
-#include <sys/ioctl.h>
-#include <fcntl.h>
 
 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;
-		}
-	}
-	pid = i==3 ? fork() : -1;
+	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);
 		return 0;
 	}
-	for (i=0; i<3; i++)
-		if (istmp[i]) close(i);
 	close(s);
 	if (pid < 0) close(*m);
 	return pid;
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;
+}
-- 
1.8.5.5

Re: Add login_tty

[Felix Janda]

Felix Janda wrote:
> Rich Felker wrote:
> [..]
> > I don't have any fundamental objection to this. It might be nice to
> > review the forkpty code for errors it should be checking and make
> > these improvements at the same time, though.
> 
> Ok, attached a proposed patch.

Any comments?

login_tty() fails if and only if the ioctl fails. This is what I have
read in online man pages of other implementations.

It might be desirable to make forkpty() fail when login_tty() fails
in the child.

Felix

Re: Add login_tty

[Szabolcs Nagy]

* 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

the current forkpty does not check dup2 either, but i
wonder if it should be

 while(dup2(fd,0)==-1 && errno==EBUSY);

instead

Re: Add login_tty

[Rich Felker]

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

Re: Add login_tty

[Justin Cormack]

On Thu, Sep 4, 2014 at 10:33 PM, Rich Felker <dalias@libc.org> wrote:
> 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?

Both FreeBSD and NetBSD block indefinitely on dup2 running the example
code, and this behaviour does not seem wholly unreasonable. But
removing the loop in Musl seems right, and having a note in the man
page saying if you get EBUSY you did something stupid, so abort on
this seems best.

Justin

Re: Add login_tty

[Felix Janda]

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

The other possibility for dup2 to fail seems to be EINTR.
(We already know that fd can't be invalid.)
Should this also be checked?

Felix

Re: Add login_tty

[Rich Felker]

On Fri, Sep 05, 2014 at 07:23:52PM +0200, Felix Janda wrote:
> 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
> > 
> > the current forkpty does not check dup2 either, but i
> > wonder if it should be
> > 
> >  while(dup2(fd,0)==-1 && errno==EBUSY);
> > 
> > instead
> 
> The other possibility for dup2 to fail seems to be EINTR.
> (We already know that fd can't be invalid.)
> Should this also be checked?

Why would EINTR happen? dup2 does not involve any sleep much less
interruptible sleep/blocking.

Rich

Re: Add login_tty

[Felix Janda]

Rich Felker wrote:
> On Fri, Sep 05, 2014 at 07:23:52PM +0200, Felix Janda wrote:
[..]
> > The other possibility for dup2 to fail seems to be EINTR.
> > (We already know that fd can't be invalid.)
> > Should this also be checked?
> 
> Why would EINTR happen? dup2 does not involve any sleep much less
> interruptible sleep/blocking.

You're right. Only EBUSY, EBADF or EINVAL can happen.

Felix

Re: Add login_tty

[Rich Felker]

On Tue, Aug 26, 2014 at 06:56:28PM +0200, Felix Janda wrote:
> Rich Felker wrote:
> [..]
> > I don't have any fundamental objection to this. It might be nice to
> > review the forkpty code for errors it should be checking and make
> > these improvements at the same time, though.
> 
> Ok, attached a proposed patch.

Sorry I never reviewed this properly before. There's been a request
for it again so I'm taking a more detailed look.

> >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.
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.

> 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?

Rich

Re: Add login_tty

[Felix Janda]

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

Re: Add login_tty

[Rich Felker]

On Sat, Nov 01, 2014 at 10:15:23PM +0100, Felix Janda wrote:
> > 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.

Indeed, looking at the kernel code, I don't see any error paths where
this operation could fail. I had figured some allocations might be
needed to represent the new fd in the fd table, but it seems not. So
the current code is probably unnecessary.

> > > +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.

OK. Surprising, but whatever. :)

In that case maybe your patch is okay as-is, aside from needing to be
factored into two changes -- one for removing useless code and the
other for separating-out login_tty.

Rich

Re: Add login_tty

[Szabolcs Nagy]

* Rich Felker <dalias@libc.org> [2014-11-01 17:45:03 -0400]:
> > 
> > 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.
> 
> Indeed, looking at the kernel code, I don't see any error paths where
> this operation could fail. I had figured some allocations might be
> needed to represent the new fd in the fd table, but it seems not. So
> the current code is probably unnecessary.
> 

i think with seccomp syscall filtering anything can fail

although libc probably dont have to take that into account

Re: Add login_tty

[Felix Janda]

[-- Attachment #1: Type: text/plain, Size: 238 bytes --]

Rich Felker wrote:
[..]
> In that case maybe your patch is okay as-is, aside from needing to be
> factored into two changes -- one for removing useless code and the
> other for separating-out login_tty.

Both of them are attached.

Felix

[-- Attachment #2: 0001-Remove-unnecessary-allocation-of-fd-0-1-2-in-forkpty.patch --]
[-- Type: text/x-patch, Size: 1385 bytes --]

From 8b60d5f69bf3b199e4050f3b1f24b76657765ea0 Mon Sep 17 00:00:00 2001
From: Felix Janda <felix.janda@posteo.de>
Date: Sat, 1 Nov 2014 23:08:24 +0100
Subject: [PATCH 1/2] Remove unnecessary allocation of fd 0/1/2 in forkpty()

Since there are no error paths in the kernel's dup2() implementation
which might be evaded by first allocating fd 0/1/2, remove the
corresponding code.
---
 src/misc/forkpty.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/misc/forkpty.c b/src/misc/forkpty.c
index 07f8d01..4a1ea9b 100644
--- a/src/misc/forkpty.c
+++ b/src/misc/forkpty.c
@@ -5,21 +5,12 @@
 
 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;
-		}
-	}
-	pid = i==3 ? fork() : -1;
+	pid = fork();
 	if (!pid) {
 		close(*m);
 		setsid();
@@ -30,8 +21,6 @@ int forkpty(int *m, char *name, const struct termios *tio, const struct winsize
 		if (s>2) close(s);
 		return 0;
 	}
-	for (i=0; i<3; i++)
-		if (istmp[i]) close(i);
 	close(s);
 	if (pid < 0) close(*m);
 	return pid;
-- 
2.0.4


[-- Attachment #3: 0002-Split-off-login_tty-form-forkpty.patch --]
[-- Type: text/x-patch, Size: 1773 bytes --]

From 00f65eed60d4e805bb03bf638289d45409b93f60 Mon Sep 17 00:00:00 2001
From: Felix Janda <felix.janda@posteo.de>
Date: Sat, 1 Nov 2014 23:20:27 +0100
Subject: [PATCH 2/2] Split off login_tty form forkpty

---
 include/utmp.h       |  2 ++
 src/misc/forkpty.c   | 10 ++--------
 src/misc/login_tty.c | 14 ++++++++++++++
 3 files changed, 18 insertions(+), 8 deletions(-)
 create mode 100644 src/misc/login_tty.c

diff --git a/include/utmp.h b/include/utmp.h
index e9ba23e..d3dcee7 100644
--- a/include/utmp.h
+++ b/include/utmp.h
@@ -43,6 +43,8 @@ void updwtmp(const char *, const struct utmp *);
 #define UTMP_FILENAME _PATH_UTMP
 #define WTMP_FILENAME _PATH_WTMP
 
+int login_tty(int);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/misc/forkpty.c b/src/misc/forkpty.c
index 4a1ea9b..007b369 100644
--- a/src/misc/forkpty.c
+++ b/src/misc/forkpty.c
@@ -1,7 +1,6 @@
 #include <pty.h>
+#include <utmp.h>
 #include <unistd.h>
-#include <sys/ioctl.h>
-#include <fcntl.h>
 
 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);
 		return 0;
 	}
 	close(s);
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;
+}
-- 
2.0.4

Re: Add login_tty

[Rich Felker]

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?

Rich

Re: Add login_tty

[Felix Janda]

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

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;
}

Felix

Re: Add login_tty

[Rich Felker]

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

Re: Add login_tty

[Felix Janda]

> > 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?

I've checked that Free- Net- and OpenBSD have the behavior of the
previous version.

Another approach would be to _exit in the child if login_tty fails.
However the parent might interpret the exit code.

Felix

> Rich

Re: Add login_tty

[Rich Felker]

On Sun, Nov 02, 2014 at 03:19:12PM +0100, Felix Janda wrote:
> > > 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?
> 
> I've checked that Free- Net- and OpenBSD have the behavior of the
> previous version.
> 
> Another approach would be to _exit in the child if login_tty fails.
> However the parent might interpret the exit code.

There's an approach I use in posix_spawn that could be copied here. My
hope was to keep forkpty simpler but it's not too bad and it would
make it so we could handle other arbitrary errors in the child if we
ever need to, so maybe it's a good idea.

Rich

Re: Add login_tty

[Felix Janda]

Rich Felker wrote:
[..]
> There's an approach I use in posix_spawn that could be copied here. My
> hope was to keep forkpty simpler but it's not too bad and it would
> make it so we could handle other arbitrary errors in the child if we
> ever need to, so maybe it's a good idea.

So how about the following?
(I'm not sure whether error checking is necessary for read.)


#include <pty.h>
#include <utmp.h>
#include <unistd.h>

int forkpty(int *m, char *name, const struct termios *tio, const struct winsize *ws)
{
	int s, ec, p[2];
	pid_t pid;

	if (pipe2(p, O_CLOEXEC)) return -1;
	if (openpty(m, &s, name, tio, ws) < 0) return -1;

	pid = fork();
	if (!pid) {
		close(*m);
		close(p[0]);
		ec = login_tty(s);
		while (write(p[1], &ec, sizeof ec) < 0);
		if (ec) _exit(127);
		close(p[1]);
		return 0;
	}
	close(s);
	close(p[1]);
	if (pid > 0) read(p[0], &ec, sizeof ec);
	close(p[0]);
	if (pid < 0 || ec < 0) {
		close(*m);
		return -1;
	}
	return pid;
}


Felix

Re: Add login_tty

[Rich Felker]

On Sun, Nov 02, 2014 at 07:56:38PM +0100, Felix Janda wrote:
> Rich Felker wrote:
> [..]
> > There's an approach I use in posix_spawn that could be copied here. My
> > hope was to keep forkpty simpler but it's not too bad and it would
> > make it so we could handle other arbitrary errors in the child if we
> > ever need to, so maybe it's a good idea.
> 
> So how about the following?
> (I'm not sure whether error checking is necessary for read.)
> 
> 
> #include <pty.h>
> #include <utmp.h>
> #include <unistd.h>
> 
> int forkpty(int *m, char *name, const struct termios *tio, const struct winsize *ws)
> {
> 	int s, ec, p[2];
> 	pid_t pid;
> 
> 	if (pipe2(p, O_CLOEXEC)) return -1;
> 	if (openpty(m, &s, name, tio, ws) < 0) return -1;
> 
> 	pid = fork();
> 	if (!pid) {
> 		close(*m);
> 		close(p[0]);
> 		ec = login_tty(s);
> 		while (write(p[1], &ec, sizeof ec) < 0);
> 		if (ec) _exit(127);
> 		close(p[1]);
> 		return 0;
> 	}
> 	close(s);
> 	close(p[1]);
> 	if (pid > 0) read(p[0], &ec, sizeof ec);
> 	close(p[0]);
> 	if (pid < 0 || ec < 0) {
> 		close(*m);
> 		return -1;
> 	}
> 	return pid;
> }

The main omission I see is that it's leaking pids (zombies) on failure
-- the pid is never returned to the caller and you don't wait on it
here, so there's no way it will ever get reaped. This is easily fixed
by calling waitpid in the failure path.

You're also leaking the pipe fds when openpty fails, etc.

Rich

Re: Add login_tty

[Felix Janda]

Thanks for the review. Below a new version.


#include <pty.h>
#include <utmp.h>
#include <unistd.h>

int forkpty(int *m, char *name, const struct termios *tio, const struct winsize *ws)
{
	int s, ec, p[2];
	pid_t pid;

	if (openpty(m, &s, name, tio, ws) < 0) return -1;
	if (pipe2(p, O_CLOEXEC)) {
		close(s);
		goto fail;
	}

	pid = fork();
	if (!pid) {
		close(*m);
		close(p[0]);
		ec = login_tty(s);
		while (write(p[1], &ec, sizeof ec) < 0);
		if (ec) _exit(127);
		close(p[1]);
		return 0;
	}
	close(s);
	close(p[1]);
	if (pid > 0) read(p[0], &ec, sizeof ec);
	close(p[0]);
	if (pid > 0) {
		if (!ec) return pid;
		waitpid(pid, &(int){0}, 0);
	}
fail:
	close(*m);
	return -1;
}


--Felix

Re: Add login_tty

[Rich Felker]

On Mon, Nov 03, 2014 at 07:29:54PM +0100, Felix Janda wrote:
> Thanks for the review. Below a new version.

Sorry I didn't get around to reviewing this right away.

> #include <pty.h>
> #include <utmp.h>
> #include <unistd.h>
> 
> int forkpty(int *m, char *name, const struct termios *tio, const struct winsize *ws)
> {
> 	int s, ec, p[2];
> 	pid_t pid;
> 
> 	if (openpty(m, &s, name, tio, ws) < 0) return -1;
> 	if (pipe2(p, O_CLOEXEC)) {
> 		close(s);
> 		goto fail;
> 	}
> 
> 	pid = fork();
> 	if (!pid) {
> 		close(*m);
> 		close(p[0]);
> 		ec = login_tty(s);

login_tty could end up closing the pipe if stdin/out/err were
initially closed in the parent, since p[1] might be 0/1/2 in that
case. I think we need to check for this and move p[1] to a new fd in
that case (and fail if that fails) before calling login_tty.

> 		while (write(p[1], &ec, sizeof ec) < 0);
> 		if (ec) _exit(127);
> 		close(p[1]);
> 		return 0;
> 	}
> 	close(s);
> 	close(p[1]);
> 	if (pid > 0) read(p[0], &ec, sizeof ec);

This read probably needs to retry-loop, in case the parent has
interrupting signal handlers.

> 	close(p[0]);
> 	if (pid > 0) {
> 		if (!ec) return pid;
> 		waitpid(pid, &(int){0}, 0);

I think waitpid could in principle fail too, but it probably shouldn't
since the process is already dead at the time waitpid is called.

> 	}
> fail:
> 	close(*m);
> 	return -1;
> }

Otherwise it looks okay now.

Rich

Re: Add login_tty

[Rich Felker]

On Sat, Dec 20, 2014 at 07:58:21PM -0500, Rich Felker wrote:
> On Mon, Nov 03, 2014 at 07:29:54PM +0100, Felix Janda wrote:
> > Thanks for the review. Below a new version.
> 
> Sorry I didn't get around to reviewing this right away.

I just went ahead and committed login_tty since it's been pending so
long. Refactoring forkpty in terms of it and making the other
improvements to forkpty is still pending. I'll see if I can get
something committed based on your work so far if you don't come up
with something first. Thanks for all the work on this and your
patience!

Rich

Re: Add login_tty

[Rich Felker]

On Sat, Dec 20, 2014 at 07:58:21PM -0500, Rich Felker wrote:
> On Mon, Nov 03, 2014 at 07:29:54PM +0100, Felix Janda wrote:
> > Thanks for the review. Below a new version.
> 
> Sorry I didn't get around to reviewing this right away.
> 
> > #include <pty.h>
> > #include <utmp.h>
> > #include <unistd.h>
> > 
> > int forkpty(int *m, char *name, const struct termios *tio, const struct winsize *ws)
> > {
> > 	int s, ec, p[2];
> > 	pid_t pid;
> > 
> > 	if (openpty(m, &s, name, tio, ws) < 0) return -1;
> > 	if (pipe2(p, O_CLOEXEC)) {
> > 		close(s);
> > 		goto fail;
> > 	}
> > 
> > 	pid = fork();
> > 	if (!pid) {
> > 		close(*m);
> > 		close(p[0]);
> > 		ec = login_tty(s);
> 
> login_tty could end up closing the pipe if stdin/out/err were
> initially closed in the parent, since p[1] might be 0/1/2 in that
> case. I think we need to check for this and move p[1] to a new fd in
> that case (and fail if that fails) before calling login_tty.

Actually this is a non-issue, since login_tty has committed itself to
returning success by the time it dup2's over top of file descriptors
0/1/2.

However I noticed another small issue:

> > 		while (write(p[1], &ec, sizeof ec) < 0);

This is writing -1, not the errno value.

> > 		if (ec) _exit(127);
> > 		close(p[1]);
> > 		return 0;
> > 	}
> > 	close(s);
> > 	close(p[1]);
> > 	if (pid > 0) read(p[0], &ec, sizeof ec);
> 
> This read probably needs to retry-loop, in case the parent has
> interrupting signal handlers.

I'm working on an improvement and I think it's better to just block
signals for the whole function. Then the retry loop wouldn't be
needed. The reason is that we don't want to allow a signal handler to
run in a child process that "never existed" from the application's
perspective.

> 
> > 	close(p[0]);
> > 	if (pid > 0) {
> > 		if (!ec) return pid;
> > 		waitpid(pid, &(int){0}, 0);
> 
> I think waitpid could in principle fail too, but it probably shouldn't
> since the process is already dead at the time waitpid is called.

Then the retry is unneeded here too.

I've got a draft based on these comments that I'll post soon for
review.

Rich

Re: Add login_tty

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 280 bytes --]

On Sat, Dec 20, 2014 at 08:38:58PM -0500, Rich Felker wrote:
> I've got a draft based on these comments that I'll post soon for
> review.

Here's the draft. While I was at it I added protection against
cancellation. openpty should have this too but I'll do that
separately.

Rich

[-- Attachment #2: forkpty.c --]
[-- Type: text/plain, Size: 1045 bytes --]

#include <pty.h>
#include <utmp.h>
#include <unistd.h>
#include <errno.h>
#include <fcntl.h>
#include <sys/wait.h>
#include <pthread.h>

int forkpty(int *pm, char *name, const struct termios *tio, const struct winsize *ws)
{
	int m, s, ec=0, p[2], cs;
	pid_t pid=-1;
	sigset_t set, oldset;

	if (openpty(&m, &s, name, tio, ws) < 0) return -1;

	sigfillset(&set);
	pthread_sigmask(SIG_BLOCK, &set, &oldset);
	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);

	if (pipe2(p, O_CLOEXEC)) {
		close(s);
		goto out;
	}

	pid = fork();
	if (!pid) {
		close(m);
		close(p[0]);
		if (login_tty(s)) {
			write(p[1], &errno, sizeof errno);
			_exit(127);
		}
		close(p[1]);
		pthread_setcancelstate(cs, 0);
		pthread_sigmask(SIG_SETMASK, &oldset, 0);
		return 0;
	}
	close(s);
	close(p[1]);
	if (read(p[0], &ec, sizeof ec) > 0) {
		int status;
		waitpid(pid, &status, 0);
		pid = -1;
		errno = ec;
	}
	close(p[0]);

out:
	if (pid > 0) *pm = m;
	else close(m);

	pthread_setcancelstate(cs, 0);
	pthread_sigmask(SIG_SETMASK, &oldset, 0);

	return pid;
}