From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x35.google.com (mail-oa1-x35.google.com [IPv6:2001:4860:4864:20::35]) by sourceware.org (Postfix) with ESMTPS id BE271386D634 for ; Wed, 31 May 2023 12:24:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BE271386D634 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-oa1-x35.google.com with SMTP id 586e51a60fabf-19f3550bcceso2237549fac.0 for ; Wed, 31 May 2023 05:24:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1685535860; x=1688127860; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=+QVTEH4NuxSMlcc4eTbY2+QOIQyK2paICTGUzCSJ+Wg=; b=JYyhh7Zeek8UUbqS0ykWRYLm5OXgu8WK9zQ50HyZcA2krPrNGRS37ClcL/UVLwzFto TPqpjao+OgdKpCjW5uoSihvN/w7a5MT+16Nr0kiIk46NNlQInnKENwOpOsXjd4qY0UwF jSZEqZ2ru35/QIonm7U2gIsVV784rLvme25DjAgKhb1Me+HeN/GzsjHz7kWfeh4DWQ02 hRrC8l5kqBmtbsjxpetwHpYz/heZFEVb5jaVAL56XvbmnU7/Vo9oYBBdwcrxquib4FGH Qc8niXQT9xK82wy4/scrVN190Yggz75ztey6H7yVKfujqIWYW29vbqA1x0JYg5wh1gnY 7bYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685535860; x=1688127860; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=+QVTEH4NuxSMlcc4eTbY2+QOIQyK2paICTGUzCSJ+Wg=; b=WDCeDgTO0zUgbE/lqKPZdIq6E8RWZjhnYHEdfoxzu71hZkqF/PvEsabSremYA4HkDy OZkxygFlG2oteoUgAhodNM2gJzWjs5zJ0MR4OpniIHRrrVGHzN63gYWIXx4kXLr4/Gp1 Rxezh5BTZCWB+QA0YsknXLcsRaMtQ7vKTGw8BhhHibighDvSKV6c312sxsq6LWwe/+Uq bcucaFOkl5XIH+BjI0yNVtGKgAVaQo++pVXKQ0Y1Wvwft34brQOCq8CxzWHEPJDYWFli aGMnYLZeDcottG/GXjNrppWlF03g7xzVuG5phoZQkvLv6Nletp2BIallP/ZGinMO4JVS 3SbQ== X-Gm-Message-State: AC+VfDyeH7jmW+eZ7K/jE0iH+pduCACW6uKcvJvxHvc/tVdhDgrzzYZT ZbLfOXVPMz8CM3eotTRBbcNj79/UvuEJvCoiL/tgDA== X-Google-Smtp-Source: ACHHUZ6gAs33RMQkQXRnF7VFm9wKZs8m8Gqzb491AVqZte/mlHL7RsF6CjY/npG9TqICXYyANSaWhg== X-Received: by 2002:a05:6870:b790:b0:196:4dc8:e2b0 with SMTP id ed16-20020a056870b79000b001964dc8e2b0mr3279120oab.26.1685535860417; Wed, 31 May 2023 05:24:20 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c1:4dd5:94e0:97d1:f451:639a? ([2804:1b3:a7c1:4dd5:94e0:97d1:f451:639a]) by smtp.gmail.com with ESMTPSA id zh1-20020a0568716b8100b0017fea9c156esm471607oab.18.2023.05.31.05.24.18 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 31 May 2023 05:24:19 -0700 (PDT) Message-ID: <00fcc8a3-1525-5e27-f5d2-88e17b8e1033@linaro.org> Date: Wed, 31 May 2023 09:24:17 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.11.1 Subject: Re: [PATCH v3] ifaddrs: Get rid of alloca Content-Language: en-US To: libc-alpha@sourceware.org References: <20230530152539.2063770-1-josimmon@redhat.com> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <20230530152539.2063770-1-josimmon@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 30/05/23 12:25, Joe Simmons-Talbott via Libc-alpha wrote: > Use scratch_buffer and malloc rather than alloca to avoid potential stack > overflows. > --- > Changes to v2: > * Initialize the scratch_buffer earlier to avoid memory access errors > when calling scratch_buffer_free for failure cases. > > Changes to v1: > * in __netlink_request use an 8kb buffer size and malloc rather than a > scratch_buffer. > > Suggested-by: Adhemerval Zanella Netto > > sysdeps/unix/sysv/linux/ifaddrs.c | 51 +++++++++++++++---------------- > 1 file changed, 25 insertions(+), 26 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c > index 184ee224cb..f62510811a 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 > . */ > > -#include > #include > #include > #include > #include > #include > #include > +#include > #include > #include > #include > @@ -122,6 +122,13 @@ __netlink_sendreq (struct netlink_handle *h, int type) > } > > > +static void > +ifree (char **ptr) > +{ > + free (*ptr); > +} > + > + > int > __netlink_request (struct netlink_handle *h, int type) > { > @@ -131,26 +138,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 __attribute__ ((__cleanup__ (ifree))) = malloc (buf_size); > + if (buf == NULL) > + return -1; With the cleanup handler you can remove all the 'goto out_fail' and just return -1. diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c index f62510811a..ef87b00423 100644 --- a/sysdeps/unix/sysv/linux/ifaddrs.c +++ b/sysdeps/unix/sysv/linux/ifaddrs.c @@ -150,7 +150,7 @@ __netlink_request (struct netlink_handle *h, int type) struct iovec iov = { buf, buf_size }; if (__netlink_sendreq (h, type) < 0) - goto out_fail; + return -1; while (! done) { @@ -168,13 +168,13 @@ __netlink_request (struct netlink_handle *h, int type) read_len = TEMP_FAILURE_RETRY (__recvmsg (h->fd, &msg, 0)); __netlink_assert_response (h->fd, read_len); if (read_len < 0) - goto out_fail; + return -1; if (nladdr.nl_pid != 0) continue; if (__glibc_unlikely (msg.msg_flags & MSG_TRUNC)) - goto out_fail; + return -1; size_t count = 0; size_t remaining_len = read_len; @@ -200,7 +200,7 @@ __netlink_request (struct netlink_handle *h, int type) errno = EIO; else errno = -nlerr->error; - goto out_fail; + return -1; } } @@ -212,7 +212,7 @@ __netlink_request (struct netlink_handle *h, int type) nlm_next = (struct netlink_res *) malloc (sizeof (struct netlink_res) + read_len); if (nlm_next == NULL) - goto out_fail; + return -1; nlm_next->next = NULL; nlm_next->nlh = memcpy (nlm_next + 1, buf, read_len); nlm_next->size = read_len; @@ -225,9 +225,6 @@ __netlink_request (struct netlink_handle *h, int type) } return 0; - -out_fail: - return -1; } LGTM with this change. > > struct iovec iov = { buf, buf_size }; > > @@ -229,13 +224,9 @@ __netlink_request (struct netlink_handle *h, int type) > h->end_ptr = nlm_next; > } > > - if (use_malloc) > - free (buf); > return 0; > > out_fail: > - if (use_malloc) > - free (buf); > return -1; > } > > @@ -324,6 +315,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 +418,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, 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 +818,7 @@ getifaddrs_internal (struct ifaddrs **ifap) > exit_free: > __netlink_free_handle (&nh); > __netlink_close (&nh); > + scratch_buffer_free (&buf); > > return result; > }