* [PATCH] setipv4sourcefilter: Avoid using alloca.
@ 2023-05-24 18:18 Joe Simmons-Talbott
2023-05-25 2:09 ` Siddhesh Poyarekar
0 siblings, 1 reply; 8+ messages in thread
From: Joe Simmons-Talbott @ 2023-05-24 18:18 UTC (permalink / raw)
To: libc-alpha; +Cc: 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] setipv4sourcefilter: Avoid using alloca.
2023-05-24 18:18 [PATCH] setipv4sourcefilter: Avoid using alloca Joe Simmons-Talbott
@ 2023-05-25 2:09 ` Siddhesh Poyarekar
2023-05-25 7:33 ` Florian Weimer
0 siblings, 1 reply; 8+ messages in thread
From: Siddhesh Poyarekar @ 2023-05-25 2:09 UTC (permalink / raw)
To: Joe Simmons-Talbott, libc-alpha
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;
> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] setipv4sourcefilter: Avoid using alloca.
2023-05-25 2:09 ` Siddhesh Poyarekar
@ 2023-05-25 7:33 ` Florian Weimer
2023-05-25 11:04 ` Adhemerval Zanella Netto
0 siblings, 1 reply; 8+ messages in thread
From: Florian Weimer @ 2023-05-25 7:33 UTC (permalink / raw)
To: Siddhesh Poyarekar; +Cc: Joe Simmons-Talbott, libc-alpha
* 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] setipv4sourcefilter: Avoid using alloca.
2023-05-25 7:33 ` Florian Weimer
@ 2023-05-25 11:04 ` Adhemerval Zanella Netto
2023-05-25 11:27 ` Florian Weimer
0 siblings, 1 reply; 8+ messages in thread
From: Adhemerval Zanella Netto @ 2023-05-25 11:04 UTC (permalink / raw)
To: Florian Weimer, Siddhesh Poyarekar; +Cc: Joe Simmons-Talbott, libc-alpha
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] setipv4sourcefilter: Avoid using alloca.
2023-05-25 11:04 ` Adhemerval Zanella Netto
@ 2023-05-25 11:27 ` Florian Weimer
2023-05-25 11:57 ` Siddhesh Poyarekar
0 siblings, 1 reply; 8+ messages in thread
From: Florian Weimer @ 2023-05-25 11:27 UTC (permalink / raw)
To: Adhemerval Zanella Netto
Cc: Siddhesh Poyarekar, Joe Simmons-Talbott, libc-alpha
* 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] setipv4sourcefilter: Avoid using alloca.
2023-05-25 11:27 ` Florian Weimer
@ 2023-05-25 11:57 ` Siddhesh Poyarekar
2023-05-25 12:20 ` Adhemerval Zanella Netto
0 siblings, 1 reply; 8+ messages in thread
From: Siddhesh Poyarekar @ 2023-05-25 11:57 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella Netto; +Cc: Joe Simmons-Talbott, libc-alpha
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] setipv4sourcefilter: Avoid using alloca.
2023-05-25 11:57 ` Siddhesh Poyarekar
@ 2023-05-25 12:20 ` Adhemerval Zanella Netto
2023-05-25 12:25 ` Siddhesh Poyarekar
0 siblings, 1 reply; 8+ messages in thread
From: Adhemerval Zanella Netto @ 2023-05-25 12:20 UTC (permalink / raw)
To: Siddhesh Poyarekar, Florian Weimer; +Cc: Joe Simmons-Talbott, libc-alpha
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] setipv4sourcefilter: Avoid using alloca.
2023-05-25 12:20 ` Adhemerval Zanella Netto
@ 2023-05-25 12:25 ` Siddhesh Poyarekar
0 siblings, 0 replies; 8+ messages in thread
From: Siddhesh Poyarekar @ 2023-05-25 12:25 UTC (permalink / raw)
To: Adhemerval Zanella Netto, Florian Weimer; +Cc: Joe Simmons-Talbott, libc-alpha
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>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-05-25 12:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-24 18:18 [PATCH] setipv4sourcefilter: Avoid using alloca Joe Simmons-Talbott
2023-05-25 2:09 ` Siddhesh Poyarekar
2023-05-25 7:33 ` Florian Weimer
2023-05-25 11:04 ` Adhemerval Zanella Netto
2023-05-25 11:27 ` Florian Weimer
2023-05-25 11:57 ` Siddhesh Poyarekar
2023-05-25 12:20 ` Adhemerval Zanella Netto
2023-05-25 12:25 ` Siddhesh Poyarekar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).