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

[Florian Weimer <fweimer@redhat.com>]

* 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