[PATCH] setipv4sourcefilter: Avoid using alloca.

[PATCH] setipv4sourcefilter: Avoid using alloca.

[Joe Simmons-Talbott]

Use a scratch_buffer rather than alloca/malloc to avoid potential stack
overflow.
---
 sysdeps/unix/sysv/linux/setipv4sourcefilter.c | 24 ++++++-------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/setipv4sourcefilter.c b/sysdeps/unix/sysv/linux/setipv4sourcefilter.c
index 7f146d789a..8c77ba33bd 100644
--- a/sysdeps/unix/sysv/linux/setipv4sourcefilter.c
+++ b/sysdeps/unix/sysv/linux/setipv4sourcefilter.c
@@ -16,12 +16,12 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <alloca.h>
 #include <errno.h>
 #include <stdlib.h>
 #include <string.h>
 #include <stdint.h>
 #include <netinet/in.h>
+#include <scratch_buffer.h>
 #include <sys/socket.h>
 
 
@@ -33,17 +33,12 @@ setipv4sourcefilter (int s, struct in_addr interface, struct in_addr group,
   /* We have to create an struct ip_msfilter object which we can pass
      to the kernel.  */
   size_t needed = IP_MSFILTER_SIZE (numsrc);
-  int use_alloca = __libc_use_alloca (needed);
 
-  struct ip_msfilter *imsf;
-  if (use_alloca)
-    imsf = (struct ip_msfilter *) alloca (needed);
-  else
-    {
-      imsf = (struct ip_msfilter *) malloc (needed);
-      if (imsf == NULL)
-	return -1;
-    }
+  struct scratch_buffer buf;
+  scratch_buffer_init (&buf);
+  if (!scratch_buffer_set_array_size (&buf, 1, needed))
+    return -1;
+  struct ip_msfilter *imsf = buf.data;
 
   imsf->imsf_multiaddr = group;
   imsf->imsf_interface = interface;
@@ -53,12 +48,7 @@ setipv4sourcefilter (int s, struct in_addr interface, struct in_addr group,
 
   int result = __setsockopt (s, SOL_IP, IP_MSFILTER, imsf, needed);
 
-  if (! use_alloca)
-    {
-      int save_errno = errno;
-      free (imsf);
-      __set_errno (save_errno);
-    }
+  scratch_buffer_free (&buf);
 
   return result;
 }
-- 
2.39.2

Re: [PATCH] setipv4sourcefilter: Avoid using alloca.

[Siddhesh Poyarekar]

On 2023-05-24 14:18, Joe Simmons-Talbott via Libc-alpha wrote:
> Use a scratch_buffer rather than alloca/malloc to avoid potential stack
> overflow.
> ---
>   sysdeps/unix/sysv/linux/setipv4sourcefilter.c | 24 ++++++-------------
>   1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/setipv4sourcefilter.c b/sysdeps/unix/sysv/linux/setipv4sourcefilter.c
> index 7f146d789a..8c77ba33bd 100644
> --- a/sysdeps/unix/sysv/linux/setipv4sourcefilter.c
> +++ b/sysdeps/unix/sysv/linux/setipv4sourcefilter.c
> @@ -16,12 +16,12 @@
>      License along with the GNU C Library; if not, see
>      <https://www.gnu.org/licenses/>.  */
>   
> -#include <alloca.h>
>   #include <errno.h>
>   #include <stdlib.h>
>   #include <string.h>
>   #include <stdint.h>
>   #include <netinet/in.h>
> +#include <scratch_buffer.h>
>   #include <sys/socket.h>
>   
>   
> @@ -33,17 +33,12 @@ setipv4sourcefilter (int s, struct in_addr interface, struct in_addr group,
>     /* We have to create an struct ip_msfilter object which we can pass
>        to the kernel.  */
>     size_t needed = IP_MSFILTER_SIZE (numsrc);
> -  int use_alloca = __libc_use_alloca (needed);
>   
> -  struct ip_msfilter *imsf;
> -  if (use_alloca)
> -    imsf = (struct ip_msfilter *) alloca (needed);
> -  else
> -    {
> -      imsf = (struct ip_msfilter *) malloc (needed);
> -      if (imsf == NULL)
> -	return -1;
> -    }
> +  struct scratch_buffer buf;
> +  scratch_buffer_init (&buf);
> +  if (!scratch_buffer_set_array_size (&buf, 1, needed))
> +    return -1;
> +  struct ip_msfilter *imsf = buf.data;
>   
>     imsf->imsf_multiaddr = group;
>     imsf->imsf_interface = interface;
> @@ -53,12 +48,7 @@ setipv4sourcefilter (int s, struct in_addr interface, struct in_addr group,
>   
>     int result = __setsockopt (s, SOL_IP, IP_MSFILTER, imsf, needed);
>   
> -  if (! use_alloca)
> -    {
> -      int save_errno = errno;
> -      free (imsf);
> -      __set_errno (save_errno);
> -    }
> +  scratch_buffer_free (&buf);

scratch_buffer_free will also likely tamper with errno (it calls free 
after all) so it might make sense to save/restore errno here.  In fact I 
wonder if it makes sense to have scratch_buffer_free do that so that 
it's always safe to use it without worrying about errno.

Thanks,
Sid

>   
>     return result;
>   }

Re: [PATCH] setipv4sourcefilter: Avoid using alloca.

[Florian Weimer]

* Siddhesh Poyarekar:

> scratch_buffer_free will also likely tamper with errno (it calls free
> after all) so it might make sense to save/restore errno here.  In fact
> I wonder if it makes sense to have scratch_buffer_free do that so that
> it's always safe to use it without worrying about errno.

We need to change free not to clobber errno.  Mainly this requires
protecting munmap and mprotect calls.  It's a QoI issue.

Thanks,
Florian

Re: [PATCH] setipv4sourcefilter: Avoid using alloca.

[Adhemerval Zanella Netto]

On 25/05/23 04:33, Florian Weimer via Libc-alpha wrote:
> * Siddhesh Poyarekar:
> 
>> scratch_buffer_free will also likely tamper with errno (it calls free
>> after all) so it might make sense to save/restore errno here.  In fact
>> I wonder if it makes sense to have scratch_buffer_free do that so that
>> it's always safe to use it without worrying about errno.
> 
> We need to change free not to clobber errno.  Mainly this requires
> protecting munmap and mprotect calls.  It's a QoI issue.
> 
> Thanks,
> Florian

We already save/restore errno on free since 69fda43b8dd795c.  We can optimize
it a bit by adding munmap/mprotect that calls INTERNAL_SYSCALL_CALL, but I
am not sure if the complexity will really be worth here.

Re: [PATCH] setipv4sourcefilter: Avoid using alloca.

[Florian Weimer]

* Adhemerval Zanella Netto:

> On 25/05/23 04:33, Florian Weimer via Libc-alpha wrote:
>> * Siddhesh Poyarekar:
>> 
>>> scratch_buffer_free will also likely tamper with errno (it calls free
>>> after all) so it might make sense to save/restore errno here.  In fact
>>> I wonder if it makes sense to have scratch_buffer_free do that so that
>>> it's always safe to use it without worrying about errno.
>> 
>> We need to change free not to clobber errno.  Mainly this requires
>> protecting munmap and mprotect calls.  It's a QoI issue.
>> 
>> Thanks,
>> Florian
>
> We already save/restore errno on free since 69fda43b8dd795c.  We can optimize
> it a bit by adding munmap/mprotect that calls INTERNAL_SYSCALL_CALL, but I
> am not sure if the complexity will really be worth here.

Ah, right, then scratch_buffer_free should be okay, too.

Thanks,
Florian

Re: [PATCH] setipv4sourcefilter: Avoid using alloca.

[Siddhesh Poyarekar]

On 2023-05-25 07:27, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 25/05/23 04:33, Florian Weimer via Libc-alpha wrote:
>>> * Siddhesh Poyarekar:
>>>
>>>> scratch_buffer_free will also likely tamper with errno (it calls free
>>>> after all) so it might make sense to save/restore errno here.  In fact
>>>> I wonder if it makes sense to have scratch_buffer_free do that so that
>>>> it's always safe to use it without worrying about errno.
>>>
>>> We need to change free not to clobber errno.  Mainly this requires
>>> protecting munmap and mprotect calls.  It's a QoI issue.
>>>
>>> Thanks,
>>> Florian
>>
>> We already save/restore errno on free since 69fda43b8dd795c.  We can optimize
>> it a bit by adding munmap/mprotect that calls INTERNAL_SYSCALL_CALL, but I
>> am not sure if the complexity will really be worth here.
> 
> Ah, right, then scratch_buffer_free should be okay, too.

I guess, but should we still stick to preserving errno to account for 
lack of errno preservation in non-glibc malloc implementations?

Thanks,
Sid

Re: [PATCH] setipv4sourcefilter: Avoid using alloca.

[Adhemerval Zanella Netto]

On 25/05/23 08:57, Siddhesh Poyarekar wrote:
> On 2023-05-25 07:27, Florian Weimer wrote:
>> * Adhemerval Zanella Netto:
>>
>>> On 25/05/23 04:33, Florian Weimer via Libc-alpha wrote:
>>>> * Siddhesh Poyarekar:
>>>>
>>>>> scratch_buffer_free will also likely tamper with errno (it calls free
>>>>> after all) so it might make sense to save/restore errno here.  In fact
>>>>> I wonder if it makes sense to have scratch_buffer_free do that so that
>>>>> it's always safe to use it without worrying about errno.
>>>>
>>>> We need to change free not to clobber errno.  Mainly this requires
>>>> protecting munmap and mprotect calls.  It's a QoI issue.
>>>>
>>>> Thanks,
>>>> Florian
>>>
>>> We already save/restore errno on free since 69fda43b8dd795c.  We can optimize
>>> it a bit by adding munmap/mprotect that calls INTERNAL_SYSCALL_CALL, but I
>>> am not sure if the complexity will really be worth here.
>>
>> Ah, right, then scratch_buffer_free should be okay, too.
> 
> I guess, but should we still stick to preserving errno to account for lack of errno preservation in non-glibc malloc implementations?

I don't think it is worth, this requirement will be in the next POSIX [1] and 
it also means that we will need to propagate this assumption on all internal
glibc code (which is only boilerplate code in the end).

[1] https://www.austingroupbugs.net/view.php?id=385

Re: [PATCH] setipv4sourcefilter: Avoid using alloca.

[Siddhesh Poyarekar]

On 2023-05-25 08:20, Adhemerval Zanella Netto wrote:
>>>> We already save/restore errno on free since 69fda43b8dd795c.  We can optimize
>>>> it a bit by adding munmap/mprotect that calls INTERNAL_SYSCALL_CALL, but I
>>>> am not sure if the complexity will really be worth here.
>>>
>>> Ah, right, then scratch_buffer_free should be okay, too.
>>
>> I guess, but should we still stick to preserving errno to account for lack of errno preservation in non-glibc malloc implementations?
> 
> I don't think it is worth, this requirement will be in the next POSIX [1] and
> it also means that we will need to propagate this assumption on all internal
> glibc code (which is only boilerplate code in the end).
> 
> [1] https://www.austingroupbugs.net/view.php?id=385

In that case, LGTM.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>