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 |
fail
|
Patch series failed to build
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
pending
|
Patch applied
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
pending
|
Patch applied
|
Commit Message
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(-)
Comments
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
@@ -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);
}