Add the --with-tty-gid configure option [BZ #24941]

Message ID 87h862kdyx.fsf@oldenburg2.str.redhat.com
State Superseded
Headers

Commit Message

Florian Weimer Aug. 27, 2019, 12:08 p.m. UTC
  2019-08-27  Florian Weimer  <fweimer@redhat.com>

	[BZ #24941]
	Add the --with-tty-gid configure option.
	* config.make.in (define-tty-gid-cflags): New variable.
	* configure.ac (--with-tty-gid): New option.
	* manual/install.texi (Configuring and compiling): Document
	--with-tty-gid.
	* sysdeps/unix/Makefile [$(subdir) == login] (CFLAGS-grantpt.c):
	Add $(define-tty-gid-cflags).
	* sysdeps/unix/grantpt.c (grantpt): Use TTY_GID constant for gid
	if defined.
	* configure, INSTALL: Regeneration.
  

Comments

Gabriel F. T. Gomes Oct. 31, 2019, 8:41 p.m. UTC | #1
Hi, Florian,

Thanks for, in the bug report [1], helping me understand the rationale for
this patch (the problem was well described here, anyway, I had just failed
to notice the details).

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=24941#c3

On Tue, 27 Aug 2019, Florian Weimer wrote:

>2019-08-27  Florian Weimer  <fweimer@redhat.com>
>
>	[BZ #24941]
>	Add the --with-tty-gid configure option.
>	* config.make.in (define-tty-gid-cflags): New variable.
>	* configure.ac (--with-tty-gid): New option.
>	* manual/install.texi (Configuring and compiling): Document
>	--with-tty-gid.
>	* sysdeps/unix/Makefile [$(subdir) == login] (CFLAGS-grantpt.c):
>	Add $(define-tty-gid-cflags).
>	* sysdeps/unix/grantpt.c (grantpt): Use TTY_GID constant for gid
>	if defined.
>	* configure, INSTALL: Regeneration.

I thinks this needs a new commit message, preferably one that summarizes
why this patch is useful...  Something in the lines of:

When, in multi-threaded applications, a forked process tries to call
grantpt, it could hang in an internal NSS lock (__nss_database_lookup2),
if the fork happened while NSS was used in another thread.  This patch
adds the --with-tty-gid configure option, which avoids the hang, by
avoiding calls to NSS functions.


Other than that, the patch looks good to me.

Reviewed-by: Gabriel F. T. Gomes <gabrielftg@linux.ibm.com>

Thanks


>+@item --with-tty-gid=@var{GID}
>+Use @var{GID} as the value for the group ID of the @samp{tty} group.
>+This enables a more efficient implementation of the @code{grantpt}
>+function, and the function will also work reliably in a forked
>+subprocess of a multi-threaded process.

OK.

>+#ifdef TTY_GID
>+  /* TTY_GID is set by the --with-tty-gid configure option.  */
>+  enum { gid = TTY_GID };
>+#else
>   static int tty_gid = -1;
>   if (__glibc_unlikely (tty_gid == -1))
>     {
>@@ -154,6 +158,7 @@ grantpt (int fd)
> 	tty_gid = p->gr_gid;
>     }
>   gid_t gid = tty_gid == -1 ? __getgid () : tty_gid;
>+#endif

OK.
  
Florian Weimer May 27, 2020, 10:02 a.m. UTC | #2
I'm withdrawing this patch.  I have a rewrite of grantpt which fixes
this as well.

Thanks,
Florian
  

Patch

diff --git a/INSTALL b/INSTALL
index 16987cd048..48bbf7d613 100644
--- a/INSTALL
+++ b/INSTALL
@@ -106,6 +106,12 @@  if 'CFLAGS' is specified it must enable optimization.  For example:
      particular case and potentially change debugging information and
      metadata only).
 
+'--with-tty-gid=GID'
+     Use GID as the value for the group ID of the 'tty' group.  This
+     enables a more efficient implementation of the 'grantpt' function,
+     and the function will also work reliably in a forked subprocess of
+     a multi-threaded process.
+
 '--disable-shared'
      Don't build shared libraries even if it is possible.  Not all
      systems support shared libraries; you need ELF support and
diff --git a/NEWS b/NEWS
index a64b89986a..ac302f739b 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,11 @@  Major new features:
   18661-1:2014 and TS 18661-3:2015 as amended by the resolution of
   Clarification Request 13 to TS 18661-3.
 
+* The GNU C Library can now be configured with the option --with-tty-gid=5,
+  hard-coding the GID of group "tty" to 5 (the value on most GNU/Linux
+  systems).  This enables a more efficient implementation of grantpt, which
+  also works reliably in a forked subprocess of a multi-threaded process.
+
 Deprecated and removed features, and other changes affecting compatibility:
 
 * The totalorder and totalordermag functions, and the corresponding
diff --git a/config.make.in b/config.make.in
index 2fed3da773..b04c1ece1b 100644
--- a/config.make.in
+++ b/config.make.in
@@ -112,6 +112,7 @@  BUILD_CC = @BUILD_CC@
 CFLAGS = @CFLAGS@
 CPPFLAGS-config = @CPPFLAGS@
 CPPUNDEFS = @CPPUNDEFS@
+define-tty-gid-cflags = @define_tty_gid_cflags@
 extra-nonshared-cflags = @extra_nonshared_cflags@
 ASFLAGS-config = @ASFLAGS_config@
 AR = @AR@
diff --git a/configure b/configure
index c773c487b5..687c6a135d 100755
--- a/configure
+++ b/configure
@@ -685,6 +685,7 @@  force_install
 bindnow
 hardcoded_path_in_tests
 enable_timezone_tools
+define_tty_gid_cflags
 extra_nonshared_cflags
 use_default_link
 sysheaders
@@ -765,6 +766,7 @@  with_selinux
 with_headers
 with_default_link
 with_nonshared_cflags
+with_tty_gid
 enable_sanity_checks
 enable_shared
 enable_profile
@@ -1487,6 +1489,7 @@  Optional Packages:
   --with-default-link     do not use explicit linker scripts
   --with-nonshared-cflags=CFLAGS
                           build nonshared libraries with additional CFLAGS
+  --with-tty-gid=GID      set the ID of the tty group to GID
   --with-cpu=CPU          select code for CPU variant
 
 Some influential environment variables:
@@ -3354,6 +3357,16 @@  fi
 
 
 
+
+# Check whether --with-tty-gid was given.
+if test "${with_tty_gid+set}" = set; then :
+  withval=$with_tty_gid; define_tty_gid_cflags=-DTTY_GID=$withval
+else
+  define_tty_gid_cflags=
+fi
+
+
+
 # Check whether --enable-sanity-checks was given.
 if test "${enable_sanity_checks+set}" = set; then :
   enableval=$enable_sanity_checks; enable_sanity=$enableval
diff --git a/configure.ac b/configure.ac
index 598ba6c4ae..483fe3d211 100644
--- a/configure.ac
+++ b/configure.ac
@@ -162,6 +162,13 @@  AC_ARG_WITH([nonshared-cflags],
 	    [extra_nonshared_cflags=])
 AC_SUBST(extra_nonshared_cflags)
 
+AC_ARG_WITH([tty-gid],
+	    AC_HELP_STRING([--with-tty-gid=GID],
+			   [set the ID of the tty group to GID]),
+	    [define_tty_gid_cflags=-DTTY_GID=$withval],
+	    [define_tty_gid_cflags=])
+AC_SUBST(define_tty_gid_cflags)
+
 AC_ARG_ENABLE([sanity-checks],
 	      AC_HELP_STRING([--disable-sanity-checks],
 			     [really do not use threads (should not be used except in special situations) @<:@default=yes@:>@]),
diff --git a/manual/install.texi b/manual/install.texi
index b2d569ac5a..917f41aed9 100644
--- a/manual/install.texi
+++ b/manual/install.texi
@@ -131,6 +131,12 @@  that the objects in @file{libc_nonshared.a} are compiled with this flag
 (although this will not affect the generated code in this particular
 case and potentially change debugging information and metadata only).
 
+@item --with-tty-gid=@var{GID}
+Use @var{GID} as the value for the group ID of the @samp{tty} group.
+This enables a more efficient implementation of the @code{grantpt}
+function, and the function will also work reliably in a forked
+subprocess of a multi-threaded process.
+
 @c disable static doesn't work currently
 @c @item --disable-static
 @c Don't build static libraries.  Static libraries aren't that useful these
diff --git a/sysdeps/unix/Makefile b/sysdeps/unix/Makefile
index 93496480e9..f02351da5f 100644
--- a/sysdeps/unix/Makefile
+++ b/sysdeps/unix/Makefile
@@ -109,4 +109,8 @@  $(common-objpfx)s-%.d: $(..)sysdeps/unix/s-%.S \
 
 postclean-generated += sysd-syscalls
 
-endif
+endif # $(subdir) == misc
+
+ifeq (login,$(subdir))
+CFLAGS-grantpt.c += $(define-tty-gid-cflags)
+endif # $(subdir) == login
diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c
index 3214ab06de..57a29347d3 100644
--- a/sysdeps/unix/grantpt.c
+++ b/sysdeps/unix/grantpt.c
@@ -135,6 +135,10 @@  grantpt (int fd)
 	goto helper;
     }
 
+#ifdef TTY_GID
+  /* TTY_GID is set by the --with-tty-gid configure option.  */
+  enum { gid = TTY_GID };
+#else
   static int tty_gid = -1;
   if (__glibc_unlikely (tty_gid == -1))
     {
@@ -154,6 +158,7 @@  grantpt (int fd)
 	tty_gid = p->gr_gid;
     }
   gid_t gid = tty_gid == -1 ? __getgid () : tty_gid;
+#endif
 
 #if HAVE_PT_CHOWN
   /* Make sure the group of the device is that special group.  */