[PATCH 0/2] posix: Add group_member2 and deprecate group_member

[PATCH 0/2] posix: Add group_member2 and deprecate group_member

[Joe Simmons-Talbott]

Add group_member2 which uses a scratch_buffer rather than alloca and
returns -1 on error and sets errno.  Replace the alloca usage in
group_member with a scratch_buffer and call __fortify_fail when
allocation fails.  Deprecate group_member for Linux.

Joe Simmons-Talbott (2):
  posix/group_member: Add group_member2 with error return.
  posix: Deprecate group_member for Linux

 include/unistd.h                       |  1 +
 posix/Makefile                         |  4 +++
 posix/group_member.c                   | 32 ++++++++++++++++++++
 posix/group_member.h                   | 29 ++++++++++++++++++
 posix/tst-group_member.c               | 41 +++++++++++++++++++++++++
 posix/tst-group_member2.c              | 42 ++++++++++++++++++++++++++
 posix/unistd.h                         |  6 ++--
 sysdeps/unix/sysv/linux/group_member.h | 27 +++++++++++++++++
 8 files changed, 179 insertions(+), 3 deletions(-)
 create mode 100644 posix/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/group_member.h

-- 
2.39.2

[PATCH 1/2] posix/group_member: Add group_member2 with error return.

[Joe Simmons-Talbott]

Because group_member doesn't return an error status and we are trying to
get rid of alloca usage.  Add a new group_member2 which returns -1 and
sets errno on error.  The old group_member will be deprecated in a later
change.
---
 include/unistd.h          |  1 +
 posix/Makefile            |  1 +
 posix/group_member.c      | 32 +++++++++++++++++++++++++++++
 posix/tst-group_member2.c | 42 +++++++++++++++++++++++++++++++++++++++
 posix/unistd.h            |  3 +++
 5 files changed, 79 insertions(+)
 create mode 100644 posix/tst-group_member2.c

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 c36b9d981e..b46ff3259c 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -294,6 +294,7 @@ tests := \
   tst-glob_symlinks \
   tst-gnuglob \
   tst-gnuglob64 \
+  tst-group_member2 \
   tst-mmap \
   tst-mmap-offset \
   tst-nanosleep \
diff --git a/posix/group_member.c b/posix/group_member.c
index 22422b1f9f..6064eb7264 100644
--- a/posix/group_member.c
+++ b/posix/group_member.c
@@ -16,6 +16,8 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#include <errno.h>
+#include <scratch_buffer.h>
 #include <sys/types.h>
 #include <unistd.h>
 #include <stdlib.h>
@@ -47,3 +49,33 @@ __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 buf;
+  scratch_buffer_init (&buf);
+
+  n = __getgroups (0, NULL);
+  if (!scratch_buffer_set_array_size (&buf, n, sizeof (*groups)))
+    {
+      errno = ENOMEM;
+      return -1;
+    }
+  groups = buf.data;
+
+  n = __getgroups (n, groups);
+
+  while (n-- > 0)
+    if (groups[n] == gid)
+      {
+        scratch_buffer_free (&buf);
+        return 1;
+      }
+
+  scratch_buffer_free (&buf);
+  return 0;
+}
+weak_alias (__group_member2, group_member2)
diff --git a/posix/tst-group_member2.c b/posix/tst-group_member2.c
new file mode 100644
index 0000000000..4a360c71da
--- /dev/null
+++ b/posix/tst-group_member2.c
@@ -0,0 +1,42 @@
+/* Basic tests for group_memmber2.
+   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_member2 (groups[n]));
+
+  return EXIT_SUCCESS;
+}
+
+#include <support/test-driver.c>
+
diff --git a/posix/unistd.h b/posix/unistd.h
index 0477527a60..96816b186d 100644
--- a/posix/unistd.h
+++ b/posix/unistd.h
@@ -713,6 +713,9 @@ extern int getgroups (int __size, __gid_t __list[]) __THROW __wur
 #ifdef	__USE_GNU
 /* Return nonzero iff the calling process is in group GID.  */
 extern int group_member (__gid_t __gid) __THROW;
