setipv4sourcefilter: Avoid using alloca.
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
dj/TryBot-32bit |
success
|
Build for i686
|
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 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(-)
Comments
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;
> }
* 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
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.
* 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
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
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
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>
@@ -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;
}