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

[Tiwei Bie <tiwei.bie@intel.com>]

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.

> +				__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

Thanks,
Tiwei

> +	}
> +
>  	thread = pthread_self();
>  	CPU_ZERO(&cpuset);
>  	CPU_SET(0, &cpuset);
> -- 
> 2.20.1
>