diff mbox series

[v2] check_native: Get rid of alloca

Message ID 20230601145500.2039369-1-josimmon@redhat.com
State Superseded
Headers
Series [v2] check_native: Get rid of alloca |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 pending Patch applied

Commit Message

Joe Simmons-Talbott June 1, 2023, 2:55 p.m. UTC 
  Use malloc rather than alloca to avoid potential stack overflow.
---
Changes to v1:
  * Do not use a cleanup handler.

 sysdeps/unix/sysv/linux/check_native.c | 35 ++++++++------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

Comments

Joe Simmons-Talbott June 9, 2023, 3 p.m. UTC | #1 
On Thu, Jun 01, 2023 at 10:55:00AM -0400, Joe Simmons-Talbott wrote:
> Use malloc rather than alloca to avoid potential stack overflow.

Ping.

Thanks,
Joe
> ---
> Changes to v1:
>   * Do not use a cleanup handler.
> 
>  sysdeps/unix/sysv/linux/check_native.c | 35 ++++++++------------------
>  1 file changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/check_native.c b/sysdeps/unix/sysv/linux/check_native.c
> index 34876ca624..601f14e5b0 100644
> --- a/sysdeps/unix/sysv/linux/check_native.c
> +++ b/sysdeps/unix/sysv/linux/check_native.c
> @@ -48,11 +48,20 @@ __check_native (uint32_t a1_index, int *a1_native,
>    nladdr.nl_family = AF_NETLINK;
>  
>    socklen_t addr_len = sizeof (nladdr);
> -  bool use_malloc = false;
>  
>    if (fd < 0)
>      return;
>  
> +  /* 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;
> +
>    if (__bind (fd, (struct sockaddr *) &nladdr, sizeof (nladdr)) != 0
>        || __getsockname (fd, (struct sockaddr *) &nladdr, &addr_len) != 0)
>      goto out;
> @@ -81,26 +90,6 @@ __check_native (uint32_t a1_index, int *a1_native,
>    memset (&nladdr, '\0', sizeof (nladdr));
>    nladdr.nl_family = AF_NETLINK;
>  
> -#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
> -  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;
> -    }
> -
>    struct iovec iov = { buf, buf_size };
>  
>    if (TEMP_FAILURE_RETRY (__sendto (fd, (void *) &req, sizeof (req), 0,
> @@ -170,7 +159,5 @@ __check_native (uint32_t a1_index, int *a1_native,
>  
>  out:
>    __close_nocancel_nostatus (fd);
> -
> -  if (use_malloc)
> -    free (buf);
> +  free(buf);
>  }
> -- 
> 2.39.2
>
Adhemerval Zanella Netto June 12, 2023, 9:09 p.m. UTC | #2 
On 01/06/23 11:55, Joe Simmons-Talbott via Libc-alpha wrote:
> Use malloc rather than alloca to avoid potential stack overflow.

LGTM with the small nit below fixed.

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

> ---
> Changes to v1:
>   * Do not use a cleanup handler.
> 
>  sysdeps/unix/sysv/linux/check_native.c | 35 ++++++++------------------
>  1 file changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/check_native.c b/sysdeps/unix/sysv/linux/check_native.c
> index 34876ca624..601f14e5b0 100644
> --- a/sysdeps/unix/sysv/linux/check_native.c
> +++ b/sysdeps/unix/sysv/linux/check_native.c
> @@ -48,11 +48,20 @@ __check_native (uint32_t a1_index, int *a1_native,
>    nladdr.nl_family = AF_NETLINK;
>  
>    socklen_t addr_len = sizeof (nladdr);
> -  bool use_malloc = false;
>  
>    if (fd < 0)
>      return;
>  
> +  /* 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;
> +
>    if (__bind (fd, (struct sockaddr *) &nladdr, sizeof (nladdr)) != 0
>        || __getsockname (fd, (struct sockaddr *) &nladdr, &addr_len) != 0)
>      goto out;
> @@ -81,26 +90,6 @@ __check_native (uint32_t a1_index, int *a1_native,
>    memset (&nladdr, '\0', sizeof (nladdr));
>    nladdr.nl_family = AF_NETLINK;
>  
> -#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
> -  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;
> -    }
> -
>    struct iovec iov = { buf, buf_size };
>  
>    if (TEMP_FAILURE_RETRY (__sendto (fd, (void *) &req, sizeof (req), 0,
> @@ -170,7 +159,5 @@ __check_native (uint32_t a1_index, int *a1_native,
>  
>  out:
>    __close_nocancel_nostatus (fd);
> -
> -  if (use_malloc)
> -    free (buf);
> +  free(buf);

Space before '('.

>  }
Joe Simmons-Talbott June 13, 2023, 2:06 p.m. UTC | #3 
On Mon, Jun 12, 2023 at 06:09:58PM -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 01/06/23 11:55, Joe Simmons-Talbott via Libc-alpha wrote:
> > Use malloc rather than alloca to avoid potential stack overflow.
> 
> LGTM with the small nit below fixed.

Thanks for the review.  Fixed in v3.

Thanks,
Joe
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 
> > ---
> > Changes to v1:
> >   * Do not use a cleanup handler.
> > 
> >  sysdeps/unix/sysv/linux/check_native.c | 35 ++++++++------------------
> >  1 file changed, 11 insertions(+), 24 deletions(-)
> > 
> > diff --git a/sysdeps/unix/sysv/linux/check_native.c b/sysdeps/unix/sysv/linux/check_native.c
> > index 34876ca624..601f14e5b0 100644
> > --- a/sysdeps/unix/sysv/linux/check_native.c
> > +++ b/sysdeps/unix/sysv/linux/check_native.c
> > @@ -48,11 +48,20 @@ __check_native (uint32_t a1_index, int *a1_native,
> >    nladdr.nl_family = AF_NETLINK;
> >  
> >    socklen_t addr_len = sizeof (nladdr);
> > -  bool use_malloc = false;
> >  
> >    if (fd < 0)
> >      return;
> >  
> > +  /* 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;
> > +
> >    if (__bind (fd, (struct sockaddr *) &nladdr, sizeof (nladdr)) != 0
> >        || __getsockname (fd, (struct sockaddr *) &nladdr, &addr_len) != 0)
> >      goto out;
> > @@ -81,26 +90,6 @@ __check_native (uint32_t a1_index, int *a1_native,
> >    memset (&nladdr, '\0', sizeof (nladdr));
> >    nladdr.nl_family = AF_NETLINK;
> >  
> > -#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
> > -  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;
> > -    }
> > -
> >    struct iovec iov = { buf, buf_size };
> >  
> >    if (TEMP_FAILURE_RETRY (__sendto (fd, (void *) &req, sizeof (req), 0,
> > @@ -170,7 +159,5 @@ __check_native (uint32_t a1_index, int *a1_native,
> >  
> >  out:
> >    __close_nocancel_nostatus (fd);
> > -
> > -  if (use_malloc)
> > -    free (buf);
> > +  free(buf);
> 
> Space before '('.
> 
> >  }
>
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/check_native.c b/sysdeps/unix/sysv/linux/check_native.c
index 34876ca624..601f14e5b0 100644
--- a/sysdeps/unix/sysv/linux/check_native.c
+++ b/sysdeps/unix/sysv/linux/check_native.c
@@ -48,11 +48,20 @@  __check_native (uint32_t a1_index, int *a1_native,
   nladdr.nl_family = AF_NETLINK;
 
   socklen_t addr_len = sizeof (nladdr);
-  bool use_malloc = false;
 
   if (fd < 0)
     return;
 
+  /* 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;
+
   if (__bind (fd, (struct sockaddr *) &nladdr, sizeof (nladdr)) != 0
       || __getsockname (fd, (struct sockaddr *) &nladdr, &addr_len) != 0)
     goto out;
@@ -81,26 +90,6 @@  __check_native (uint32_t a1_index, int *a1_native,
   memset (&nladdr, '\0', sizeof (nladdr));
   nladdr.nl_family = AF_NETLINK;
 
-#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
-  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;
-    }
-
   struct iovec iov = { buf, buf_size };
 
   if (TEMP_FAILURE_RETRY (__sendto (fd, (void *) &req, sizeof (req), 0,
@@ -170,7 +159,5 @@  __check_native (uint32_t a1_index, int *a1_native,
 
 out:
   __close_nocancel_nostatus (fd);
-
-  if (use_malloc)
-    free (buf);
+  free(buf);
 }