From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 30706432E1; Thu, 9 Nov 2023 09:21:27 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B5FB742DEF; Thu, 9 Nov 2023 09:21:26 +0100 (CET) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 9DE9840267 for ; Thu, 9 Nov 2023 09:21:25 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 66C2C2231C; Thu, 9 Nov 2023 09:21:25 +0100 (CET) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v3 4/5] pcapng: avoid using alloca() X-MimeOLE: Produced By Microsoft Exchange V6.5 Date: Thu, 9 Nov 2023 09:21:22 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9EFFC@smartserver.smartshare.dk> In-Reply-To: <20231108183557.381955-5-stephen@networkplumber.org> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v3 4/5] pcapng: avoid using alloca() Thread-Index: AdoScodOo/Yh8LMmTq6TXbN5NAE2oQAcMnDw References: <20230921042349.104150-1-stephen@networkplumber.org> <20231108183557.381955-1-stephen@networkplumber.org> <20231108183557.381955-5-stephen@networkplumber.org> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Stephen Hemminger" , X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Wednesday, 8 November 2023 19.36 >=20 > The function alloca() like VLA's has problems if the caller > passes a large value. Instead use a fixed size buffer (4K) > which will be more than sufficient for the info related blocks > in the file. Add bounds checks as well. >=20 > Signed-off-by: Stephen Hemminger > --- I can't find the definition of BUFSIZ. Please make sure to add a comment = to the definition of BUFSIZ mentioning - like in your patch description = - that it will be more than sufficient for the info related blocks in = the file. More comments inline below, regarding existing bugs found while = reviewing. Assuming BUFSIZ has a comment describing the reason for its value, Acked-by: Morten Br=F8rup > lib/pcapng/rte_pcapng.c | 34 +++++++++++++--------------------- > 1 file changed, 13 insertions(+), 21 deletions(-) >=20 > diff --git a/lib/pcapng/rte_pcapng.c b/lib/pcapng/rte_pcapng.c > index 13fd2b97fb80..67f74d31aa32 100644 > --- a/lib/pcapng/rte_pcapng.c > +++ b/lib/pcapng/rte_pcapng.c > @@ -140,9 +140,8 @@ pcapng_section_block(rte_pcapng_t *self, > { > struct pcapng_section_header *hdr; > struct pcapng_option *opt; > - void *buf; > + uint8_t buf[BUFSIZ]; > uint32_t len; > - ssize_t cc; >=20 > len =3D sizeof(*hdr); > if (hw) > @@ -158,8 +157,7 @@ pcapng_section_block(rte_pcapng_t *self, > len +=3D pcapng_optlen(0); > len +=3D sizeof(uint32_t); >=20 > - buf =3D calloc(1, len); > - if (!buf) > + if (len > sizeof(buf)) > return -1; Existing bug: rte_errno must be set before returning -1. This bug occurs = multiple times in rte_pcapng.c, probably also in code you're not = updating in this patch. >=20 > hdr =3D (struct pcapng_section_header *)buf; > @@ -193,10 +191,7 @@ pcapng_section_block(rte_pcapng_t *self, > /* clone block_length after option */ > memcpy(opt, &hdr->block_length, sizeof(uint32_t)); >=20 > - cc =3D write(self->outfd, buf, len); > - free(buf); > - > - return cc; > + return write(self->outfd, buf, len); Existing bug: if write() returns -1, errno must be stored in rte_errno = before returning -1. This bug might also occur multiple times in = rte_pcapng.c.