check_native: Get rid of alloca

Use malloc rather than alloca to avoid potential stack overflow.
---
 sysdeps/unix/sysv/linux/check_native.c | 27 +++++++++-----------------
 1 file changed, 9 insertions(+), 18 deletions(-)

On Wed, May 31, 2023 at 09:01:22AM -0400, Joe Simmons-Talbott wrote:
> Use malloc rather than alloca to avoid potential stack overflow.

This is failing on 32bit builds[1] and I'm not sure how to fix it or why
it's failing here but didn't with the same construct in my ifaddrs patch
[2]

Thanks,
Joe

[1] https://www.delorie.com/trybots/32bit/20795/make.tail.txt
[2] https://sourceware.org/pipermail/libc-alpha/2023-May/148681.html

> ---
>  sysdeps/unix/sysv/linux/check_native.c | 27 +++++++++-----------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/check_native.c b/sysdeps/unix/sysv/linux/check_native.c
> index 34876ca624..45b328f240 100644
> --- a/sysdeps/unix/sysv/linux/check_native.c
> +++ b/sysdeps/unix/sysv/linux/check_native.c
> @@ -37,6 +37,12 @@
>  
>  #include "netlinkaccess.h"
>  
> +static void
> +ifree (char **ptr)
> +{
> +  free (*ptr);
> +}
> +
>  void
>  __check_native (uint32_t a1_index, int *a1_native,
>  		uint32_t a2_index, int *a2_native)
> @@ -48,7 +54,6 @@ __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;
> @@ -82,24 +87,13 @@ __check_native (uint32_t a1_index, int *a1_native,
>    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;
> -    }
> +  char *buf __attribute__ ((__cleanup__ (ifree))) = malloc (buf_size);
> +  if (buf == NULL)
> +    goto out;
>  
>    struct iovec iov = { buf, buf_size };
>  
> @@ -170,7 +164,4 @@ __check_native (uint32_t a1_index, int *a1_native,
>  
>  out:
>    __close_nocancel_nostatus (fd);
> -
> -  if (use_malloc)
> -    free (buf);
>  }
> -- 
> 2.39.2
>

On 31/05/23 17:35, Joe Simmons-Talbott via Libc-alpha wrote:
> On Wed, May 31, 2023 at 09:01:22AM -0400, Joe Simmons-Talbott wrote:
>> Use malloc rather than alloca to avoid potential stack overflow.
> 
> This is failing on 32bit builds[1] and I'm not sure how to fix it or why
> it's failing here but didn't with the same construct in my ifaddrs patch
> [2]
> 
> Thanks,
> Joe
> 
> [1] https://www.delorie.com/trybots/32bit/20795/make.tail.txt
> [2] https://sourceware.org/pipermail/libc-alpha/2023-May/148681.html


The cleanup attribute runs a function when the variable goes out of scope, 
which means the goto on the __bind/__getsockname force the buf to be on
the error path scope and then free will indeed trigger UB by accessing
uninitialized memory (for example https://godbolt.org/z/soWbhMnYT).

I think it would be better to just remove the 'out:' label, using goto
and cleanup handlers is tricky as you just saw it.  Also, the netlink
buffer follows the same constraint for NLMSG_DEFAULT_SIZE (as I wrote
in a previous patch).

Something like this, on top of your patch:

diff --git a/sysdeps/unix/sysv/linux/check_native.c b/sysdeps/unix/sysv/linux/check_native.c
index 45b328f240..5e6fc9f86f 100644
--- a/sysdeps/unix/sysv/linux/check_native.c
+++ b/sysdeps/unix/sysv/linux/check_native.c
@@ -43,11 +43,21 @@ ifree (char **ptr)
   free (*ptr);
 }
 