+/* Return nonzero iff the calling process is in group GID.  Return
+   -1 on error and set errno.  */
+extern int group_member2 (__gid_t __gid) __THROW;
 #endif
 
 /* Set the user ID of the calling process to UID.
-- 
2.39.2

[PATCH 2/2] posix: Deprecate group_member for Linux

[Joe Simmons-Talbott]

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.  Thus deprecate group_member.  Add
a testcase.
---
 posix/Makefile                         |  3 ++
 posix/group_member.h                   | 29 ++++++++++++++++++
 posix/tst-group_member.c               | 41 ++++++++++++++++++++++++++
 posix/unistd.h                         |  9 ++----
 sysdeps/unix/sysv/linux/group_member.h | 27 +++++++++++++++++
 5 files changed, 103 insertions(+), 6 deletions(-)
 create mode 100644 posix/group_member.h
 create mode 100644 posix/tst-group_member.c
 create mode 100644 sysdeps/unix/sysv/linux/group_member.h

diff --git a/posix/Makefile b/posix/Makefile
index b46ff3259c..09308d4568 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -295,6 +295,7 @@ tests := \
   tst-gnuglob \
   tst-gnuglob64 \
   tst-group_member2 \
+  tst-group_member \
   tst-mmap \
   tst-mmap-offset \
   tst-nanosleep \
@@ -616,6 +617,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.h b/posix/group_member.h
new file mode 100644
index 0000000000..cc3c3ba603
--- /dev/null
+++ b/posix/group_member.h
@@ -0,0 +1,29 @@
+/* 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 _GROUP_MEMBER_H
+#define _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;
+/* Return nonzero iff the calling process is in group GID.  Return
+   -1 on error and set errno.  */
+extern int group_member2 (__gid_t __gid) __THROW;
+#endif
+
+#endif /* GROUP_MEMBER_H */
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/unistd.h b/posix/unistd.h
index 96816b186d..d73a9eeea2 100644
--- a/posix/unistd.h
+++ b/posix/unistd.h
@@ -710,13 +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;
-/* Return nonzero iff the calling process is in group GID.  Return
-   -1 on error and set errno.  */
-extern int group_member2 (__gid_t __gid) __THROW;
-#endif
+# include <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/unix/sysv/linux/group_member.h b/sysdeps/unix/sysv/linux/group_member.h
new file mode 100644
index 0000000000..97a9f30dfe
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/group_member.h
@@ -0,0 +1,27 @@
+/* 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 _GROUP_MEMBER_H
+#define _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 /* GROUP_MEMBER_H */
-- 
2.39.2

Re: [PATCH 0/2] posix: Add group_member2 and deprecate group_member

[Joseph Myers]

On Thu, 12 Oct 2023, Joe Simmons-Talbott wrote:

> Add group_member2 which uses a scratch_buffer rather than alloca and
> returns -1 on error and sets errno.  Replace the alloca usage in
> group_member with a scratch_buffer and call __fortify_fail when
> allocation fails.  Deprecate group_member for Linux.

How was this tested?  It's missing a Versions update so I wouldn't have 
expected the tests to link (also missing an addition of group_member.h to 
the list of installed headers in posix/Makefile, abilist updates for all 
architectures and a NEWS update for the new user-visible function; since 
group_member isn't documented in the manual, it's probably reasonable that 
it doesn't add documentation for group_member2).

If group_member.h isn't intended to be a public API itself (if user code 
isn't intended to do #include <group_member.h>) then it should be 
bits/group_member.h instead with appropriate tests that it's only included 
from <unistd.h> and not directly from user code.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 0/2] posix: Add group_member2 and deprecate group_member

[Joe Simmons-Talbott]

On Thu, Oct 12, 2023 at 04:59:51PM +0000, Joseph Myers wrote:
> On Thu, 12 Oct 2023, Joe Simmons-Talbott wrote:
> 
> > Add group_member2 which uses a scratch_buffer rather than alloca and
> > returns -1 on error and sets errno.  Replace the alloca usage in
> > group_member with a scratch_buffer and call __fortify_fail when
> > allocation fails.  Deprecate group_member for Linux.
> 
> How was this tested?  It's missing a Versions update so I wouldn't have 
> expected the tests to link (also missing an addition of group_member.h to 
> the list of installed headers in posix/Makefile, abilist updates for all 
> architectures and a NEWS update for the new user-visible function; since 
> group_member isn't documented in the manual, it's probably reasonable that 
> it doesn't add documentation for group_member2).
> 
> If group_member.h isn't intended to be a public API itself (if user code 
> isn't intended to do #include <group_member.h>) then it should be 
> bits/group_member.h instead with appropriate tests that it's only included 
> from <unistd.h> and not directly from user code.

Sorry, this was from me combining two git branches and one wasn't ready
to submit yet.  Thanks for pointing out what was missing.  I'll post a
v2 with the missing parts soon.

What are folk's thoughts on whether to make group_member.h public or
not?  I don't have strong feelings either way.

Thanks,
Joe