diff mbox series

[v3,4/5] pcapng: avoid using alloca()

Message ID 20231108183557.381955-5-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series dumpcap and pcapng fixes |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Stephen Hemminger Nov. 8, 2023, 6:35 p.m. UTC 
  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.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/pcapng/rte_pcapng.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

Comments

Morten Brørup Nov. 9, 2023, 8:21 a.m. UTC | #1 
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, 8 November 2023 19.36
> 
> 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.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

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ørup <mb@smartsharesystems.com>


>  lib/pcapng/rte_pcapng.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
> 
> 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;
> 
>  	len = sizeof(*hdr);
>  	if (hw)
> @@ -158,8 +157,7 @@ pcapng_section_block(rte_pcapng_t *self,
>  	len += pcapng_optlen(0);
>  	len += sizeof(uint32_t);
> 
> -	buf = 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.

> 
>  	hdr = (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));
> 
> -	cc = 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.
Stephen Hemminger Nov. 9, 2023, 3:44 p.m. UTC | #2 
On Thu, 9 Nov 2023 09:21:22 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

> 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ørup <mb@smartsharesystems.com>

The constant BUFSIZ comes from stdio.h and used lots of places in libraries.
It is 8192 in current glibc and unlikely to be a problem.
Chose it because this a on stack buffer used before writing to a file which
is similar to what stdio does.

The library does not use stdio because most of the I/O is writing packets
which needs to be fast and overhead of extra stdio buffer is harmful.
Looking into using io_uring in a future version.
Morten Brørup Nov. 9, 2023, 4:25 p.m. UTC | #3 
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, 9 November 2023 16.45
> 
> On Thu, 9 Nov 2023 09:21:22 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > 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ørup <mb@smartsharesystems.com>
> 
> The constant BUFSIZ comes from stdio.h and used lots of places in
> libraries.
> It is 8192 in current glibc and unlikely to be a problem.

OK, didn't know that. So I looked it up, trying to learn more about it.

I found two sources [1], [2] mentioning that BUFSIZ is guaranteed to be at least 256.

[1]: https://www.gnu.org/software/libc/manual/html_node/Controlling-Buffering.html#BUFSIZ
[2]: Page 234 in "The C Standard Library" by P.J. Plauger, ISBN: 0-13-131509-9, from 1992

If 256 suffices, then I am OK with using BUFSIZ.

I hope the authors of the other libraries using BUFSIZ don't assume more than the C standard promises about it.

> Chose it because this a on stack buffer used before writing to a file
> which
> is similar to what stdio does.
> 
> The library does not use stdio because most of the I/O is writing
> packets
> which needs to be fast and overhead of extra stdio buffer is harmful.
> Looking into using io_uring in a future version.
diff mbox series

Patch

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;
 
 	len = sizeof(*hdr);
 	if (hw)
@@ -158,8 +157,7 @@  pcapng_section_block(rte_pcapng_t *self,
 	len += pcapng_optlen(0);
 	len += sizeof(uint32_t);
 
-	buf = calloc(1, len);
-	if (!buf)
+	if (len > sizeof(buf))
 		return -1;
 
 	hdr = (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));
 
-	cc = write(self->outfd, buf, len);
-	free(buf);
-
-	return cc;
+	return write(self->outfd, buf, len);
 }
 
 /* Write an interface block for a DPDK port */
@@ -213,7 +208,7 @@  rte_pcapng_add_interface(rte_pcapng_t *self, uint16_t port,
 	struct pcapng_option *opt;
 	const uint8_t tsresol = 9;	/* nanosecond resolution */
 	uint32_t len;
-	void *buf;
+	uint8_t buf[BUFSIZ];
 	char ifname_buf[IF_NAMESIZE];
 	char ifhw[256];
 	uint64_t speed = 0;
@@ -267,8 +262,7 @@  rte_pcapng_add_interface(rte_pcapng_t *self, uint16_t port,
 	len += pcapng_optlen(0);
 	len += sizeof(uint32_t);
 
-	buf = alloca(len);
-	if (!buf)
+	if (len > sizeof(buf))
 		return -1;
 
 	hdr = (struct pcapng_interface_block *)buf;
@@ -296,17 +290,16 @@  rte_pcapng_add_interface(rte_pcapng_t *self, uint16_t port,
 		opt = pcapng_add_option(opt, PCAPNG_IFB_HARDWARE,
 					 ifhw, strlen(ifhw));
 	if (filter) {
-		/* Encoding is that the first octet indicates string vs BPF */
 		size_t len;
-		char *buf;
 
 		len = strlen(filter) + 1;
-		buf = alloca(len);
-		*buf = '\0';
-		memcpy(buf + 1, filter, len);
+		opt->code = PCAPNG_IFB_FILTER;
+		opt->length = len;
+		/* Encoding is that the first octet indicates string vs BPF */
+		opt->data[0] = 0;
+		memcpy(opt->data + 1, filter, strlen(filter));
 
-		opt = pcapng_add_option(opt, PCAPNG_IFB_FILTER,
-					buf, len);
+		opt = (struct pcapng_option *)((uint8_t *)opt + pcapng_optlen(len));
 	}
 
 	opt = pcapng_add_option(opt, PCAPNG_OPT_END, NULL, 0);
@@ -333,7 +326,7 @@  rte_pcapng_write_stats(rte_pcapng_t *self, uint16_t port_id,
 	uint64_t start_time = self->offset_ns;
 	uint64_t sample_time;
 	uint32_t optlen, len;
-	uint8_t *buf;
+	uint8_t buf[BUFSIZ];
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -353,8 +346,7 @@  rte_pcapng_write_stats(rte_pcapng_t *self, uint16_t port_id,
 		optlen += pcapng_optlen(0);
 
 	len = sizeof(*hdr) + optlen + sizeof(uint32_t);
-	buf = alloca(len);
-	if (buf == NULL)
+	if (len > sizeof(buf))
 		return -1;
 
 	hdr = (struct pcapng_statistics *)buf;