Re: [PATCH 1/2 v5] openpty: close slave pty fd on error

[Christian Brauner <christian dot brauner at canonical dot com>]

On Sun, Sep 10, 2017 at 07:45:27PM +0200, Christian Brauner wrote:
> Hi guys,
> 
> Any update on whether this is acceptable for inclusion or not now. Linux 4.13
> has been released which measn TIOCGPTPEER is now stable API.
> 
> Christian

Hi everyone,

Friendly ping about this patch again. I'm not sure where we left of. Last time
it seemed you were fine with the patch. Any change that you'll merge this soon?

Thanks!
Christian

> 
> On Tue, Aug 29, 2017 at 04:30:36PM +0200, Christian Brauner wrote:
> > When openpty() failed only the master fd was closed so far. Let's close the
> > slave fd as well. Also, let's unify the error handling.
> > 
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > Changelog 2017-08-29:
> > * Unify error handling: use a common function exit that frees everything that
> >   needs freeing. (@Florian)
> > Changelog 2017-08-29:
> > * Do not be stupid and only close the file descriptors on error! Duh. (Thanks,
> >   @Andreas)
> > ---
> >  ChangeLog       |  4 ++++
> >  login/openpty.c | 30 ++++++++++++++++--------------
> >  2 files changed, 20 insertions(+), 14 deletions(-)
> > 
> > diff --git a/ChangeLog b/ChangeLog
> > index bc1cf94dc3..bc5fb8e27f 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,3 +1,7 @@
> > +2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
> > +
> > +	* login/openpty.c (openpty): Close slave pty file descriptor on error.
> > +
> >  2017-08-25  H.J. Lu  <hongjiu.lu@intel.com>
> >  
> >  	* sysdeps/x86/cpu-features.h [__ASSEMBLER__]
> > diff --git a/login/openpty.c b/login/openpty.c
> > index 41ab0483e2..9e556c27a5 100644
> > --- a/login/openpty.c
> > +++ b/login/openpty.c
> > @@ -92,29 +92,24 @@ openpty (int *amaster, int *aslave, char *name,
> >    char _buf[512];
> >  #endif
> >    char *buf = _buf;
> > -  int master, slave;
> > +  int master, ret = -1, slave = -1;
> >  
> >    master = getpt ();
> >    if (master == -1)
> >      return -1;
> >  
> >    if (grantpt (master))
> > -    goto fail;
> > +    goto on_error;
> >  
> >    if (unlockpt (master))
> > -    goto fail;
> > +    goto on_error;
> >  
> >    if (pts_name (master, &buf, sizeof (_buf)))
> > -    goto fail;
> > +    goto on_error;
> >  
> >    slave = open (buf, O_RDWR | O_NOCTTY);
> >    if (slave == -1)
> > -    {
> > -      if (buf != _buf)
> > -	free (buf);
> > -
> > -      goto fail;
> > -    }
> > +    goto on_error;
> >  
> >    /* XXX Should we ignore errors here?  */
> >    if (termp)
> > @@ -129,12 +124,19 @@ openpty (int *amaster, int *aslave, char *name,
> >    if (name != NULL)
> >      strcpy (name, buf);
> >  
> > +  ret = 0;
> > +
> > + on_error:
> > +  if (ret == -1) {
> > +    close (master);
> > +
> > +    if (slave != -1)
> > +      close (slave);
> > +  }
> > +
> >    if (buf != _buf)
> >      free (buf);
> > -  return 0;
> >  
> > - fail:
> > -  close (master);
> > -  return -1;
> > +  return ret;
> >  }
> >  libutil_hidden_def (openpty)
> > -- 
> > 2.14.1
> > 
> 
> On Tue, Aug 29, 2017 at 04:30:37PM +0200, Christian Brauner wrote:
> > Newer kernels expose the ioctl TIOCGPTPEER [1] call to userspace which allows to
> > safely allocate a file descriptor for a pty slave based solely on the master
> > file descriptor. This allows us to avoid path-based operations and makes this
> > function a lot safer in the face of devpts mounts in different mount namespaces.
> > 
> > [1]: https://patchwork.kernel.org/patch/9760743/
> > 
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > Changelog 2017-08-28:
> > * Instead of #ifdefing the TIOCGPTPEER ioctl flag we now try the ioctl() first
> >   and if it fails we fallback to path-based allocation of the slave fd. This
> >   allows us retain backward compatibility with kernels that do not support this
> >   ioctl call.
> > * A note on the following codepath
> > 
> >    if (name != NULL)
> >      {
> >        if (*buf == '\0')
> >          if (pts_name (master, &buf, sizeof (_buf)))
> >            goto fail;
> > 
> >        strcpy (name, buf);
> >      }
> > 
> >   "buf" is guaranteed to be allocated in this case. If the pts_name() call above
> >   failed we would have never reached this code path. If it has been called
> >   succesfully it will either have handed us a valid buffer or "buf" will still
> >   point to the static char array "_buf" which is initialized to 0.
> > Changelog 2017-08-28:
> > * Preserve #ifdef for TIOCGPTPEER since it needs to work on non-Linux distros
> >   too.
> > * Only intialize first byte of "_buf".
> > Changelog 2017-08-29:
> > * Adapt to unified error handling as suggested by Florian.
> > ---
> >  ChangeLog       |  5 +++++
> >  login/openpty.c | 30 ++++++++++++++++++++++++------
> >  2 files changed, 29 insertions(+), 6 deletions(-)
> > 
> > diff --git a/ChangeLog b/ChangeLog
> > index bc5fb8e27f..30829e4c16 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,3 +1,8 @@
> > +2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
> > +
> > +	* login/openpty.c (openpty): If defined, use the TIOCGPTPEER ioctl call
> > +	to allocate the slave pty file descriptor.
> > +
> >  2017-08-26  Christian Brauner  <christian.brauner@ubuntu.com>
> >  
> >  	* login/openpty.c (openpty): Close slave pty file descriptor on error.
> > diff --git a/login/openpty.c b/login/openpty.c
> > index 9e556c27a5..6703128ea8 100644
> > --- a/login/openpty.c
> > +++ b/login/openpty.c
> > @@ -94,6 +94,8 @@ openpty (int *amaster, int *aslave, char *name,
> >    char *buf = _buf;
> >    int master, ret = -1, slave = -1;
> >  
> > +  *buf = '\0';
> > +
> >    master = getpt ();
> >    if (master == -1)
> >      return -1;
> > @@ -104,12 +106,22 @@ openpty (int *amaster, int *aslave, char *name,
> >    if (unlockpt (master))
> >      goto on_error;
> >  
> > -  if (pts_name (master, &buf, sizeof (_buf)))
> > -    goto on_error;
> > -
> > -  slave = open (buf, O_RDWR | O_NOCTTY);
> > +#ifdef TIOCGPTPEER
> > +  /* Try to allocate slave fd solely based on master fd first. */
> > +  slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY);
> > +#endif
> >    if (slave == -1)
> > -    goto on_error;
> > +    {
> > +      /* Fallback to path-based slave fd allocation in case kernel doesn't
> > +       * support TIOCGPTPEER.
> > +       */
> > +      if (pts_name (master, &buf, sizeof (_buf)))
> > +        goto on_error;
> > +
> > +      slave = open (buf, O_RDWR | O_NOCTTY);
> > +      if (slave == -1)
> > +        goto on_error;
> > +    }
> >  
> >    /* XXX Should we ignore errors here?  */
> >    if (termp)
> > @@ -122,7 +134,13 @@ openpty (int *amaster, int *aslave, char *name,
> >    *amaster = master;
> >    *aslave = slave;
> >    if (name != NULL)
> > -    strcpy (name, buf);
> > +    {
> > +      if (*buf == '\0')
> > +        if (pts_name (master, &buf, sizeof (_buf)))
> > +          goto on_error;
> > +
> > +      strcpy (name, buf);
> > +    }
> >  
> >    ret = 0;
> >  
> > -- 
> > 2.14.1
> > 
>