diff mbox series

[v8] posix: Deprecate group_member for Linux

Message ID 20240326130649.3769408-1-josimmon@redhat.com
State New
Headers show
Series [v8] posix: Deprecate group_member for Linux | expand

Commit Message

Joe Simmons-Talbott March 26, 2024, 1:06 p.m. UTC
The alloca usage in group_member could lead to stack overflow on Linux.
Removing the alloca usage would require group_member to handle the error
condition where memory could not be allocated and that cannot be done
since group_member returns a boolean value.  Thus deprecate group_member.
Add an internal only implementation of __group_member2 using a
scratch_buffer and return -1 for memory allocation errors.  Use
__group_member2 for in place of __group_member internally.  Add testcases
for both group_member and __group_member2.
---
Changes to v7:
* rebased to latest master.

Changes to v6:
* Use the intial scratch_buffer size as the starting point for
  determining how much space is needed to store the group list.
* Call getgroups() with a zero size and set the scratch_buffer size
  based on the returned number of groups.

Changes to v5:
* Add __group_member2 and use it internally in the place of the now
  deprecated group_member.
* Add a testcase for __group_member2.

Changes to v4:
* Rebase onto latest commit.

Changes to v3:
* Fix include guards to match file location _BITS_GROUP_MEMBER_H
* Fix indentation of preprocessor directives

Changes to v2:
* Move the linux group_member.h to the bits directory
* Include the correct group_member.h in posix/unistd.h

Changes to v1:
* Add NEWS entry
* Move group_member.h to bits/group_member.h
* Include bits/group_member.h in installed headers
* Add tests to group_member.h files to only be included from unistd.h

 NEWS                                        |  4 ++
 bits/group_member.h                         | 31 +++++++++++++++
 include/unistd.h                            |  1 +
 posix/Makefile                              |  8 ++++
 posix/group_member.c                        | 35 +++++++++++++++++
 posix/tst-group_member.c                    | 41 ++++++++++++++++++++
 posix/tst-group_member2.c                   | 43 +++++++++++++++++++++
 posix/unistd.h                              |  6 +--
 sysdeps/posix/euidaccess.c                  |  9 ++++-
 sysdeps/unix/sysv/linux/bits/group_member.h | 32 +++++++++++++++
 sysdeps/unix/sysv/linux/faccessat.c         |  8 +++-
 11 files changed, 212 insertions(+), 6 deletions(-)
 create mode 100644 bits/group_member.h
 create mode 100644 posix/tst-group_member.c
 create mode 100644 posix/tst-group_member2.c
 create mode 100644 sysdeps/unix/sysv/linux/bits/group_member.h

Comments

Paul Eggert March 26, 2024, 5 p.m. UTC | #1
The change to faccessat.c (quoted below) is problematic, as it could 
entail multiple calls to getegid or to getgid, with inconsistent results 
if some other thread is changing the group id of the current process. 
Instead, I suggest declaring "granted" via a simple "int granted;" and 
then using an if statement to initialize it, so that the logic is not 
duplicated and the multiple calls eliminated.

> --- a/sysdeps/unix/sysv/linux/faccessat.c
> +++ b/sysdeps/unix/sysv/linux/faccessat.c
> @@ -59,11 +59,17 @@ __faccessat (int fd, const char *file, int mode, int flag)
>   		   || (stats.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))))
>       return 0;
>   
> +  int gm = __group_member2 (stats.st_gid);
> +  if (uid != stats.st_uid &&
> +      (stats.st_gid != ((flag & AT_EACCESS) ? __getegid () : __getgid ())))
> +    if (gm == -1)
> +      return -1;
> +
>     int granted = (uid == stats.st_uid
>   		 ? (unsigned int) (stats.st_mode & (mode << 6)) >> 6
>   		 : (stats.st_gid == ((flag & AT_EACCESS)
>   				     ? __getegid () : __getgid ())
> -		    || __group_member (stats.st_gid))
> +		    || gm)
>   		 ? (unsigned int) (stats.st_mode & (mode << 3)) >> 3
>   		 : (stats.st_mode & mode));
>
Joe Simmons-Talbott March 27, 2024, 11:40 a.m. UTC | #2
On Tue, Mar 26, 2024 at 1:08 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> The change to faccessat.c (quoted below) is problematic, as it could
> entail multiple calls to getegid or to getgid, with inconsistent results
> if some other thread is changing the group id of the current process.
> Instead, I suggest declaring "granted" via a simple "int granted;" and
> then using an if statement to initialize it, so that the logic is not
> duplicated and the multiple calls eliminated.
>

