From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id F28941B277; Fri, 26 Apr 2019 14:31:22 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Apr 2019 05:31:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,397,1549958400"; d="scan'208";a="145947372" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.48]) by orsmga003.jf.intel.com with SMTP; 26 Apr 2019 05:31:18 -0700 Received: by (sSMTP sendmail emulation); Fri, 26 Apr 2019 13:31:18 +0100 Date: Fri, 26 Apr 2019 13:31:17 +0100 From: Bruce Richardson To: "Burakov, Anatoly" Cc: David Hunt , dev@dpdk.org, stable@dpdk.org Message-ID: <20190426123117.GA1695@bricha3-MOBL.ger.corp.intel.com> References: <20190426084337.3921-1-david.hunt@intel.com> <20190426112422.15719-1-david.hunt@intel.com> <339cb6ac-9c56-b9cb-28e8-950ad2cd70b4@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <339cb6ac-9c56-b9cb-28e8-950ad2cd70b4@intel.com> User-Agent: Mutt/1.11.4 (2019-03-13) Subject: Re: [dpdk-dev] [PATCH v2] examples/vm_power_manager: fix string null termination 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: Fri, 26 Apr 2019 12:31:23 -0000 On Fri, Apr 26, 2019 at 12:56:08PM +0100, Burakov, Anatoly wrote: > On 26-Apr-19 12:24 PM, David Hunt wrote: > > coverity complains about a null-termination after a read, > > so we terminate after exiting the do-while loop. The position > > is conditional on whether idx is within the buffer or at the > > end of the buffer. > > > > Coverity issue: 337680 > > Fixes: a63504a90f ("examples/power: add JSON string handling") > > CC: stable@dpdk.org > > > > Signed-off-by: David Hunt > > > > --- > > v2: > > * Move null termination outside of do-while. > > --- > > examples/vm_power_manager/channel_monitor.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/examples/vm_power_manager/channel_monitor.c b/examples/vm_power_manager/channel_monitor.c > > index 971e4f2bc..03fdcd15a 100644 > > --- a/examples/vm_power_manager/channel_monitor.c > > +++ b/examples/vm_power_manager/channel_monitor.c > > @@ -822,6 +822,8 @@ read_json_packet(struct channel_info *chan_info) > > break; > > } while (indent > 0); > > + json_data[idx + (idx < MAX_JSON_STRING_LEN - 1)] = '\0'; > > + > > I don't think you need this complicated logic here. You start at idx = 0, so > even if you receive 0 bytes, you'll terminate buffer at index 0. You also > break when idx reaches (MAX_JSON_STRING_LEN - 1), so it's also safe to do > json_data[idx] after the loop. In all other cases, you still increment idx > before breaking out (e.g. when reaching indent == 0), so it's also safe to > do json_data[idx] in those cases. > +1 to that. An alternative and simpler option might be to memset the who array to zero before you start anyway. /Bruce From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 1AA41A05D3 for ; Fri, 26 Apr 2019 14:31:26 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2B8DC1B69B; Fri, 26 Apr 2019 14:31:24 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id F28941B277; Fri, 26 Apr 2019 14:31:22 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Apr 2019 05:31:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,397,1549958400"; d="scan'208";a="145947372" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.48]) by orsmga003.jf.intel.com with SMTP; 26 Apr 2019 05:31:18 -0700 Received: by (sSMTP sendmail emulation); Fri, 26 Apr 2019 13:31:18 +0100 Date: Fri, 26 Apr 2019 13:31:17 +0100 From: Bruce Richardson To: "Burakov, Anatoly" Cc: David Hunt , dev@dpdk.org, stable@dpdk.org Message-ID: <20190426123117.GA1695@bricha3-MOBL.ger.corp.intel.com> References: <20190426084337.3921-1-david.hunt@intel.com> <20190426112422.15719-1-david.hunt@intel.com> <339cb6ac-9c56-b9cb-28e8-950ad2cd70b4@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: <339cb6ac-9c56-b9cb-28e8-950ad2cd70b4@intel.com> User-Agent: Mutt/1.11.4 (2019-03-13) Subject: Re: [dpdk-dev] [PATCH v2] examples/vm_power_manager: fix string null termination 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190426123117.xGz7lDsfjP77Rn3bKBBSV7aPSFS1o9LPtUUBvckCPds@z> On Fri, Apr 26, 2019 at 12:56:08PM +0100, Burakov, Anatoly wrote: > On 26-Apr-19 12:24 PM, David Hunt wrote: > > coverity complains about a null-termination after a read, > > so we terminate after exiting the do-while loop. The position > > is conditional on whether idx is within the buffer or at the > > end of the buffer. > > > > Coverity issue: 337680 > > Fixes: a63504a90f ("examples/power: add JSON string handling") > > CC: stable@dpdk.org > > > > Signed-off-by: David Hunt > > > > --- > > v2: > > * Move null termination outside of do-while. > > --- > > examples/vm_power_manager/channel_monitor.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/examples/vm_power_manager/channel_monitor.c b/examples/vm_power_manager/channel_monitor.c > > index 971e4f2bc..03fdcd15a 100644 > > --- a/examples/vm_power_manager/channel_monitor.c > > +++ b/examples/vm_power_manager/channel_monitor.c > > @@ -822,6 +822,8 @@ read_json_packet(struct channel_info *chan_info) > > break; > > } while (indent > 0); > > + json_data[idx + (idx < MAX_JSON_STRING_LEN - 1)] = '\0'; > > + > > I don't think you need this complicated logic here. You start at idx = 0, so > even if you receive 0 bytes, you'll terminate buffer at index 0. You also > break when idx reaches (MAX_JSON_STRING_LEN - 1), so it's also safe to do > json_data[idx] after the loop. In all other cases, you still increment idx > before breaking out (e.g. when reaching indent == 0), so it's also safe to > do json_data[idx] in those cases. > +1 to that. An alternative and simpler option might be to memset the who array to zero before you start anyway. /Bruce