[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 31 Oct 2014 12:19:07 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: Add login_tty
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@...teo.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
