[PATCH v6] ifaddrs: Get rid of alloca

[PATCH v6] ifaddrs: Get rid of alloca

[Joe Simmons-Talbott]

Use scratch_buffer and malloc rather than alloca to avoid potential stack
overflows.
---
Changes to v5:
  * Don't bypass the __libc_scratch_buffer_set_array_size check by
    passing both the item size and count to scratch_buffer_set_array_size. 

 sysdeps/unix/sysv/linux/ifaddrs.c | 46 ++++++++++++++-----------------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c
index 184ee224cb..0db9bb7847 100644
--- a/sysdeps/unix/sysv/linux/ifaddrs.c
+++ b/sysdeps/unix/sysv/linux/ifaddrs.c
@@ -16,13 +16,13 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <alloca.h>
 #include <assert.h>
 #include <errno.h>
 #include <ifaddrs.h>
 #include <net/if.h>
 #include <netinet/in.h>
 #include <netpacket/packet.h>
+#include <scratch_buffer.h>
 #include <stdbool.h>
 #include <stdint.h>
 #include <stdlib.h>
@@ -131,26 +131,14 @@ __netlink_request (struct netlink_handle *h, int type)
   ssize_t read_len;
   bool done = false;
 
-#ifdef PAGE_SIZE
-  /* Help the compiler optimize out the malloc call if PAGE_SIZE
-     is constant and smaller or equal to PTHREAD_STACK_MIN/4.  */
-  const size_t buf_size = PAGE_SIZE;
-#else
-  const size_t buf_size = __getpagesize ();
-#endif
-  bool use_malloc = false;
-  char *buf;
-
-  if (__libc_use_alloca (buf_size))
-    buf = alloca (buf_size);
-  else
-    {
-      buf = malloc (buf_size);
-      if (buf != NULL)
-	use_malloc = true;
-      else
-	goto out_fail;
-    }
+  /* Netlink requires that user buffer needs to be either 8kb or page size
+     (whichever is bigger), however this has been changed over time and now
+     8Kb is sufficient (check NLMSG_DEFAULT_SIZE on Linux
+     linux/include/linux/netlink.h).  */
+  const size_t buf_size = 8192;
+  char *buf = malloc (buf_size);
+  if (buf == NULL)
+    goto out_fail;
 
   struct iovec iov = { buf, buf_size };
 
@@ -229,13 +217,11 @@ __netlink_request (struct netlink_handle *h, int type)
       h->end_ptr = nlm_next;
     }
 
-  if (use_malloc)
-    free (buf);
+  free(buf);
   return 0;
 
 out_fail:
-  if (use_malloc)
-    free (buf);
+  free(buf);
   return -1;
 }
 
@@ -324,6 +310,8 @@ getifaddrs_internal (struct ifaddrs **ifap)
   char *ifa_data_ptr;	/* Pointer to the unused part of memory for
 				ifa_data.  */
   int result = 0;
+  struct scratch_buffer buf;
+  scratch_buffer_init (&buf);
 
   *ifap = NULL;
 
@@ -425,7 +413,12 @@ getifaddrs_internal (struct ifaddrs **ifap)
     }
 
   /* Table for mapping kernel index to entry in our list.  */
-  map_newlink_data = alloca (newlink * sizeof (int));
+  if (!scratch_buffer_set_array_size (&buf, newlink, sizeof (int)))
+    {
+      result = -1;
+      goto exit_free;
+    }
+  map_newlink_data = buf.data;
   memset (map_newlink_data, '\xff', newlink * sizeof (int));
 
   ifa_data_ptr = (char *) &ifas[newlink + newaddr];
@@ -820,6 +813,7 @@ getifaddrs_internal (struct ifaddrs **ifap)
  exit_free:
   __netlink_free_handle (&nh);
   __netlink_close (&nh);
+  scratch_buffer_free (&buf);
 
   return result;
 }
-- 
2.39.2

Re: [PATCH v6] ifaddrs: Get rid of alloca

[Adhemerval Zanella Netto]

On 21/06/23 17:00, Joe Simmons-Talbott via Libc-alpha wrote:
> Use scratch_buffer and malloc rather than alloca to avoid potential stack
> overflows.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>


