| Message ID | 20230526002247.1898289-1-josimmon@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ifaddrs: Get rid of alloca | expand |
On 25/05/23 21:22, Joe Simmons-Talbott via Libc-alpha wrote: > Use scratch_buffers rather than alloca to avoid potential stack > overflows. > --- > sysdeps/unix/sysv/linux/ifaddrs.c | 36 +++++++++++++++---------------- > 1 file changed, 17 insertions(+), 19 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c > index 184ee224cb..ca37b4f6db 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> > @@ -138,19 +138,11 @@ __netlink_request (struct netlink_handle *h, int type) > #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; > - } > + struct scratch_buffer sbuf; > + scratch_buffer_init (&sbuf); > + if (!scratch_buffer_set_array_size (&sbuf, 1, buf_size)) > + goto out_fail; > + char *buf = sbuf.data; Although netlink documentation [1] specifies that user buffer needs to be either 8kb or the architecture page size (whichever is bigger), this has been changed over time and now a 8Kb is suffice [2]. In fact using larger buffers is not really effective, unless you also tune the netlink socket buffer size [3]. So to use effectively use netlink, you will need to use at least a 8Kb buffer which is larger than the default static one (1024). So there is no much point in using a scratch_buffer here. Also, I think it should remove the PAGE_SIZE and use a 8Kb buffer instead. Something like: -- static void ifree (char **ptr) { free (*ptr); } int __netlink_request (struct netlink_handle *h, int type) { [...] /* 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 a 8Kb is suffice (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) return -1; [...] } -- [1] Documentation/userspace-api/netlink/intro.rst [2] https://patchwork.ozlabs.org/project/netdev/patch/e9409957290af5249750afa0f10de3a6@linux.vnet.ibm.com/ [3] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=d35c99ff77ecb2eb239731b799386f3b3637a31e > > struct iovec iov = { buf, buf_size }; > > @@ -229,13 +221,11 @@ __netlink_request (struct netlink_handle *h, int type) > h->end_ptr = nlm_next; > } > > - if (use_malloc) > - free (buf); > + scratch_buffer_free (&sbuf); > return 0; > > out_fail: > - if (use_malloc) > - free (buf); > + scratch_buffer_free (&sbuf); > return -1; > } > > @@ -324,6 +314,7 @@ 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; > > *ifap = NULL; > > @@ -425,7 +416,13 @@ getifaddrs_internal (struct ifaddrs **ifap) > } > > /* Table for mapping kernel index to entry in our list. */ > - map_newlink_data = alloca (newlink * sizeof (int)); > + scratch_buffer_init (&buf); > + if (!scratch_buffer_set_array_size (&buf, 1, 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 +817,7 @@ getifaddrs_internal (struct ifaddrs **ifap) > exit_free: > __netlink_free_handle (&nh); > __netlink_close (&nh); > + scratch_buffer_free (&buf); This might trigger an invalid free on uninitialized memory if __netlink_request fails. Move the on the scratch_buffer initialization just after the buf declaration. > > return result; > }
diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c index 184ee224cb..ca37b4f6db 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> @@ -138,19 +138,11 @@ __netlink_request (struct netlink_handle *h, int type) #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; - } + struct scratch_buffer sbuf; + scratch_buffer_init (&sbuf); + if (!scratch_buffer_set_array_size (&sbuf, 1, buf_size)) + goto out_fail; + char *buf = sbuf.data; struct iovec iov = { buf, buf_size }; @@ -229,13 +221,11 @@ __netlink_request (struct netlink_handle *h, int type) h->end_ptr = nlm_next; } - if (use_malloc) - free (buf); + scratch_buffer_free (&sbuf); return 0; out_fail: - if (use_malloc) - free (buf); + scratch_buffer_free (&sbuf); return -1; } @@ -324,6 +314,7 @@ 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; *ifap = NULL; @@ -425,7 +416,13 @@ getifaddrs_internal (struct ifaddrs **ifap) } /* Table for mapping kernel index to entry in our list. */ - map_newlink_data = alloca (newlink * sizeof (int)); + scratch_buffer_init (&buf); + if (!scratch_buffer_set_array_size (&buf, 1, 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 +817,7 @@ getifaddrs_internal (struct ifaddrs **ifap) exit_free: __netlink_free_handle (&nh); __netlink_close (&nh); + scratch_buffer_free (&buf); return result; }