Thanks for the review.  I've posted a v9[1] that removes the
duplicated calls to __getegid () and __getgid () and converted the
logic from nested ternary operators to an if/else structure.

Thanks,
Joe

[1] https://sourceware.org/pipermail/libc-alpha/2024-March/155607.html

> > --- a/sysdeps/unix/sysv/linux/faccessat.c
> > +++ b/sysdeps/unix/sysv/linux/faccessat.c
> > @@ -59,11 +59,17 @@ __faccessat (int fd, const char *file, int mode, int flag)
> >                  || (stats.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))))
> >       return 0;
> >
> > +  int gm = __group_member2 (stats.st_gid);
> > +  if (uid != stats.st_uid &&
> > +      (stats.st_gid != ((flag & AT_EACCESS) ? __getegid () : __getgid ())))
> > +    if (gm == -1)
> > +      return -1;
> > +
> >     int granted = (uid == stats.st_uid
> >                ? (unsigned int) (stats.st_mode & (mode << 6)) >> 6
> >                : (stats.st_gid == ((flag & AT_EACCESS)
> >                                    ? __getegid () : __getgid ())
> > -                 || __group_member (stats.st_gid))
> > +                 || gm)
> >                ? (unsigned int) (stats.st_mode & (mode << 3)) >> 3
> >                : (stats.st_mode & mode));
> >
>
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index da4b2223e9..82e1c43306 100644
--- a/NEWS
+++ b/NEWS
@@ -141,6 +141,10 @@  Deprecated and removed features, and other changes affecting compatibility:
 
 * The ia64*-*-linux-gnu configurations are no longer supported.
 
+* Deprecated group_member on Linux as it uses alloca to allocate a large
+  buffer and has no capability for indicating failure for other memory
+  allocations.
+
 Changes to build and runtime requirements:
 
 * Building on LoongArch requires at a minimum binutils 2.41 for vector
diff --git a/bits/group_member.h b/bits/group_member.h
new file mode 100644
index 0000000000..7c43e7ee06
--- /dev/null
+++ b/bits/group_member.h
@@ -0,0 +1,31 @@ 
+/* group_member declaration
+   Copyright (C) 2023 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef _UNISTD_H
+# error "Never use <bits/group_member.h> directly; include <unistd.h> instead."
+#endif
+
+#ifndef _BITS_GROUP_MEMBER_H
+# define _BITS_GROUP_MEMBER_H 1
+
+# ifdef	__USE_GNU
+/* Return nonzero iff the calling process is in group GID.  */
+extern int group_member (__gid_t __gid) __THROW;
+# endif
+
+#endif /* _BITS_GROUP_MEMBER_H */
diff --git a/include/unistd.h b/include/unistd.h
index e241603b81..39d5bda372 100644
--- a/include/unistd.h
+++ b/include/unistd.h
@@ -131,6 +131,7 @@  extern __gid_t __getegid (void) attribute_hidden;
 extern int __getgroups (int __size, __gid_t __list[]) attribute_hidden;
 libc_hidden_proto (__getpgid)
 extern int __group_member (__gid_t __gid) attribute_hidden;
+extern int __group_member2 (__gid_t __gid) attribute_hidden;
 extern int __setuid (__uid_t __uid);
 extern int __setreuid (__uid_t __ruid, __uid_t __euid);
 extern int __setgid (__gid_t __gid);
diff --git a/posix/Makefile b/posix/Makefile
index a1e84853a8..b71d6c8750 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -29,6 +29,7 @@  headers := \
   bits/getopt_core.h \
   bits/getopt_ext.h \
   bits/getopt_posix.h \