+static void
+iclose (int *fd)
+{
+  if (*fd >= 0)
+    __close_nocancel_nostatus (*fd);
+}
+
 void
 __check_native (uint32_t a1_index, int *a1_native,
 		uint32_t a2_index, int *a2_native)
 {
-  int fd = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
+  int __attribute__ ((__cleanup__ (iclose))) fd
+    = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
+  if (fd < 0)
+    return;
 
   struct sockaddr_nl nladdr;
   memset (&nladdr, '\0', sizeof (nladdr));
@@ -55,12 +65,9 @@ __check_native (uint32_t a1_index, int *a1_native,
 
   socklen_t addr_len = sizeof (nladdr);
 
-  if (fd < 0)
-    return;
-
   if (__bind (fd, (struct sockaddr *) &nladdr, sizeof (nladdr)) != 0
       || __getsockname (fd, (struct sockaddr *) &nladdr, &addr_len) != 0)
-    goto out;
+    return;
 
   pid_t pid = nladdr.nl_pid;
   struct req
@@ -86,21 +93,21 @@ __check_native (uint32_t a1_index, int *a1_native,
   memset (&nladdr, '\0', sizeof (nladdr));
   nladdr.nl_family = AF_NETLINK;
 
-#ifdef PAGE_SIZE
-  const size_t buf_size = PAGE_SIZE;
-#else
-  const size_t buf_size = __getpagesize ();
-#endif
+  /* 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 __attribute__ ((__cleanup__ (ifree))) = malloc (buf_size);
   if (buf == NULL)
-    goto out;
+    return;
 
   struct iovec iov = { buf, buf_size };
 
   if (TEMP_FAILURE_RETRY (__sendto (fd, (void *) &req, sizeof (req), 0,
 				    (struct sockaddr *) &nladdr,
 				    sizeof (nladdr))) < 0)
-    goto out;
+    return;
 
   bool done = false;
   do
@@ -119,10 +126,10 @@ __check_native (uint32_t a1_index, int *a1_native,
       ssize_t read_len = TEMP_FAILURE_RETRY (__recvmsg (fd, &msg, 0));
       __netlink_assert_response (fd, read_len);
       if (read_len < 0)
-	goto out;
+	return;
 
       if (msg.msg_flags & MSG_TRUNC)
-	goto out;
+	return;
 
       struct nlmsghdr *nlmh;
       for (nlmh = (struct nlmsghdr *) buf;
@@ -153,7 +160,7 @@ __check_native (uint32_t a1_index, int *a1_native,
 
 	      if (a1_index == 0xffffffffu
 		  && a2_index == 0xffffffffu)
-		goto out;
+		return;
 	    }
 	  else if (nlmh->nlmsg_type == NLMSG_DONE)
 	    /* We found the end, leave the loop.  */
@@ -161,7 +168,4 @@ __check_native (uint32_t a1_index, int *a1_native,
 	}
     }
   while (! done);
-
-out:
-  __close_nocancel_nostatus (fd);
 }

* Adhemerval Zanella Netto via Libc-alpha:

> +static void
> +iclose (int *fd)
> +{
> +  if (*fd >= 0)
> +    __close_nocancel_nostatus (*fd);
> +}
> +
>  void
>  __check_native (uint32_t a1_index, int *a1_native,
>  		uint32_t a2_index, int *a2_native)
>  {
> -  int fd = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
> +  int __attribute__ ((__cleanup__ (iclose))) fd
> +    = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
> +  if (fd < 0)
> +    return;

I think introducing attribute cleanup where equivalent functionality can
be implemented otherwise requires a discussion first.

Personally, I think that if we want to program with destructors, we
should switch to C++.

Thanks,
Florian

On Jun 01 2023, Florian Weimer via Libc-alpha wrote:

> * Adhemerval Zanella Netto via Libc-alpha:
>
>> +static void
>> +iclose (int *fd)
>> +{
>> +  if (*fd >= 0)
>> +    __close_nocancel_nostatus (*fd);
>> +}
>> +
>>  void
>>  __check_native (uint32_t a1_index, int *a1_native,
>>  		uint32_t a2_index, int *a2_native)
>>  {
>> -  int fd = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
>> +  int __attribute__ ((__cleanup__ (iclose))) fd
>> +    = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
>> +  if (fd < 0)
>> +    return;
>
> I think introducing attribute cleanup where equivalent functionality can
> be implemented otherwise requires a discussion first.

The cleanup attribute requires compiling the function and all callees
with -fasynchronous-unwind-tables.

* Andreas Schwab:

> On Jun 01 2023, Florian Weimer via Libc-alpha wrote:
>
>> * Adhemerval Zanella Netto via Libc-alpha:
>>
>>> +static void
>>> +iclose (int *fd)
>>> +{
>>> +  if (*fd >= 0)
>>> +    __close_nocancel_nostatus (*fd);
>>> +}
>>> +
>>>  void
>>>  __check_native (uint32_t a1_index, int *a1_native,
>>>  		uint32_t a2_index, int *a2_native)
>>>  {
>>> -  int fd = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
>>> +  int __attribute__ ((__cleanup__ (iclose))) fd
>>> +    = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
>>> +  if (fd < 0)
>>> +    return;
>>
>> I think introducing attribute cleanup where equivalent functionality can
>> be implemented otherwise requires a discussion first.
>
> The cleanup attribute requires compiling the function and all callees
> with -fasynchronous-unwind-tables.

I think GCC will still call the destructor on normal scope exit (if no
exception is thrown), even with -fno-exceptions.  That should be
sufficient here?

Thanks,
Florian

On Jun 01 2023, Florian Weimer wrote:

> * Andreas Schwab:
>
>> On Jun 01 2023, Florian Weimer via Libc-alpha wrote:
>>
>>> * Adhemerval Zanella Netto via Libc-alpha:
>>>
>>>> +static void
>>>> +iclose (int *fd)
>>>> +{
>>>> +  if (*fd >= 0)
>>>> +    __close_nocancel_nostatus (*fd);
>>>> +}
>>>> +
>>>>  void
>>>>  __check_native (uint32_t a1_index, int *a1_native,
>>>>  		uint32_t a2_index, int *a2_native)
>>>>  {
>>>> -  int fd = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
>>>> +  int __attribute__ ((__cleanup__ (iclose))) fd
>>>> +    = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
>>>> +  if (fd < 0)
>>>> +    return;
>>>
>>> I think introducing attribute cleanup where equivalent functionality can
>>> be implemented otherwise requires a discussion first.
>>
>> The cleanup attribute requires compiling the function and all callees
>> with -fasynchronous-unwind-tables.
>
> I think GCC will still call the destructor on normal scope exit (if no
> exception is thrown), even with -fno-exceptions.  That should be
> sufficient here?

The cancellation could be asynchronous.

* Andreas Schwab:

> On Jun 01 2023, Florian Weimer wrote:
>
>> * Andreas Schwab:
>>
>>> On Jun 01 2023, Florian Weimer via Libc-alpha wrote:
>>>
>>>> * Adhemerval Zanella Netto via Libc-alpha:
>>>>
>>>>> +static void
>>>>> +iclose (int *fd)
>>>>> +{
>>>>> +  if (*fd >= 0)
>>>>> +    __close_nocancel_nostatus (*fd);
>>>>> +}
>>>>> +
>>>>>  void
>>>>>  __check_native (uint32_t a1_index, int *a1_native,
>>>>>  		uint32_t a2_index, int *a2_native)
>>>>>  {
>>>>> -  int fd = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
>>>>> +  int __attribute__ ((__cleanup__ (iclose))) fd
>>>>> +    = __socket (PF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
>>>>> +  if (fd < 0)
>>>>> +    return;
>>>>
>>>> I think introducing attribute cleanup where equivalent functionality can
>>>> be implemented otherwise requires a discussion first.
>>>
>>> The cleanup attribute requires compiling the function and all callees
>>> with -fasynchronous-unwind-tables.
>>
>> I think GCC will still call the destructor on normal scope exit (if no
>> exception is thrown), even with -fno-exceptions.  That should be
>> sufficient here?
>
> The cancellation could be asynchronous.

The current code does handle cancellation.  Isn't sendto a cancellation
point?

In any case, we'd have to build with -fexceptions to turn the destructor
into a cancellation handler, not -fasynchronous-unwind-tables.

Thanks,
Florian

On Jun 01 2023, Florian Weimer wrote:

> The current code does handle cancellation.  Isn't sendto a cancellation
> point?

Cancellation depends on unwinding.

> In any case, we'd have to build with -fexceptions to turn the destructor
> into a cancellation handler, not -fasynchronous-unwind-tables.

-fexceptions does not enable asynchronous unwinding.

* Andreas Schwab:

> On Jun 01 2023, Florian Weimer wrote:
>
>> The current code does handle cancellation.  Isn't sendto a cancellation
>> point?
>
> Cancellation depends on unwinding.

Sorry, I meant to write “does NOT handle cancellation”.

>> In any case, we'd have to build with -fexceptions to turn the destructor
>> into a cancellation handler, not -fasynchronous-unwind-tables.
>
> -fexceptions does not enable asynchronous unwinding.

But the unwinding is synchronous if it is triggered from a cancellation
point because on the caller side, it looks like we are unwinding through
a regular function call which is not _THROW.

Using malloc will never be async-cancel-safe.

Anyway, if we use __attribute__ ((cleanup)) to write cancellation
handlers (as opposed to mere destructors), that definitely is worthy of
discussion.

Thanks,
Florian

On 01/06/23 06:07, Florian Weimer wrote:
> * Andreas Schwab:
> 
>> On Jun 01 2023, Florian Weimer wrote:
>>
>>> The current code does handle cancellation.  Isn't sendto a cancellation
>>> point?
>>
>> Cancellation depends on unwinding.
> 
> Sorry, I meant to write “does NOT handle cancellation”.
> 
>>> In any case, we'd have to build with -fexceptions to turn the destructor
>>> into a cancellation handler, not -fasynchronous-unwind-tables.
>>
>> -fexceptions does not enable asynchronous unwinding.
> 
> But the unwinding is synchronous if it is triggered from a cancellation
> point because on the caller side, it looks like we are unwinding through
> a regular function call which is not _THROW.
> 
> Using malloc will never be async-cancel-safe.
> 
> Anyway, if we use __attribute__ ((cleanup)) to write cancellation
> handlers (as opposed to mere destructors), that definitely is worthy of
> discussion.

Fair enough, I think there is no urgent need to use __attribute__ ((cleanup)),
although it does simplify some resource cleanup.

Should we remove the one use on sysdeps/posix/readv.c and sysdeps/posix/writev.c?
It is only used on Hurd (which I am not use if support cancellation), but it
a precedent to other developers that __attribute__ ((cleanup)) might be used
in other code.

* Adhemerval Zanella Netto:

> Should we remove the one use on sysdeps/posix/readv.c and
> sysdeps/posix/writev.c?  It is only used on Hurd (which I am not use
> if support cancellation), but it a precedent to other developers that
> __attribute__ ((cleanup)) might be used in other code.

Hurd supports cancellation.  I think it would make sense to replace
these instances of __attribute__ ((cleanup) with with the usual
cancellation handler macros (assuming that it works, it may need IS_IN
(libc) conditionals).

Thanks,
Florian