[dpdk-dev] [PATCH v4 16/18] app/proc-info: sprintf overrun bug

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Andy Green
> Sent: Friday, May 11, 2018 2:47 AM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH v4 16/18] app/proc-info: sprintf overrun bug
> 
> /home/agreen/projects/dpdk/app/proc-info/main.c: In function
> ‘nic_xstats_display’:
> /home/agreen/projects/dpdk/app/proc-info/main.c:495:45:
> error: ‘%s’ directive writing up to 255 bytes into a region of size between 165
> and 232 [-Werror=format-overflow=]
>     sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
>                                              ^~
>      PRIu64"\n", host_id, port_id, counter_type,
>                                    ~~~~~~~~~~~~
> /home/agreen/projects/dpdk/app/proc-info/main.c:495:4: note:
> ‘sprintf’ output between 31 and 435 bytes into a destination of size 256
>     sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      PRIu64"\n", host_id, port_id, counter_type,
>      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      xstats_names[i].name, values[i]);
> 
> Signed-off-by: Andy Green <andy at warmcat.com>
> ---
>  app/proc-info/main.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/app/proc-info/main.c b/app/proc-info/main.c index
> 539e13243..df46c235e 100644
> --- a/app/proc-info/main.c
> +++ b/app/proc-info/main.c
> @@ -488,14 +488,19 @@ nic_xstats_display(uint16_t port_id)
>  		if (enable_collectd_format) {
>  			char counter_type[MAX_STRING_LEN];
>  			char buf[MAX_STRING_LEN];
> +			size_t n;
> 
>  			collectd_resolve_cnt_type(counter_type,
>  						  sizeof(counter_type),
>  						  xstats_names[i].name);
> -			sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
> +			n = snprintf(buf, MAX_STRING_LEN,
> +				"PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
>  				PRIu64"\n", host_id, port_id, counter_type,
>  				xstats_names[i].name, values[i]);
> -			ret = write(stdout_fd, buf, strlen(buf));
> +			buf[sizeof(buf) - 1] = '\0';

No need to NULL terminate, since snprintf already does it.

> +			if (n > sizeof(buf) - 1)
> +				n = sizeof(buf) - 1;

If (n > sizeof(buf) -1 ), it means that there wasn't enough space to write all the data,
So shouldn't we return an error here?

> +			ret = write(stdout_fd, buf, n);
>  			if (ret < 0)
>  				goto err;
>  		} else {

Missing fixes line and CC stable.

Fixes: 2deb6b5246d7 ("app/procinfo: add collectd format and host id")
Cc: stable at dpdk.org