Message ID | 20231017115942.1425796-1-josimmon@redhat.com |
---|---|
State | New |
Headers | show |
Series | [v3] posix: Deprecate group_member for Linux | expand |
On Okt 17 2023, Joe Simmons-Talbott wrote: > +#ifndef _GROUP_MEMBER_H > +#define _GROUP_MEMBER_H 1 That should be _BITS_GROUP_MEMBER_H.
* 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 value. Thus deprecate group_member. > Add a testcase. > --- > Changes to v2: > * Move the linux group_member.h to the bits directory > * Include the correct group_member.h in posix/unistd.h > > NEWS | 5 ++- > bits/group_member.h | 31 ++++++++++++++++ > posix/Makefile | 4 ++ > posix/tst-group_member.c | 41 +++++++++++++++++++++ > posix/unistd.h | 6 +-- > sysdeps/unix/sysv/linux/bits/group_member.h | 32 ++++++++++++++++ > 6 files changed, 115 insertions(+), 4 deletions(-) > create mode 100644 bits/group_member.h > create mode 100644 posix/tst-group_member.c > create mode 100644 sysdeps/unix/sysv/linux/bits/group_member.h There are __group_member calls in sysdeps/posix/euidaccess.c and sysdeps/unix/sysv/linux/faccessat.c. We can return ENOMEM in those cases. Furthermore, euidaccess should really be layered on top of faccessat (and use kernel support for AT_EACCESS if available). Those are separate changes, but necessary to avoid exposing the alloca path to non-deprecated functionality. Thanks, Florian
On Tue, Oct 17, 2023 at 04:00:00PM +0200, Florian Weimer wrote: > * 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 value. Thus deprecate group_member. > > Add a testcase. > > --- > > Changes to v2: > > * Move the linux group_member.h to the bits directory > > * Include the correct group_member.h in posix/unistd.h > > > > NEWS | 5 ++- > > bits/group_member.h | 31 ++++++++++++++++ > > posix/Makefile | 4 ++ > > posix/tst-group_member.c | 41 +++++++++++++++++++++ > > posix/unistd.h | 6 +-- > > sysdeps/unix/sysv/linux/bits/group_member.h | 32 ++++++++++++++++ > > 6 files changed, 115 insertions(+), 4 deletions(-) > > create mode 100644 bits/group_member.h > > create mode 100644 posix/tst-group_member.c > > create mode 100644 sysdeps/unix/sysv/linux/bits/group_member.h > > There are __group_member calls in sysdeps/posix/euidaccess.c and > sysdeps/unix/sysv/linux/faccessat.c. We can return ENOMEM in those > cases. Furthermore, euidaccess should really be layered on top of > faccessat (and use kernel support for AT_EACCESS if available). > > Those are separate changes, but necessary to avoid exposing the alloca > path to non-deprecated functionality. Thanks for pointing those out. So are you suggesting that I perhaps do need to create an internal only __group_member2 that uses malloc or a scratch_buffer? Thanks, Joe
* Joe Simmons-Talbott: >> There are __group_member calls in sysdeps/posix/euidaccess.c and >> sysdeps/unix/sysv/linux/faccessat.c. We can return ENOMEM in those >> cases. Furthermore, euidaccess should really be layered on top of >> faccessat (and use kernel support for AT_EACCESS if available). >> >> Those are separate changes, but necessary to avoid exposing the alloca >> path to non-deprecated functionality. > > Thanks for pointing those out. So are you suggesting that I perhaps do > need to create an internal only __group_member2 that uses malloc or a > scratch_buffer? Yes, I think that would be prudent. Thanks, Florian
On Tue, Dec 12, 2023 at 12:51:28PM +0100, Florian Weimer wrote: > * Joe Simmons-Talbott: > > >> There are __group_member calls in sysdeps/posix/euidaccess.c and > >> sysdeps/unix/sysv/linux/faccessat.c. We can return ENOMEM in those > >> cases. Furthermore, euidaccess should really be layered on top of > >> faccessat (and use kernel support for AT_EACCESS if available). > >> > >> Those are separate changes, but necessary to avoid exposing the alloca > >> path to non-deprecated functionality. > > > > Thanks for pointing those out. So are you suggesting that I perhaps do > > need to create an internal only __group_member2 that uses malloc or a > > scratch_buffer? > > Yes, I think that would be prudent. I've added an internal only implentation as __group_member2 that uses a scratch_buffer. I updated sysdeps/unix/sysv/linux/faccessat.c and sysdeps/posix/euidaccess.c to use __group_member2 and return -1 on memory allocation failure. I also added a testcase to test __group_member2. This is all in v6 of the patch. [1] Thanks, Joe [1] https://sourceware.org/pipermail/libc-alpha/2023-December/153266.html
diff --git a/NEWS b/NEWS index cc4b81f0ac..eadd8c5981 100644 --- a/NEWS +++ b/NEWS @@ -40,7 +40,10 @@ Major new features: Deprecated and removed features, and other changes affecting compatibility: - [Add deprecations, removals and changes affecting compatibility here] +* 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: diff --git a/bits/group_member.h b/bits/group_member.h new file mode 100644 index 0000000000..a8035c4ed7 --- /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 _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; +#endif + +#endif /* GROUP_MEMBER_H */ diff --git a/posix/Makefile b/posix/Makefile index c36b9d981e..d3e3c08fd1 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 \ @@ -294,6 +295,7 @@ tests := \ tst-glob_symlinks \ tst-gnuglob \ tst-gnuglob64 \ + tst-group_member \ tst-mmap \ tst-mmap-offset \ tst-nanosleep \ @@ -615,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/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 0477527a60..b65d4c63d0 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/unix/sysv/linux/bits/group_member.h b/sysdeps/unix/sysv/linux/bits/group_member.h new file mode 100644 index 0000000000..8b47c1e379 --- /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 _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 */