Re: [PATCH] grantpt: Get rid of alloca

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2023-06-05 18:02, Joe Simmons-Talbott wrote:
> On Mon, Jun 05, 2023 at 03:14:06PM -0300, Adhemerval Zanella Netto wrote:
>>
>>
>> On 05/06/23 10:37, Joe Simmons-Talbott via Libc-alpha wrote:
>>> On Mon, Jun 05, 2023 at 03:29:45PM +0200, Florian Weimer wrote:
>>>> * Joe Simmons-Talbott via Libc-alpha:
>>>>
>>>>> Replace alloca with a scratch_buffer to avoid potential stack overflows.
>>>>> ---
>>>>>   sysdeps/unix/grantpt.c | 12 +++++++++++-
>>>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c
>>>>> index 38fce52576..e5d2390bf2 100644
>>>>> --- a/sysdeps/unix/grantpt.c
>>>>> +++ b/sysdeps/unix/grantpt.c
>>>>> @@ -20,6 +20,7 @@
>>>>>   #include <fcntl.h>
>>>>>   #include <grp.h>
>>>>>   #include <limits.h>
>>>>> +#include <scratch_buffer.h>
>>>>>   #include <stdlib.h>
>>>>>   #include <string.h>
>>>>>   #include <sys/resource.h>
>>>>> @@ -147,7 +148,14 @@ grantpt (int fd)
>>>>>   	/* `sysconf' does not support _SC_GETGR_R_SIZE_MAX.
>>>>>   	   Try a moderate value.  */
>>>>>   	grbuflen = 1024;
>>>>> -      grtmpbuf = (char *) __alloca (grbuflen);
>>>>> +      struct scratch_buffer sbuf;
>>>>> +      scratch_buffer_init (&sbuf);
>>>>> +      if (!scratch_buffer_set_array_size (&sbuf, 1, grbuflen))
>>>>> +	{
>>>>> +	  retval -1;
>>>>> +	  goto cleanup;
>>>>> +	}
>>>>> +      grtmpbuf = sbuf.data;
>>>>>         __getgrnam_r (TTY_GROUP, &grbuf, grtmpbuf, grbuflen, &p);
>>>>>         if (p != NULL)
>>>>>   	tty_gid = p->gr_gid;
>>>>> @@ -255,6 +263,8 @@ grantpt (int fd)
>>>>>     if (buf != _buf)
>>>>>       free (buf);
>>>>>   
>>>>> +  scratch_buffer_free(sbuf);
>>>>> +
>>>>>     return retval;
>>>>>   }
>>>>>   libc_hidden_def (grantpt)
>>>>
>>>> How did you test this?  This code is only used on Hurd due to the
>>>> override in sysdeps/unix/sysv/linux/grantpt.c.
>>>
>>> The only testing I did was a 'make && make check' on x86_64 and i686 linux.
>>
>> You will need to build for i686-gnu (Hurd) to actually enable this code.  Which
>> leads to another, which Siddheash has brough, which is --enable-pt_chown does
>> make sense to be provided as a configure switch.

Yeah I looked at the code again and perhaps it's something for Samuel to 
think about.  We don't seem to have target-specific configure options, 
so it's a nop for Linux.

> I tested this patch today with build-many-glibcs.py for i686-gnu and it
> passed 'make check'.  Does your second sentence mean that I needed to
> pass '--enable-pt_chown' to configure for the test build?

Not really, the code you're changing is outside of HAVE_PT_CHOWN, so 
you're good to go I think.  I'd wait for Samuel to ack this since this 
is exclusively hurd code.

Thanks,
Sid