* [PATCH 1/2] openpty: close slave pty fd on error @ 2017-08-26 14:21 Christian Brauner 2017-08-26 14:21 ` [PATCH 2/2] openpty: use TIOCGPTPEER to open slave side fd Christian Brauner 2017-08-29 9:07 ` [PATCH 1/2] openpty: close slave pty fd on error Florian Weimer 0 siblings, 2 replies; 18+ messages in thread From: Christian Brauner @ 2017-08-26 14:21 UTC (permalink / raw) To: libc-alpha, stgraber, serge; +Cc: Christian Brauner When openpty() failed only the master fd was closed so far. Let's close the slave fd as well. Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> --- ChangeLog | 4 ++++ login/openpty.c | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-) 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..8fbc66a3ef 100644 --- a/login/openpty.c +++ b/login/openpty.c @@ -92,7 +92,7 @@ openpty (int *amaster, int *aslave, char *name, char _buf[512]; #endif char *buf = _buf; - int master, slave; + int master, slave = -1; master = getpt (); if (master == -1) @@ -135,6 +135,8 @@ openpty (int *amaster, int *aslave, char *name, fail: close (master); + if (slave != -1) + close(slave); return -1; } libutil_hidden_def (openpty) -- 2.14.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] openpty: use TIOCGPTPEER to open slave side fd 2017-08-26 14:21 [PATCH 1/2] openpty: close slave pty fd on error Christian Brauner @ 2017-08-26 14:21 ` Christian Brauner 2017-08-28 7:34 ` Florian Weimer 2017-08-29 9:07 ` [PATCH 1/2] openpty: close slave pty fd on error Florian Weimer 1 sibling, 1 reply; 18+ messages in thread From: Christian Brauner @ 2017-08-26 14:21 UTC (permalink / raw) To: libc-alpha, stgraber, serge; +Cc: Christian Brauner 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 | 5 +++++ login/openpty.c | 12 +++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) 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 8fbc66a3ef..293cc0a0db 100644 --- a/login/openpty.c +++ b/login/openpty.c @@ -104,10 +104,14 @@ openpty (int *amaster, int *aslave, char *name, if (unlockpt (master)) goto fail; +#ifdef TIOCGPTPEER + slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY); +#else if (pts_name (master, &buf, sizeof (_buf))) goto fail; slave = open (buf, O_RDWR | O_NOCTTY); +#endif if (slave == -1) { if (buf != _buf) @@ -127,7 +131,13 @@ openpty (int *amaster, int *aslave, char *name, *amaster = master; *aslave = slave; if (name != NULL) - strcpy (name, buf); + { +#ifdef TIOCGPTPEER + if (pts_name (master, &buf, sizeof (_buf))) + goto fail; +#endif + strcpy (name, buf); + } if (buf != _buf) free (buf); -- 2.14.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] openpty: use TIOCGPTPEER to open slave side fd 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 ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Florian Weimer @ 2017-08-28 7:34 UTC (permalink / raw) To: Christian Brauner, libc-alpha, stgraber, serge On 08/26/2017 03:44 PM, Christian Brauner wrote: > +#ifdef TIOCGPTPEER > + slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY); > +#else > if (pts_name (master, &buf, sizeof (_buf))) > goto fail; > > slave = open (buf, O_RDWR | O_NOCTTY); > +#endif I don't think you can #ifdef out existing code this way without introducing failures on older kernels. You need to try the ioctl first, and if that fails, use the old pts_name code. Thanks, Florian ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] openpty: use TIOCGPTPEER to open slave side fd 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 2 siblings, 0 replies; 18+ messages in thread From: Christian Brauner @ 2017-08-28 11:14 UTC (permalink / raw) To: Florian Weimer; +Cc: Christian Brauner, libc-alpha, stgraber, serge On Mon, Aug 28, 2017 at 09:34:11AM +0200, Florian Weimer wrote: > On 08/26/2017 03:44 PM, Christian Brauner wrote: > > +#ifdef TIOCGPTPEER > > + slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY); > > +#else > > if (pts_name (master, &buf, sizeof (_buf))) > > goto fail; > > > > slave = open (buf, O_RDWR | O_NOCTTY); > > +#endif > > I don't think you can #ifdef out existing code this way without > introducing failures on older kernels. You need to try the ioctl first, > and if that fails, use the old pts_name code. Cool. Will resend the [PATCH 2/2] soon. I take it that [PATH 1/1] holds up as it stands. Thanks! Christian > > Thanks, > Florian ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] openpty: use TIOCGPTPEER to open slave side fd 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 2 siblings, 0 replies; 18+ messages in thread From: Joseph Myers @ 2017-08-28 11:39 UTC (permalink / raw) To: Florian Weimer; +Cc: Christian Brauner, libc-alpha, stgraber, serge On Mon, 28 Aug 2017, Florian Weimer wrote: > On 08/26/2017 03:44 PM, Christian Brauner wrote: > > +#ifdef TIOCGPTPEER > > + slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY); > > +#else > > if (pts_name (master, &buf, sizeof (_buf))) > > goto fail; > > > > slave = open (buf, O_RDWR | O_NOCTTY); > > +#endif > > I don't think you can #ifdef out existing code this way without > introducing failures on older kernels. You need to try the ioctl first, > and if that fails, use the old pts_name code. And in principle there should be appropriate __ASSUME_* conditionals so that when building with a new-enough --enable-kernel the old code can be conditioned out (however, because this code is not Linux-specific, the old case would need to stay in the code even when __ASSUME_TIOCGPTPEER is defined unconditionally in the Linux kernel-features.h). -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2 v2] openpty: use TIOCGPTPEER to open slave side fd 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 ` Christian Brauner 2017-08-28 12:22 ` Joseph Myers 2017-08-28 12:34 ` Andreas Schwab 2 siblings, 2 replies; 18+ messages in thread From: Christian Brauner @ 2017-08-28 12:11 UTC (permalink / raw) To: libc-alpha, stgraber, serge, fweimer, joseph; +Cc: Christian Brauner 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 | 5 +++++ login/openpty.c | 36 +++++++++++++++++++++++++----------- 2 files changed, 30 insertions(+), 11 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 8fbc66a3ef..43b34d7a9c 100644 --- a/login/openpty.c +++ b/login/openpty.c @@ -87,9 +87,9 @@ openpty (int *amaster, int *aslave, char *name, const struct termios *termp, const struct winsize *winp) { #ifdef PATH_MAX - char _buf[PATH_MAX]; + char _buf[PATH_MAX] = {0}; #else - char _buf[512]; + char _buf[512] = {0}; #endif char *buf = _buf; int master, slave = -1; @@ -104,16 +104,24 @@ openpty (int *amaster, int *aslave, char *name, if (unlockpt (master)) goto fail; - if (pts_name (master, &buf, sizeof (_buf))) - goto fail; - - slave = open (buf, O_RDWR | O_NOCTTY); + /* Try to allocate slave fd solely based on master fd first. */ + slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY); if (slave == -1) { - if (buf != _buf) - free (buf); - - goto fail; + /* Fallback to path-based slave fd allocation in case kernel doesn't + * support TIOCGPTPEER. + */ + if (pts_name (master, &buf, sizeof (_buf))) + goto fail; + + slave = open (buf, O_RDWR | O_NOCTTY); + if (slave == -1) + { + if (buf != _buf) + free (buf); + + goto fail; + } } /* XXX Should we ignore errors here? */ @@ -127,7 +135,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 fail; + + strcpy (name, buf); + } if (buf != _buf) free (buf); -- 2.14.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v2] openpty: use TIOCGPTPEER to open slave side fd 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 1 sibling, 0 replies; 18+ messages in thread From: Joseph Myers @ 2017-08-28 12:22 UTC (permalink / raw) To: Christian Brauner; +Cc: libc-alpha, stgraber, serge, fweimer On Mon, 28 Aug 2017, Christian Brauner wrote: > + /* Try to allocate slave fd solely based on master fd first. */ > + slave = ioctl (master, TIOCGPTPEER, O_RDWR | O_NOCTTY); You still need the #ifdef. openpty is used on non-Linux systems, and the oldest kernel headers supported for building glibc are 3.2. -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v2] openpty: use TIOCGPTPEER to open slave side fd 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 1 sibling, 1 reply; 18+ messages in thread From: Andreas Schwab @ 2017-08-28 12:34 UTC (permalink / raw) To: Christian Brauner; +Cc: libc-alpha, stgraber, serge, fweimer, joseph On Aug 28 2017, Christian Brauner <christian.brauner@ubuntu.com> wrote: > diff --git a/login/openpty.c b/login/openpty.c > index 8fbc66a3ef..43b34d7a9c 100644 > --- a/login/openpty.c > +++ b/login/openpty.c > @@ -87,9 +87,9 @@ openpty (int *amaster, int *aslave, char *name, > const struct termios *termp, const struct winsize *winp) > { > #ifdef PATH_MAX > - char _buf[PATH_MAX]; > + char _buf[PATH_MAX] = {0}; > #else > - char _buf[512]; > + char _buf[512] = {0}; > #endif No need to clear the whole array, just the first byte. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2 v3] openpty: use TIOCGPTPEER to open slave side fd 2017-08-28 12:34 ` Andreas Schwab @ 2017-08-28 12:51 ` Christian Brauner 0 siblings, 0 replies; 18+ messages in thread From: Christian Brauner @ 2017-08-28 12:51 UTC (permalink / raw) To: libc-alpha, stgraber, serge, fweimer, joseph, schwab; +Cc: Christian Brauner 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 | 5 +++++ login/openpty.c | 36 +++++++++++++++++++++++++++--------- 2 files changed, 32 insertions(+), 9 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 8fbc66a3ef..33754e18a7 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, slave = -1; + *buf = '\0'; + master = getpt (); if (master == -1) return -1; @@ -104,16 +106,26 @@ openpty (int *amaster, int *aslave, char *name, if (unlockpt (master)) goto fail; - if (pts_name (master, &buf, sizeof (_buf))) - goto fail; - - 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) { - if (buf != _buf) - free (buf); - - goto fail; + /* Fallback to path-based slave fd allocation in case kernel doesn't + * support TIOCGPTPEER. + */ + if (pts_name (master, &buf, sizeof (_buf))) + goto fail; + + slave = open (buf, O_RDWR | O_NOCTTY); + if (slave == -1) + { + if (buf != _buf) + free (buf); + + goto fail; + } } /* XXX Should we ignore errors here? */ @@ -127,7 +139,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 fail; + + strcpy (name, buf); + } if (buf != _buf) free (buf); -- 2.14.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] openpty: close slave pty fd on error 2017-08-26 14:21 [PATCH 1/2] openpty: close slave pty fd on error Christian Brauner 2017-08-26 14:21 ` [PATCH 2/2] openpty: use TIOCGPTPEER to open slave side fd Christian Brauner @ 2017-08-29 9:07 ` Florian Weimer 2017-08-29 13:46 ` [PATCH 1/2 v4] " Christian Brauner 1 sibling, 1 reply; 18+ messages in thread From: Florian Weimer @ 2017-08-29 9:07 UTC (permalink / raw) To: Christian Brauner, libc-alpha, stgraber, serge On 08/26/2017 03:44 PM, Christian Brauner wrote: > fail: > close (master); > + if (slave != -1) > + close(slave); > return -1; This is inconsistent with how the code frees buf if there is an error: For buf, the free operation happens before the fail tail. I think we should keep this consistent: either free exactly what is needed, or have a single function exit which checks for initialization and frees what has been allocated. Thanks, Florian ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2 v4] openpty: close slave pty fd on error 2017-08-29 9:07 ` [PATCH 1/2] openpty: close slave pty fd on error Florian Weimer @ 2017-08-29 13:46 ` 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 0 siblings, 2 replies; 18+ messages in thread From: Christian Brauner @ 2017-08-29 13:46 UTC (permalink / raw) To: libc-alpha, stgraber, serge, fweimer, joseph; +Cc: Christian Brauner 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 | 4 ++++ login/openpty.c | 28 ++++++++++++++-------------- 2 files changed, 18 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 fd and unify error handling. + 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..a7b1ab5dde 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,17 @@ openpty (int *amaster, int *aslave, char *name, if (name != NULL) strcpy (name, buf); + ret = 0; + + on_error: + 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2 v4] openpty: use TIOCGPTPEER to open slave side fd 2017-08-29 13:46 ` [PATCH 1/2 v4] " Christian Brauner @ 2017-08-29 13:46 ` Christian Brauner 2017-08-29 14:00 ` [PATCH 1/2 v4] openpty: close slave pty fd on error Andreas Schwab 1 sibling, 0 replies; 18+ messages in thread From: Christian Brauner @ 2017-08-29 13:46 UTC (permalink / raw) To: libc-alpha, stgraber, serge, fweimer, joseph; +Cc: Christian Brauner 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 a7b1ab5dde..766aa958ff 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2 v4] openpty: close slave pty fd on error 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 ` Andreas Schwab 2017-08-29 14:12 ` Christian Brauner 2017-08-29 14:31 ` [PATCH 1/2 v5] " Christian Brauner 1 sibling, 2 replies; 18+ messages in thread From: Andreas Schwab @ 2017-08-29 14:00 UTC (permalink / raw) To: Christian Brauner; +Cc: libc-alpha, stgraber, serge, fweimer, joseph On Aug 29 2017, Christian Brauner <christian.brauner@ubuntu.com> wrote: > @@ -129,12 +124,17 @@ openpty (int *amaster, int *aslave, char *name, > if (name != NULL) > strcpy (name, buf); > > + ret = 0; > + > + on_error: > + close (master); > + > + if (slave != -1) > + close(slave); > + You don't want to close the fds on success. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2 v4] openpty: close slave pty fd on error 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 1 sibling, 0 replies; 18+ messages in thread From: Christian Brauner @ 2017-08-29 14:12 UTC (permalink / raw) To: Andreas Schwab Cc: Christian Brauner, libc-alpha, stgraber, serge, fweimer, joseph On Tue, Aug 29, 2017 at 04:00:26PM +0200, Andreas Schwab wrote: > On Aug 29 2017, Christian Brauner <christian.brauner@ubuntu.com> wrote: > > > @@ -129,12 +124,17 @@ openpty (int *amaster, int *aslave, char *name, > > if (name != NULL) > > strcpy (name, buf); > > > > + ret = 0; > > + > > + on_error: > > + close (master); > > + > > + if (slave != -1) > > + close(slave); > > + > > You don't want to close the fds on success. Sorry, I was inatentive. > > Andreas. > > -- > Andreas Schwab, SUSE Labs, schwab@suse.de > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 > "And now for something completely different." ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2 v5] openpty: close slave pty fd on error 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 ` 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 ` [PATCH 1/2 v5] openpty: close slave pty fd on error Christian Brauner 1 sibling, 2 replies; 18+ messages in thread From: Christian Brauner @ 2017-08-29 14:31 UTC (permalink / raw) To: libc-alpha, fweimer, joseph, schwab; +Cc: Christian Brauner 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2 v5] openpty: use TIOCGPTPEER to open slave side fd 2017-08-29 14:31 ` [PATCH 1/2 v5] " Christian Brauner @ 2017-08-29 14:31 ` Christian Brauner 2017-09-10 17:45 ` [PATCH 1/2 v5] openpty: close slave pty fd on error Christian Brauner 1 sibling, 0 replies; 18+ messages in thread From: Christian Brauner @ 2017-08-29 14:31 UTC (permalink / raw) To: libc-alpha, fweimer, joseph, schwab; +Cc: Christian Brauner 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2 v5] openpty: close slave pty fd on error 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 2017-09-20 10:53 ` Christian Brauner 1 sibling, 1 reply; 18+ messages in thread From: Christian Brauner @ 2017-09-10 17:45 UTC (permalink / raw) To: Christian Brauner; +Cc: libc-alpha, fweimer, joseph, schwab 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2 v5] openpty: close slave pty fd on error 2017-09-10 17:45 ` [PATCH 1/2 v5] openpty: close slave pty fd on error Christian Brauner @ 2017-09-20 10:53 ` Christian Brauner 0 siblings, 0 replies; 18+ messages in thread From: Christian Brauner @ 2017-09-20 10:53 UTC (permalink / raw) To: Christian Brauner; +Cc: libc-alpha, fweimer, joseph, schwab 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 > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-09-20 10:53 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-26 14:21 [PATCH 1/2] openpty: close slave pty fd on error 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 ` [PATCH 1/2 v5] openpty: close slave pty fd on error Christian Brauner 2017-09-20 10:53 ` Christian Brauner
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).