[v2,2/5] examples/vhost_scsi: fix missing NULL-check for parameter
Checks
Commit Message
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(+)
Comments
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
>
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
@@ -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",
+ __func__);
+ return NULL;
+ }
+
thread = pthread_self();
CPU_ZERO(&cpuset);
CPU_SET(0, &cpuset);