[PATCH v2] group_member: Get rid of unbounded alloca.

[PATCH v2] group_member: Get rid of unbounded alloca.

[Joe Simmons-Talbott]

Replace a large alloca call with a scratch_buffer to avoid potential stack
overflow.  Because group_member doesn't return an error indicator abort if
we are unable to allocate memory.  Add a testcase.

Checked on x86_64-linux-gnu.
---
Changes to v1:
 * Update commit message and fix typo.

 posix/Makefile           |  1 +
 posix/group_member.c     | 27 +++++++++++++++-----------
 posix/tst-group_member.c | 41 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 11 deletions(-)
 create mode 100644 posix/tst-group_member.c

diff --git a/posix/Makefile b/posix/Makefile
index 3d368b91f6..7491ee8917 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -293,6 +293,7 @@ tests := \
   tst-glob_symlinks \
   tst-gnuglob \
   tst-gnuglob64 \
+  tst-group_member \
   tst-mmap \
   tst-mmap-offset \
   tst-nanosleep \
diff --git a/posix/group_member.c b/posix/group_member.c
index 22422b1f9f..42a4adb9b4 100644
--- a/posix/group_member.c
+++ b/posix/group_member.c
@@ -16,9 +16,10 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#include <scratch_buffer.h>
+#include <stdlib.h>
 #include <sys/types.h>
 #include <unistd.h>
-#include <stdlib.h>
 #include <limits.h>
 
 #ifndef NGROUPS_MAX
@@ -28,22 +29,26 @@
 int
 __group_member (gid_t gid)
 {
-  int n, size;
+  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)))
+    abort ();
+  groups = buf.data;
 
-  size = NGROUPS_MAX;
-  do
-    {
-      groups = __alloca (size * sizeof *groups);
-      n = __getgroups (size, groups);
-      size *= 2;
-    }
-  while (n == size / 2);
+  n = __getgroups (n, groups);
 
   while (n-- > 0)
     if (groups[n] == gid)
-      return 1;
+      {
+	scratch_buffer_free (&buf);
+        return 1;
+      }
 
+  scratch_buffer_free (&buf);
   return 0;
 }
 weak_alias (__group_member, group_member)
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>
-- 
2.39.2

Re: [PATCH v2] group_member: Get rid of unbounded alloca.

[Florian Weimer]

* Joe Simmons-Talbott via Libc-alpha:

>  int
>  __group_member (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)))
> +    abort ();
> +  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;
>  }

The abort isn't ideal.  Should we deprecate this function because it
cannot be implemented correctly?

Typically, Linux supports up to 65,536 supplementary groups, so a memory
allocation may indeed be required.  Hurd can implement it without
allocation.

Thanks,
Florian

Re: [PATCH v2] group_member: Get rid of unbounded alloca.

[Siddhesh Poyarekar]

On 2023-08-09 05:43, Florian Weimer via Libc-alpha wrote:
> * Joe Simmons-Talbott via Libc-alpha:
> 
>>   int
>>   __group_member (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)))
>> +    abort ();
>> +  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;
>>   }
> 
> The abort isn't ideal.  Should we deprecate this function because it
> cannot be implemented correctly?

It depends on how commonly used it is.  It's a GNU extension, so we 
could just make a group_member2 that returns -1 for error (setting errno 
to indicate the reason for failure) and *then* deprecate this one, while 
also adding the abort() in there to guard against an unintentional 
overflow with tiny stacks.  What do you think?

Sid

Re: [PATCH v2] group_member: Get rid of unbounded alloca.

[Florian Weimer]

* Siddhesh Poyarekar:

> On 2023-08-09 05:43, Florian Weimer via Libc-alpha wrote:
>> * Joe Simmons-Talbott via Libc-alpha:
>> 
>>>   int
>>>   __group_member (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)))
>>> +    abort ();
>>> +  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;
>>>   }
>> The abort isn't ideal.  Should we deprecate this function because it
>> cannot be implemented correctly?
>
> It depends on how commonly used it is.  It's a GNU extension, so we
> could just make a group_member2 that returns -1 for error (setting
> errno to indicate the reason for failure) and *then* deprecate this
> one, while also adding the abort() in there to guard against an
> unintentional overflow with tiny stacks.  What do you think?

A three-state return value (-1/0/1) is notoriously difficult to deal
with because a lot of code treats -1 as a positive result, especially
after migration form the previous group_member variant.

Treating failure is as safe is probably safer.  So we could document
that the protocol is similar to readdir, maybe.  Or just deprecate the
function outright (for Linux at least).

Thanks,
Florian

Re: [PATCH v2] group_member: Get rid of unbounded alloca.

[Joe Simmons-Talbott]

On Tue, Aug 29, 2023 at 01:17:33PM +0200, Florian Weimer wrote:
> * Siddhesh Poyarekar:
> 
> > On 2023-08-09 05:43, Florian Weimer via Libc-alpha wrote:
> >> * Joe Simmons-Talbott via Libc-alpha:
> >> 
> >>>   int
> >>>   __group_member (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)))
> >>> +    abort ();
> >>> +  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;
> >>>   }
> >> The abort isn't ideal.  Should we deprecate this function because it
> >> cannot be implemented correctly?
> >
> > It depends on how commonly used it is.  It's a GNU extension, so we
> > could just make a group_member2 that returns -1 for error (setting
> > errno to indicate the reason for failure) and *then* deprecate this
> > one, while also adding the abort() in there to guard against an
> > unintentional overflow with tiny stacks.  What do you think?
> 
> A three-state return value (-1/0/1) is notoriously difficult to deal
> with because a lot of code treats -1 as a positive result, especially
> after migration form the previous group_member variant.
> 
> Treating failure is as safe is probably safer.  So we could document
> that the protocol is similar to readdir, maybe.  Or just deprecate the
> function outright (for Linux at least).
> 

I've posted a new patch[1] that deprecates group_member for Linux.

[1] https://sourceware.org/pipermail/libc-alpha/2023-September/151676.html

Thanks,
Joe