On 05/11/2018 08:26 PM, De Lara Guarch, Pablo wrote:
-----Original Message-----
From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Andy Green
Sent: Friday, May 11, 2018 2:47 AM
To: dev@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 <a...@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.
OK.
+ 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?
It's just logging stuff, policy about truncated overlength logs could go
either way.
Prior to this patch its policy was to corrupt your stack if overlength
;-) so I think it's moving it on in the right direction merely
truncating it.
-Andy
+ 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: sta...@dpdk.org
