| Message ID | 20230621200053.3021604-1-josimmon@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6] ifaddrs: Get rid of alloca | expand |
On 21/06/23 17:00, Joe Simmons-Talbott via Libc-alpha wrote: > Use scratch_buffer and malloc rather than alloca to avoid potential stack > overflows. LGTM, thanks. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > --- > Changes to v5: > * Don't bypass the __libc_scratch_buffer_set_array_size check by > passing both the item size and count to scratch_buffer_set_array_size. > > sysdeps/unix/sysv/linux/ifaddrs.c | 46 ++++++++++++++----------------- > 1 file changed, 20 insertions(+), 26 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c > index 184ee224cb..0db9bb7847 100644 > --- a/sysdeps/unix/sysv/linux/ifaddrs.c > +++ b/sysdeps/unix/sysv/linux/ifaddrs.c > @@ -16,13 +16,13 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > -#include <alloca.h> > #include <assert.h> > #include <errno.h> > #include <ifaddrs.h> > #include <net/if.h> > #include <netinet/in.h> > #include <netpacket/packet.h> > +#include <scratch_buffer.h> > #include <stdbool.h> > #include <stdint.h> > #include <stdlib.h> > @@ -131,26 +131,14 @@ __netlink_request (struct netlink_handle *h, int type) > ssize_t read_len; > bool done = false; > > -#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 > - bool use_malloc = false; > - 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_fail; > - } > + /* 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_fail; > > struct iovec iov = { buf, buf_size }; > > @@ -229,13 +217,11 @@ __netlink_request (struct netlink_handle *h, int type) > h->end_ptr = nlm_next; > } > > - if (use_malloc) > - free (buf); > + free(buf); > return 0; > > out_fail: > - if (use_malloc) > - free (buf); > + free(buf); > return -1; > } > > @@ -324,6 +310,8 @@ getifaddrs_internal (struct ifaddrs **ifap) > char *ifa_data_ptr; /* Pointer to the unused part of memory for > ifa_data. */ > int result = 0; > + struct scratch_buffer buf; > + scratch_buffer_init (&buf); > > *ifap = NULL; > > @@ -425,7 +413,12 @@ getifaddrs_internal (struct ifaddrs **ifap) > } > > /* Table for mapping kernel index to entry in our list. */ > - map_newlink_data = alloca (newlink * sizeof (int)); > + if (!scratch_buffer_set_array_size (&buf, newlink, sizeof (int))) > + { > + result = -1; > + goto exit_free; > + } > + map_newlink_data = buf.data; > memset (map_newlink_data, '\xff', newlink * sizeof (int)); > > ifa_data_ptr = (char *) &ifas[newlink + newaddr]; > @@ -820,6 +813,7 @@ getifaddrs_internal (struct ifaddrs **ifap) > exit_free: > __netlink_free_handle (&nh); > __netlink_close (&nh); > + scratch_buffer_free (&buf); > > return result; > }
On 2023-06-22 09:47, Adhemerval Zanella Netto via Libc-alpha wrote: > > > On 21/06/23 17:00, Joe Simmons-Talbott via Libc-alpha wrote: >> Use scratch_buffer and malloc rather than alloca to avoid potential stack >> overflows. > > LGTM, thanks. > > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> Adhemerval, would it make sense to have Joe request write access to the repository so that he can push his patches? Would you sponsor him? Thanks, Sid
On 26/06/23 10:10, Siddhesh Poyarekar wrote: > On 2023-06-22 09:47, Adhemerval Zanella Netto via Libc-alpha wrote: >> >> >> On 21/06/23 17:00, Joe Simmons-Talbott via Libc-alpha wrote: >>> Use scratch_buffer and malloc rather than alloca to avoid potential stack >>> overflows. >> >> LGTM, thanks. >> >> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > Adhemerval, would it make sense to have Joe request write access to the repository so that he can push his patches? Would you sponsor him? Sure, if he does intend to continue work on glibc I think it should be good to allow him to install his own patches.
On Mon, Jun 26, 2023 at 04:49:46PM -0300, Adhemerval Zanella Netto wrote: > > > On 26/06/23 10:10, Siddhesh Poyarekar wrote: > > On 2023-06-22 09:47, Adhemerval Zanella Netto via Libc-alpha wrote: > >> > >> > >> On 21/06/23 17:00, Joe Simmons-Talbott via Libc-alpha wrote: > >>> Use scratch_buffer and malloc rather than alloca to avoid potential stack > >>> overflows. > >> > >> LGTM, thanks. > >> > >> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > > > Adhemerval, would it make sense to have Joe request write access to the repository so that he can push his patches? Would you sponsor him? > > Sure, if he does intend to continue work on glibc I think it should > be good to allow him to install his own patches. > It is my intention to continue working on glibc. Thanks, Joe
On 2023-06-26 17:04, Joe Simmons-Talbott wrote: > On Mon, Jun 26, 2023 at 04:49:46PM -0300, Adhemerval Zanella Netto wrote: >> >> >> On 26/06/23 10:10, Siddhesh Poyarekar wrote: >>> On 2023-06-22 09:47, Adhemerval Zanella Netto via Libc-alpha wrote: >>>> >>>> >>>> On 21/06/23 17:00, Joe Simmons-Talbott via Libc-alpha wrote: >>>>> Use scratch_buffer and malloc rather than alloca to avoid potential stack >>>>> overflows. >>>> >>>> LGTM, thanks. >>>> >>>> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> >>> >>> Adhemerval, would it make sense to have Joe request write access to the repository so that he can push his patches? Would you sponsor him? >> >> Sure, if he does intend to continue work on glibc I think it should >> be good to allow him to install his own patches. >> > It is my intention to continue working on glibc. Please request commit access for glibc here: https://sourceware.org/cgi-bin/pdw/ps_form.cgi You may mention either Adhemerval or me as the approver. Thanks, Sid
diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c index 184ee224cb..0db9bb7847 100644 --- a/sysdeps/unix/sysv/linux/ifaddrs.c +++ b/sysdeps/unix/sysv/linux/ifaddrs.c @@ -16,13 +16,13 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ -#include <alloca.h> #include <assert.h> #include <errno.h> #include <ifaddrs.h> #include <net/if.h> #include <netinet/in.h> #include <netpacket/packet.h> +#include <scratch_buffer.h> #include <stdbool.h> #include <stdint.h> #include <stdlib.h> @@ -131,26 +131,14 @@ __netlink_request (struct netlink_handle *h, int type) ssize_t read_len; bool done = false; -#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 - bool use_malloc = false; - 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_fail; - } + /* 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_fail; struct iovec iov = { buf, buf_size }; @@ -229,13 +217,11 @@ __netlink_request (struct netlink_handle *h, int type) h->end_ptr = nlm_next; } - if (use_malloc) - free (buf); + free(buf); return 0; out_fail: - if (use_malloc) - free (buf); + free(buf); return -1; } @@ -324,6 +310,8 @@ getifaddrs_internal (struct ifaddrs **ifap) char *ifa_data_ptr; /* Pointer to the unused part of memory for ifa_data. */ int result = 0; + struct scratch_buffer buf; + scratch_buffer_init (&buf); *ifap = NULL; @@ -425,7 +413,12 @@ getifaddrs_internal (struct ifaddrs **ifap) } /* Table for mapping kernel index to entry in our list. */ - map_newlink_data = alloca (newlink * sizeof (int)); + if (!scratch_buffer_set_array_size (&buf, newlink, sizeof (int))) + { + result = -1; + goto exit_free; + } + map_newlink_data = buf.data; memset (map_newlink_data, '\xff', newlink * sizeof (int)); ifa_data_ptr = (char *) &ifas[newlink + newaddr]; @@ -820,6 +813,7 @@ getifaddrs_internal (struct ifaddrs **ifap) exit_free: __netlink_free_handle (&nh); __netlink_close (&nh); + scratch_buffer_free (&buf); return result; }
Use scratch_buffer and malloc rather than alloca to avoid potential stack overflows. --- Changes to v5: * Don't bypass the __libc_scratch_buffer_set_array_size check by passing both the item size and count to scratch_buffer_set_array_size. sysdeps/unix/sysv/linux/ifaddrs.c | 46 ++++++++++++++----------------- 1 file changed, 20 insertions(+), 26 deletions(-)