public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: Christian Brauner <christian.brauner@canonical.com>
To: Christian Brauner <christian.brauner@ubuntu.com>
Cc: libc-alpha@sourceware.org, fweimer@redhat.com,
	joseph@codesourcery.com, schwab@suse.de
Subject: Re: [PATCH 1/2 v5] openpty: close slave pty fd on error
Date: Sun, 10 Sep 2017 17:45:00 -0000	[thread overview]
Message-ID: <20170910174527.duvyhfq4i2ejkqoa@gmail.com> (raw)
In-Reply-To: <20170829143037.24231-2-christian.brauner@ubuntu.com> <20170829143037.24231-1-christian.brauner@ubuntu.com>

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

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
> 

  parent reply	other threads:[~2017-09-10 17:45 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-26 14:21 [PATCH 1/2] " Christian Brauner
2017-08-26 14:21 ` [PATCH 2/2] openpty: use TIOCGPTPEER to open slave side fd Christian Brauner
2017-08-28  7:34   ` Florian Weimer
2017-08-28 11:14     ` Christian Brauner
2017-08-28 11:39     ` Joseph Myers
2017-08-28 12:11     ` [PATCH 2/2 v2] " Christian Brauner
2017-08-28 12:22       ` Joseph Myers
2017-08-28 12:34       ` Andreas Schwab
2017-08-28 12:51         ` [PATCH 2/2 v3] " Christian Brauner
2017-08-29  9:07 ` [PATCH 1/2] openpty: close slave pty fd on error Florian Weimer
2017-08-29 13:46   ` [PATCH 1/2 v4] " Christian Brauner
2017-08-29 13:46     ` [PATCH 2/2 v4] openpty: use TIOCGPTPEER to open slave side fd Christian Brauner
2017-08-29 14:00     ` [PATCH 1/2 v4] openpty: close slave pty fd on error Andreas Schwab
2017-08-29 14:12       ` Christian Brauner
2017-08-29 14:31       ` [PATCH 1/2 v5] " Christian Brauner
2017-08-29 14:31         ` [PATCH 2/2 v5] openpty: use TIOCGPTPEER to open slave side fd Christian Brauner
2017-09-10 17:45         ` Christian Brauner [this message]
2017-09-20 10:53           ` [PATCH 1/2 v5] openpty: close slave pty fd on error Christian Brauner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170910174527.duvyhfq4i2ejkqoa@gmail.com \
    --to=christian.brauner@canonical.com \
    --cc=christian.brauner@ubuntu.com \
    --cc=fweimer@redhat.com \
    --cc=joseph@codesourcery.com \
    --cc=libc-alpha@sourceware.org \
    --cc=schwab@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).