From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/6402 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: Add login_tty Date: Fri, 31 Oct 2014 12:19:07 -0400 Message-ID: <20141031161907.GD22465@brightrain.aerifal.cx> References: <20140825185756.GA6077@euler> <20140825224333.GX12888@brightrain.aerifal.cx> <20140826165627.GA1208@euler> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1414772383 28928 80.91.229.3 (31 Oct 2014 16:19:43 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 31 Oct 2014 16:19:43 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-6415-gllmg-musl=m.gmane.org@lists.openwall.com Fri Oct 31 17:19:34 2014 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1XkEvC-00053t-5h for gllmg-musl@m.gmane.org; Fri, 31 Oct 2014 17:19:30 +0100 Original-Received: (qmail 1324 invoked by uid 550); 31 Oct 2014 16:19:24 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 32716 invoked from network); 31 Oct 2014 16:19:21 -0000 Content-Disposition: inline In-Reply-To: <20140826165627.GA1208@euler> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:6402 Archived-At: 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 > 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 > +#include > +#include > + > +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