Re: [PATCH 2/2] hw/block/nvme: fix resource leak in nvme_format_ns

[Max Reitz]

On 22.03.21 07:19, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> In nvme_format_ns(), if the namespace is of zero size (which might be
> useless, but not invalid), the `count` variable will leak. Fix this by
> returning early in that case.

When looking at the Coverity report, something else caught my eye: As 
far as I’m aware, blk_aio_pwrite_zeroes() may invoke the CB before 
returning (if blk_do_pwritev_part() returns without yielding).  I don’t 
think that will happen with real hardware (who knows, though), but it 
should be possible to see with the null-co block driver.

nvme_format_ns() doesn’t quite look like it takes that into account. 
For example, because *count starts at 1 and is decremented after the 
while (len) loop, all nvme_aio_format_cb() invocations (if they are 
invoked before their blk_aio_pwrite_zeroes() returns) will see
*count == 2, and thus not free it, or call nvme_enqueue_req_completion().

I don’t know whether the latter is problematic, but not freeing `count` 
doesn’t seem right.  Perhaps this could be addressed by adding a 
condition to the `(*count)--` to see whether `(*count)-- == 1` (or 
rather `--(*count) == 0`), which would indicate that there are no AIO 
functions still in flight?

Max

> Reported-by: Coverity (CID 1451082)
> Fixes: dc04d25e2f3f ("hw/block/nvme: add support for the format nvm command")
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/block/nvme.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 6842b01ab58b..dad275971a84 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -4984,6 +4984,11 @@ static uint16_t nvme_format_ns(NvmeCtrl *n, 
> NvmeNamespace *ns, uint8_t lbaf,
>       ns->status = NVME_FORMAT_IN_PROGRESS;
>   
>       len = ns->size;
> +
> +    if (!len) {
> +        return NVME_SUCCESS;
> +    }
> +
>       offset = 0;
>   
>       count = g_new(int, 1);