+  bits/group_member.h \
   bits/local_lim.h \
   bits/mman_ext.h \
   bits/posix1_lim.h \
@@ -291,6 +292,7 @@  tests := \
   tst-glob_symlinks \
   tst-gnuglob \
   tst-gnuglob64 \
+  tst-group_member \
   tst-mmap \
   tst-mmap-offset \
   tst-nanosleep \
@@ -479,6 +481,10 @@  tests-special += \
   # tests-special
 endif
 
+# This test calls __group_member2 directly, which is not exported from glibc.
+tests-internal += tst-group_member2
+tests-static += tst-group_member2
+
 include ../Rules
 
 ifeq ($(run-built-tests),yes)
@@ -606,6 +612,8 @@  bug-glob1-ARGS = "$(objpfx)"
 tst-execvp3-ARGS = --test-dir=$(objpfx)
 CFLAGS-tst-spawn3.c += -DOBJPFX=\"$(objpfx)\"
 
+CFLAGS-tst-group_member.c += -Wno-error=deprecated-declarations
+
 # Test voluntarily overflows struct dirent
 CFLAGS-bug-glob2.c += $(no-fortify-source)
 
diff --git a/posix/group_member.c b/posix/group_member.c
index 9d68f57a68..bb92f4d631 100644
--- a/posix/group_member.c
+++ b/posix/group_member.c
@@ -18,6 +18,7 @@ 
 
 #include <sys/types.h>
 #include <unistd.h>
+#include <scratch_buffer.h>
 #include <stdlib.h>
 #include <limits.h>
 
@@ -47,3 +48,37 @@  __group_member (gid_t gid)
   return 0;
 }
 weak_alias (__group_member, group_member)
+
+int
+__group_member2 (gid_t gid)
+{
+  int n;
+  gid_t *groups;
+  struct scratch_buffer sbuf;
+  scratch_buffer_init (&sbuf);
+  groups = sbuf.data;
+
+  do
+    {
+      n = __getgroups (0, NULL);
+      if (n > sbuf.length)
+	{
+	  if (!scratch_buffer_set_array_size (&sbuf, sizeof (*groups), n))
+	    return -1;
+	  groups = sbuf.data;
+	}
+
+      n = __getgroups (n, groups);
+    }
+  while (n > sbuf.length);
+
+  while (n-- > 0)
+    if (groups[n] == gid)
+      {
+	scratch_buffer_free (&sbuf);
+        return 1;
+      }
+
+  scratch_buffer_free (&sbuf);
+  return 0;
+}
diff --git a/posix/tst-group_member.c b/posix/tst-group_member.c
new file mode 100644
index 0000000000..7f70841832
--- /dev/null
+++ b/posix/tst-group_member.c
@@ -0,0 +1,41 @@ 
+/* Basic tests for group_member.
+   Copyright (C) 2023 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <alloca.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <support/check.h>
+
+static int do_test (void)
+{
+  int n;
+  gid_t *groups;
+
+  n = getgroups (0, NULL);
+  groups = alloca (n * sizeof (*groups));
+  n = getgroups (n, groups);
+
+  while (n-- > 0)
+    TEST_COMPARE (1, group_member(groups[n]));
+
+  return EXIT_SUCCESS;
+}
+
+#include <support/test-driver.c>
diff --git a/posix/tst-group_member2.c b/posix/tst-group_member2.c
new file mode 100644
index 0000000000..ee448c578a
--- /dev/null
+++ b/posix/tst-group_member2.c
@@ -0,0 +1,43 @@ 
+/* Basic tests for group_member.
+   Copyright (C) 2023 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <alloca.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <posix/unistd.h>
+
+#include <support/check.h>
+
+extern int __group_member2 (__gid_t __gid);
+
+static int do_test (void)
+{
+  int n;
+  gid_t *groups;
+
+  n = getgroups (0, NULL);
+  groups = alloca (n * sizeof (*groups));
+  n = getgroups (n, groups);
+
+  while (n-- > 0)
+    TEST_COMPARE (1, __group_member2(groups[n]));
+
+  return EXIT_SUCCESS;
+}
+
+#include <support/test-driver.c>
diff --git a/posix/unistd.h b/posix/unistd.h
index 54d7d7527e..411de1d6d4 100644
--- a/posix/unistd.h
+++ b/posix/unistd.h
@@ -710,10 +710,10 @@  extern __gid_t getegid (void) __THROW;
    of its supplementary groups in LIST and return the number written.  */
 extern int getgroups (int __size, __gid_t __list[]) __THROW __wur
     __fortified_attr_access (__write_only__, 2, 1);
