Re: [dpdk-dev] [PATCH v2 2/5] examples/vhost_scsi: fix missing NULL-check for parameter

[Bruce Richardson <bruce.richardson@intel.com>]

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 <maxime.coquelin@redhat.com>
> > CC: Tiwei Bie <tiwei.bie@intel.com>
> > CC: Zhihong Wang <zhihong.wang@intel.com>
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  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