From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.warmcat.com (mail.warmcat.com [163.172.24.82]) by dpdk.org (Postfix) with ESMTP id 1229F1CBB8; Sat, 12 May 2018 03:33:23 +0200 (CEST) To: "De Lara Guarch, Pablo" , "dev@dpdk.org" Cc: "Pattan, Reshma" , "stable@dpdk.org" References: <152600304856.53146.9681482138854493833.stgit@localhost.localdomain> <152600319626.53146.13225564187658388686.stgit@localhost.localdomain> From: Andy Green Message-ID: Date: Sat, 12 May 2018 09:33:17 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v4 16/18] app/proc-info: sprintf overrun bug X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 12 May 2018 01:33:23 -0000 On 05/11/2018 08:26 PM, De Lara Guarch, Pablo wrote: > > >> -----Original Message----- >> From: dev [mailto:dev-bounces@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 >> --- >> 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: stable@dpdk.org >