From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/6423 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: Add login_tty Date: Sat, 1 Nov 2014 20:09:44 -0400 Message-ID: <20141102000944.GN22465@brightrain.aerifal.cx> References: <20140825185756.GA6077@euler> <20140825224333.GX12888@brightrain.aerifal.cx> <20140826165627.GA1208@euler> <20141031161907.GD22465@brightrain.aerifal.cx> <20141101211523.GA13145@euler> <20141101214503.GK22465@brightrain.aerifal.cx> <20141101222729.GB5949@euler> <20141101224325.GM22465@brightrain.aerifal.cx> <20141101225643.GA8817@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 1414887006 26279 80.91.229.3 (2 Nov 2014 00:10:06 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 2 Nov 2014 00:10:06 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-6436-gllmg-musl=m.gmane.org@lists.openwall.com Sun Nov 02 01:09:59 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 1Xkik2-0003Pz-Ii for gllmg-musl@m.gmane.org; Sun, 02 Nov 2014 01:09:58 +0100 Original-Received: (qmail 9718 invoked by uid 550); 2 Nov 2014 00:09:57 -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 9707 invoked from network); 2 Nov 2014 00:09:56 -0000 Content-Disposition: inline In-Reply-To: <20141101225643.GA8817@euler> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:6423 Archived-At: 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 > > > +#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; > > > +} > > > > 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