Message ID | 20230921162057.3610289-1-josimmon@redhat.com |
---|---|
State | New |
Headers | show |
Series | posix: Deprecate group_member for Linux | expand |
On Thu, Sep 21, 2023 at 12:20:53PM -0400, Joe Simmons-Talbott wrote: > 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. Ping. > --- > posix/Makefile | 3 ++ > posix/group_member.h | 26 ++++++++++++++++ > posix/tst-group_member.c | 41 ++++++++++++++++++++++++++ > posix/unistd.h | 6 ++-- > sysdeps/unix/sysv/linux/group_member.h | 27 +++++++++++++++++ > 5 files changed, 100 insertions(+), 3 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 905cf9fb54..b7fffdd2fe 100644 > --- a/posix/Makefile > +++ b/posix/Makefile > @@ -294,6 +294,7 @@ tests := \ > tst-glob_symlinks \ > tst-gnuglob \ > tst-gnuglob64 \ > + tst-group_member \ > tst-mmap \ > tst-mmap-offset \ > tst-nanosleep \ > @@ -615,6 +616,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..83d5220f2d > --- /dev/null > +++ b/posix/group_member.h > @@ -0,0 +1,26 @@ > +/* 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; > +#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 0477527a60..d73a9eeea2 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 <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..9b24569d4e > --- /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__; > +#endif > + > +#endif /* GROUP_MEMBER_H */ > -- > 2.39.2 >
On Thu, 2023-09-21 at 12:20 -0400, Joe Simmons-Talbott wrote: > diff --git a/sysdeps/unix/sysv/linux/group_member.h b/sysdeps/unix/sysv/linux/group_member.h > new file mode 100644 > index 0000000000..9b24569d4e > --- /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__; Maybe `__attribute_deprecated_msg__ ("may overflow the stack")` is better? And should we have an alternate interface to replace it? > +#endif > + > +#endif /* GROUP_MEMBER_H */
On Wed, Oct 4, 2023 at 10:06 AM Xi Ruoyao <xry111@xry111.site> wrote: > > Maybe `__attribute_deprecated_msg__ ("may overflow the stack")` is > better? And should we have an alternate interface to replace it? > > or maybe just use malloc __fortify_fail ("") if that fails ?
On Wed, Oct 04, 2023 at 09:06:03PM +0800, Xi Ruoyao wrote: > On Thu, 2023-09-21 at 12:20 -0400, Joe Simmons-Talbott wrote: > > diff --git a/sysdeps/unix/sysv/linux/group_member.h b/sysdeps/unix/sysv/linux/group_member.h > > new file mode 100644 > > index 0000000000..9b24569d4e > > --- /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__; > > Maybe `__attribute_deprecated_msg__ ("may overflow the stack")` is > better? And should we have an alternate interface to replace it? I'll add your suggestion in v2. There is opposition to adding a replacement group_member interface since it would require converting from a boolean interface to a tri-state interface which are difficult for programmers. Thanks, Joe
On Wed, Oct 04, 2023 at 11:18:54AM -0300, Cristian Rodríguez wrote: > On Wed, Oct 4, 2023 at 10:06 AM Xi Ruoyao <xry111@xry111.site> wrote: > > > > > Maybe `__attribute_deprecated_msg__ ("may overflow the stack")` is > > better? And should we have an alternate interface to replace it? > > > > > or maybe just use malloc __fortify_fail ("") if that fails ? How does this differ from the previous patch[1] with abort on allocation failure other than the failure message? [1] https://sourceware.org/pipermail/libc-alpha/2023-August/150773.html Thanks, Joe
diff --git a/posix/Makefile b/posix/Makefile index 905cf9fb54..b7fffdd2fe 100644 --- a/posix/Makefile +++ b/posix/Makefile @@ -294,6 +294,7 @@ tests := \ tst-glob_symlinks \ tst-gnuglob \ tst-gnuglob64 \ + tst-group_member \ tst-mmap \ tst-mmap-offset \ tst-nanosleep \ @@ -615,6 +616,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..83d5220f2d --- /dev/null +++ b/posix/group_member.h @@ -0,0 +1,26 @@ +/* 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; +#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 0477527a60..d73a9eeea2 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 <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..9b24569d4e --- /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__; +#endif + +#endif /* GROUP_MEMBER_H */