> ---
> Changes to v5:
>   * Don't bypass the __libc_scratch_buffer_set_array_size check by
>     passing both the item size and count to scratch_buffer_set_array_size. 
> 
>  sysdeps/unix/sysv/linux/ifaddrs.c | 46 ++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 26 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c
> index 184ee224cb..0db9bb7847 100644
> --- a/sysdeps/unix/sysv/linux/ifaddrs.c
> +++ b/sysdeps/unix/sysv/linux/ifaddrs.c
> @@ -16,13 +16,13 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <alloca.h>
>  #include <assert.h>
>  #include <errno.h>
>  #include <ifaddrs.h>
>  #include <net/if.h>
>  #include <netinet/in.h>
>  #include <netpacket/packet.h>
> +#include <scratch_buffer.h>
>  #include <stdbool.h>
>  #include <stdint.h>
>  #include <stdlib.h>
> @@ -131,26 +131,14 @@ __netlink_request (struct netlink_handle *h, int type)
>    ssize_t read_len;
>    bool done = false;
>  
> -#ifdef PAGE_SIZE
> -  /* Help the compiler optimize out the malloc call if PAGE_SIZE
> -     is constant and smaller or equal to PTHREAD_STACK_MIN/4.  */
> -  const size_t buf_size = PAGE_SIZE;
> -#else
> -  const size_t buf_size = __getpagesize ();
> -#endif
> -  bool use_malloc = false;
> -  char *buf;
> -
> -  if (__libc_use_alloca (buf_size))
> -    buf = alloca (buf_size);
> -  else
> -    {
> -      buf = malloc (buf_size);
> -      if (buf != NULL)
> -	use_malloc = true;
> -      else
> -	goto out_fail;
> -    }
> +  /* Netlink requires that user buffer needs to be either 8kb or page size
> +     (whichever is bigger), however this has been changed over time and now
> +     8Kb is sufficient (check NLMSG_DEFAULT_SIZE on Linux
> +     linux/include/linux/netlink.h).  */
> +  const size_t buf_size = 8192;
> +  char *buf = malloc (buf_size);
> +  if (buf == NULL)
> +    goto out_fail;
>  
>    struct iovec iov = { buf, buf_size };
>  
> @@ -229,13 +217,11 @@ __netlink_request (struct netlink_handle *h, int type)
>        h->end_ptr = nlm_next;
>      }
>  
> -  if (use_malloc)
> -    free (buf);
> +  free(buf);
>    return 0;
>  
>  out_fail:
> -  if (use_malloc)
> -    free (buf);
> +  free(buf);
>    return -1;
>  }
>  
> @@ -324,6 +310,8 @@ getifaddrs_internal (struct ifaddrs **ifap)
>    char *ifa_data_ptr;	/* Pointer to the unused part of memory for
>  				ifa_data.  */
>    int result = 0;
> +  struct scratch_buffer buf;
> +  scratch_buffer_init (&buf);
>  
>    *ifap = NULL;
>  
> @@ -425,7 +413,12 @@ getifaddrs_internal (struct ifaddrs **ifap)
>      }
>  
>    /* Table for mapping kernel index to entry in our list.  */
> -  map_newlink_data = alloca (newlink * sizeof (int));
> +  if (!scratch_buffer_set_array_size (&buf, newlink, sizeof (int)))
> +    {
> +      result = -1;
> +      goto exit_free;
> +    }
> +  map_newlink_data = buf.data;
>    memset (map_newlink_data, '\xff', newlink * sizeof (int));
>  
>    ifa_data_ptr = (char *) &ifas[newlink + newaddr];
> @@ -820,6 +813,7 @@ getifaddrs_internal (struct ifaddrs **ifap)
>   exit_free:
>    __netlink_free_handle (&nh);
>    __netlink_close (&nh);
> +  scratch_buffer_free (&buf);
>  
>    return result;
>  }

Re: [PATCH v6] ifaddrs: Get rid of alloca

[Siddhesh Poyarekar]

On 2023-06-22 09:47, Adhemerval Zanella Netto via Libc-alpha wrote:
> 
> 
> On 21/06/23 17:00, Joe Simmons-Talbott via Libc-alpha wrote:
>> Use scratch_buffer and malloc rather than alloca to avoid potential stack
>> overflows.
> 
> LGTM, thanks.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

Adhemerval, would it make sense to have Joe request write access to the 
repository so that he can push his patches?  Would you sponsor him?

Thanks,
Sid

Re: [PATCH v6] ifaddrs: Get rid of alloca

[Adhemerval Zanella Netto]

On 26/06/23 10:10, Siddhesh Poyarekar wrote:
> On 2023-06-22 09:47, Adhemerval Zanella Netto via Libc-alpha wrote:
>>
>>
>> On 21/06/23 17:00, Joe Simmons-Talbott via Libc-alpha wrote:
>>> Use scratch_buffer and malloc rather than alloca to avoid potential stack
>>> overflows.
>>
>> LGTM, thanks.
>>
>> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 
> Adhemerval, would it make sense to have Joe request write access to the repository so that he can push his patches?  Would you sponsor him?

Sure, if he does intend to continue work on glibc I think it should
be good to allow him to install his own patches.

Re: [PATCH v6] ifaddrs: Get rid of alloca

[Joe Simmons-Talbott]

On Mon, Jun 26, 2023 at 04:49:46PM -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 26/06/23 10:10, Siddhesh Poyarekar wrote:
> > On 2023-06-22 09:47, Adhemerval Zanella Netto via Libc-alpha wrote:
> >>
> >>
> >> On 21/06/23 17:00, Joe Simmons-Talbott via Libc-alpha wrote:
> >>> Use scratch_buffer and malloc rather than alloca to avoid potential stack
> >>> overflows.
> >>
> >> LGTM, thanks.
> >>
> >> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> > 
> > Adhemerval, would it make sense to have Joe request write access to the repository so that he can push his patches?  Would you sponsor him?
> 
> Sure, if he does intend to continue work on glibc I think it should
> be good to allow him to install his own patches.
> 
It is my intention to continue working on glibc.

Thanks,
Joe

Re: [PATCH v6] ifaddrs: Get rid of alloca

[Siddhesh Poyarekar]

On 2023-06-26 17:04, Joe Simmons-Talbott wrote:
> On Mon, Jun 26, 2023 at 04:49:46PM -0300, Adhemerval Zanella Netto wrote:
>>
>>
>> On 26/06/23 10:10, Siddhesh Poyarekar wrote:
>>> On 2023-06-22 09:47, Adhemerval Zanella Netto via Libc-alpha wrote:
>>>>
>>>>
>>>> On 21/06/23 17:00, Joe Simmons-Talbott via Libc-alpha wrote:
>>>>> Use scratch_buffer and malloc rather than alloca to avoid potential stack
>>>>> overflows.
>>>>
>>>> LGTM, thanks.
>>>>
>>>> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>>
>>> Adhemerval, would it make sense to have Joe request write access to the repository so that he can push his patches?  Would you sponsor him?
>>
>> Sure, if he does intend to continue work on glibc I think it should
>> be good to allow him to install his own patches.
>>
> It is my intention to continue working on glibc.

Please request commit access for glibc here:

https://sourceware.org/cgi-bin/pdw/ps_form.cgi

You may mention either Adhemerval or me as the approver.

Thanks,
Sid