[v8] posix: Deprecate group_member for Linux
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
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
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));
>
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));
> >
>
@@ -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
new file mode 100644
@@ -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 */
@@ -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);
@@ -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)
@@ -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;
+}
new file mode 100644
@@ -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>
new file mode 100644
@@ -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>
@@ -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
@@ -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);
new file mode 100644
@@ -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 */
@@ -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));