+
 #ifdef	__USE_GNU
-/* Return nonzero iff the calling process is in group GID.  */
-extern int group_member (__gid_t __gid) __THROW;
-#endif
+# include <bits/group_member.h>
+#endif 
 
 /* Set the user ID of the calling process to UID.
    If the calling process is the super-user, set the real
diff --git a/sysdeps/posix/euidaccess.c b/sysdeps/posix/euidaccess.c
index 4c5c2220bd..0c19021a30 100644
--- a/sysdeps/posix/euidaccess.c
+++ b/sysdeps/posix/euidaccess.c
@@ -81,7 +81,7 @@  extern int errno;
 
 #ifdef _LIBC
 
-# define group_member __group_member
+# define group_member __group_member2
 # define euidaccess __euidaccess
 
 #else
@@ -167,9 +167,14 @@  euidaccess (const char *path, int mode)
 		    || (stats.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))))
     return 0;
 
+  int gm = group_member (stats.st_gid);
+  if (euid != stats.st_uid && egid != stats.st_gid)
+    if (gm == -1)
+      return -1;
+
   if (euid == stats.st_uid)
     granted = (unsigned int) (stats.st_mode & (mode << 6)) >> 6;
-  else if (egid == stats.st_gid || group_member (stats.st_gid))
+  else if (egid == stats.st_gid || gm)
     granted = (unsigned int) (stats.st_mode & (mode << 3)) >> 3;
   else
     granted = (stats.st_mode & mode);
diff --git a/sysdeps/unix/sysv/linux/bits/group_member.h b/sysdeps/unix/sysv/linux/bits/group_member.h
new file mode 100644
index 0000000000..0dd9505c76
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/bits/group_member.h
@@ -0,0 +1,32 @@ 
+/* group_member declaration
+   Copyright (C) 2023 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef _UNISTD_H
+# error "Never use <bits/group_member.h> directly; include <unistd.h> instead."
+#endif
+
+#ifndef _BITS_GROUP_MEMBER_H
+# define _BITS_GROUP_MEMBER_H 1
+
+# ifdef	__USE_GNU
+/* Return nonzero iff the calling process is in group GID.  Deprecated */
+extern int group_member (__gid_t __gid) __THROW
+	__attribute_deprecated_msg__ ("may overflow the stack");
+# endif
+
+#endif /* _BITS_GROUP_MEMBER_H */
diff --git a/sysdeps/unix/sysv/linux/faccessat.c b/sysdeps/unix/sysv/linux/faccessat.c
index 2fa57fd63d..c6fb73aa84 100644
--- a/sysdeps/unix/sysv/linux/faccessat.c
+++ b/sysdeps/unix/sysv/linux/faccessat.c
@@ -59,11 +59,17 @@  __faccessat (int fd, const char *file, int mode, int flag)
 		   || (stats.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))))
     return 0;
 
+  int gm = __group_member2 (stats.st_gid);
+  if (uid != stats.st_uid &&
+      (stats.st_gid != ((flag & AT_EACCESS) ? __getegid () : __getgid ())))
+    if (gm == -1)
+      return -1;
+
   int granted = (uid == stats.st_uid
 		 ? (unsigned int) (stats.st_mode & (mode << 6)) >> 6
 		 : (stats.st_gid == ((flag & AT_EACCESS)
 				     ? __getegid () : __getgid ())
-		    || __group_member (stats.st_gid))
+		    || gm)
 		 ? (unsigned int) (stats.st_mode & (mode << 3)) >> 3
 		 : (stats.st_mode & mode));