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

["Morten Brørup" <mb@smartsharesystems.com>]

> 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.