From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 4B3E74CA6; Mon, 8 Apr 2019 11:37:12 +0200 (CEST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Apr 2019 02:37:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,324,1549958400"; d="scan'208";a="289666384" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.35]) by orsmga004.jf.intel.com with SMTP; 08 Apr 2019 02:37:08 -0700 Received: by (sSMTP sendmail emulation); Mon, 08 Apr 2019 10:37:08 +0100 Date: Mon, 8 Apr 2019 10:37:07 +0100 From: Bruce Richardson To: Tiwei Bie Cc: dev@dpdk.org, stable@dpdk.org, Maxime Coquelin , Zhihong Wang Message-ID: <20190408093707.GA1734@bricha3-MOBL.ger.corp.intel.com> References: <20190405134511.49066-1-bruce.richardson@intel.com> <20190405143709.50352-1-bruce.richardson@intel.com> <20190405143709.50352-3-bruce.richardson@intel.com> <20190408033110.GB20719@dpdk-tbie.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190408033110.GB20719@dpdk-tbie.sh.intel.com> User-Agent: Mutt/1.11.4 (2019-03-13) Subject: Re: [dpdk-dev] [PATCH v2 2/5] examples/vhost_scsi: fix missing NULL-check for parameter 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: Mon, 08 Apr 2019 09:37:13 -0000 On Mon, Apr 08, 2019 at 11:31:11AM +0800, Tiwei Bie wrote: > On Fri, Apr 05, 2019 at 03:37:06PM +0100, Bruce Richardson wrote: > > Coverity points out that there is a check in the main thread loop for the > > ctrlr->bdev being NULL, but by that stage the pointer has already been > > dereferenced. Therefore, for safety, before we enter the loop do an > > initial check on the parameter structure. > > > > Coverity issue: 158657 > > Fixes: db75c7af19bb ("examples/vhost_scsi: introduce a new sample app") > > CC: stable@dpdk.org > > CC: Maxime Coquelin > > CC: Tiwei Bie > > CC: Zhihong Wang > > > > Signed-off-by: Bruce Richardson > > --- > > examples/vhost_scsi/vhost_scsi.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/examples/vhost_scsi/vhost_scsi.c b/examples/vhost_scsi/vhost_scsi.c > > index 2908ff68b..a6465d089 100644 > > --- a/examples/vhost_scsi/vhost_scsi.c > > +++ b/examples/vhost_scsi/vhost_scsi.c > > @@ -285,6 +285,12 @@ ctrlr_worker(void *arg) > > cpu_set_t cpuset; > > pthread_t thread; > > > > + if (ctrlr == NULL || ctrlr->bdev == NULL) { > > + fprintf(stdout, "%s: Error, invalid argument passed to worker thread\n", > > Might be better to print to stderr. > Yes, this looks a typo on my part. > > + __func__); > > + return NULL; > > Might be better to exit() directly like what the other > error handling in this thread does: > > https://github.com/DPDK/dpdk/blob/bdcfcceb7a0b7534a0dba669279d18bd0f98d5e5/examples/vhost_scsi/vhost_scsi.c#L296-L299 > > Otherwise destroy_device() will hang on sem_wait(&exit_sem): > > https://github.com/DPDK/dpdk/blob/bdcfcceb7a0b7534a0dba669279d18bd0f98d5e5/examples/vhost_scsi/vhost_scsi.c#L390 > Ok, will change in next revision 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 8AA73A0096 for ; Mon, 8 Apr 2019 11:37:14 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 27D662C24; Mon, 8 Apr 2019 11:37:14 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 4B3E74CA6; Mon, 8 Apr 2019 11:37:12 +0200 (CEST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Apr 2019 02:37:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,324,1549958400"; d="scan'208";a="289666384" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.35]) by orsmga004.jf.intel.com with SMTP; 08 Apr 2019 02:37:08 -0700 Received: by (sSMTP sendmail emulation); Mon, 08 Apr 2019 10:37:08 +0100 Date: Mon, 8 Apr 2019 10:37:07 +0100 From: Bruce Richardson To: Tiwei Bie Cc: dev@dpdk.org, stable@dpdk.org, Maxime Coquelin , Zhihong Wang Message-ID: <20190408093707.GA1734@bricha3-MOBL.ger.corp.intel.com> References: <20190405134511.49066-1-bruce.richardson@intel.com> <20190405143709.50352-1-bruce.richardson@intel.com> <20190405143709.50352-3-bruce.richardson@intel.com> <20190408033110.GB20719@dpdk-tbie.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: <20190408033110.GB20719@dpdk-tbie.sh.intel.com> User-Agent: Mutt/1.11.4 (2019-03-13) Subject: Re: [dpdk-dev] [PATCH v2 2/5] examples/vhost_scsi: fix missing NULL-check for parameter 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: <20190408093707.FzgJetohikzeHnyrPLXaN4DlfnEPEOykY60WqZLrn6o@z> On Mon, Apr 08, 2019 at 11:31:11AM +0800, Tiwei Bie wrote: > On Fri, Apr 05, 2019 at 03:37:06PM +0100, Bruce Richardson wrote: > > Coverity points out that there is a check in the main thread loop for the > > ctrlr->bdev being NULL, but by that stage the pointer has already been > > dereferenced. Therefore, for safety, before we enter the loop do an > > initial check on the parameter structure. > > > > Coverity issue: 158657 > > Fixes: db75c7af19bb ("examples/vhost_scsi: introduce a new sample app") > > CC: stable@dpdk.org > > CC: Maxime Coquelin > > CC: Tiwei Bie > > CC: Zhihong Wang > > > > Signed-off-by: Bruce Richardson > > --- > > examples/vhost_scsi/vhost_scsi.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/examples/vhost_scsi/vhost_scsi.c b/examples/vhost_scsi/vhost_scsi.c > > index 2908ff68b..a6465d089 100644 > > --- a/examples/vhost_scsi/vhost_scsi.c > > +++ b/examples/vhost_scsi/vhost_scsi.c > > @@ -285,6 +285,12 @@ ctrlr_worker(void *arg) > > cpu_set_t cpuset; > > pthread_t thread; > > > > + if (ctrlr == NULL || ctrlr->bdev == NULL) { > > + fprintf(stdout, "%s: Error, invalid argument passed to worker thread\n", > > Might be better to print to stderr. > Yes, this looks a typo on my part. > > + __func__); > > + return NULL; > > Might be better to exit() directly like what the other > error handling in this thread does: > > https://github.com/DPDK/dpdk/blob/bdcfcceb7a0b7534a0dba669279d18bd0f98d5e5/examples/vhost_scsi/vhost_scsi.c#L296-L299 > > Otherwise destroy_device() will hang on sem_wait(&exit_sem): > > https://github.com/DPDK/dpdk/blob/bdcfcceb7a0b7534a0dba669279d18bd0f98d5e5/examples/vhost_scsi/vhost_scsi.c#L390 > Ok, will change in next revision