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

[Rich Felker]

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

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

[Felix Janda]

Rich Felker wrote:
[..]

Post by Rich Felker
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

[Felix Janda]

Post by Felix Janda
[..]

Post by Rich Felker
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

[Szabolcs Nagy]

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

[Rich Felker]

Post by Szabolcs Nagy

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

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

Post by Szabolcs Nagy
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

[Justin Cormack]

Post by Rich Felker
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

[Felix Janda]

Post by Szabolcs Nagy

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

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

[Rich Felker]

Post by Felix Janda

Post by Szabolcs Nagy

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

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

[Felix Janda]

[..]

Post by Rich Felker

Post by Felix Janda
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

[Rich Felker]

Post by Felix Janda
[..]

Post by Rich Felker
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.

Post by Felix Janda

Post by Rich Felker
From f1d88438a6d00defcf96562ef536a4af71827ee7 Mon Sep 17 00:00:00 2001

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.

Post by Felix Janda
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.

Post by Felix Janda
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

[Felix Janda]

Rich Felker wrote:
[..]

Post by Rich Felker

Post by Felix Janda

Post by Rich Felker
From f1d88438a6d00defcf96562ef536a4af71827ee7 Mon Sep 17 00:00:00 2001

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.

Post by Rich Felker

Post by Felix Janda
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.

Post by Rich Felker

Post by Felix Janda
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

[Rich Felker]

Post by Felix Janda

Post by Rich Felker

Post by Felix Janda
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.

Post by Felix Janda

Post by Rich Felker

Post by Felix Janda
+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

[Szabolcs Nagy]

Post by Rich Felker

Post by Felix Janda
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

[Felix Janda]

Rich Felker wrote:
[..]

Post by Rich Felker
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

[Rich Felker]

Post by Felix Janda
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,

Post by Felix Janda
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

[Felix Janda]

Post by Rich Felker

Post by Felix Janda
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,

Post by Felix Janda
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

[Rich Felker]

Post by Felix Janda

Post by Rich Felker

Post by Felix Janda
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,

Post by Felix Janda
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. :(

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

Rich

[Felix Janda]

Post by Rich Felker

Post by 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

Post by Rich Felker
Rich

[Rich Felker]

Post by Felix Janda

Post by Rich Felker

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

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

[Felix Janda]

Rich Felker wrote:
[..]

Post by Rich Felker
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

[Rich Felker]

Post by Felix Janda
[..]

Post by Rich Felker
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

[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

[Rich Felker]

Post by Felix Janda
Thanks for the review. Below a new version.

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

Post by Felix Janda
#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.

Post by Felix Janda
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.

Post by Felix Janda
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.

Post by Felix Janda
}
close(*m);
return -1;
}

Otherwise it looks okay now.

Rich

[Rich Felker]

Post by Rich Felker

Post by Felix Janda
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

[Rich Felker]

Post by Rich Felker

Post by Felix Janda
Thanks for the review. Below a new version.

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

Post by Felix Janda
#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.

Post by Rich Felker

Post by Felix Janda
while (write(p[1], &ec, sizeof ec) < 0);

This is writing -1, not the errno value.

Post by Rich Felker

Post by Felix Janda
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.

Post by Rich Felker

Post by Felix Janda
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

[Rich Felker]

Post by Rich Felker
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