Re: [Qemu-block] [PATCH] block/nvme: fix Coverity reports

[Paolo Bonzini]

On 09/02/2018 17:16, Kevin Wolf wrote:
> Am 09.02.2018 um 16:24 hat Paolo Bonzini geschrieben:
>> 1) string not null terminated in sysfs_find_group_file
>>
>> 2) NULL pointer dereference and dead local variable in nvme_init.
>>
>> Signed-off-by: Paolo Bonzini 
>> ---
>>  block/nvme.c| 14 ++
>>  util/vfio-helpers.c |  2 +-
>>  2 files changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index e9d0e218fc..ce217ffc81 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -553,7 +553,6 @@ static int nvme_init(BlockDriverState *bs, const char 
>> *device, int namespace,
>>  uint64_t cap;
>>  uint64_t timeout_ms;
>>  uint64_t deadline, now;
>> -Error *local_err = NULL;
>>  
>>  qemu_co_mutex_init(>dma_map_lock);
>>  qemu_co_queue_init(>dma_flush_queue);
>> @@ -645,11 +644,6 @@ static int nvme_init(BlockDriverState *bs, const char 
>> *device, int namespace,
>> false, nvme_handle_event, nvme_poll_cb);
>>  
>>  nvme_identify(bs, namespace, errp);
>> -if (local_err) {
>> -error_propagate(errp, local_err);
>> -ret = -EIO;
>> -goto fail_handler;
>> -}
>>  
>>  /* Set up command queues. */
>>  if (!nvme_add_io_queue(bs, errp)) {
> 
> I don't think this is right, errp must not be assigned twice, and you
> don't want to return 0 when an error is set. Even if we were return the
> right return value and errp content, it would be rather surprising to
> have an error set without jumping to an error label.
> 
> So we should either pass _err to nvme_identify() or make it return
> a success flag so we can run a proper error path.

You're right, that was dumb. :(

Paolo

Re: [Qemu-block] [PATCH] block/nvme: fix Coverity reports

[Kevin Wolf]

Am 09.02.2018 um 16:24 hat Paolo Bonzini geschrieben:
> 1) string not null terminated in sysfs_find_group_file
> 
> 2) NULL pointer dereference and dead local variable in nvme_init.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block/nvme.c| 14 ++
>  util/vfio-helpers.c |  2 +-
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index e9d0e218fc..ce217ffc81 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -553,7 +553,6 @@ static int nvme_init(BlockDriverState *bs, const char 
> *device, int namespace,
>  uint64_t cap;
>  uint64_t timeout_ms;
>  uint64_t deadline, now;
> -Error *local_err = NULL;
>  
>  qemu_co_mutex_init(>dma_map_lock);
>  qemu_co_queue_init(>dma_flush_queue);
> @@ -645,11 +644,6 @@ static int nvme_init(BlockDriverState *bs, const char 
> *device, int namespace,
> false, nvme_handle_event, nvme_poll_cb);
>  
>  nvme_identify(bs, namespace, errp);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -ret = -EIO;
> -goto fail_handler;
> -}
>  
>  /* Set up command queues. */
>  if (!nvme_add_io_queue(bs, errp)) {

I don't think this is right, errp must not be assigned twice, and you
don't want to return 0 when an error is set. Even if we were return the
right return value and errp content, it would be rather surprising to
have an error set without jumping to an error label.

So we should either pass _err to nvme_identify() or make it return
a success flag so we can run a proper error path.

Kevin