[v2] group_member: Get rid of unbounded alloca.

  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
  

* 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
  

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
  

* 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
  

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