[PATCH 0/2] hw/nvme: coverity fixes

[PATCH 0/2] hw/nvme: coverity fixes

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Fix two issues reported by Coverity (CID 1507979 and 1508281).

Klaus Jensen (2):
  hw/nvme: fix memory leak in fdp ruhid parsing
  hw/nvme: fix memory leak in nvme_dsm

 hw/nvme/ctrl.c | 3 +++
 hw/nvme/ns.c   | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH 1/2] hw/nvme: fix memory leak in fdp ruhid parsing

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Coverity reports a memory leak of memory when parsing ruhids at
namespace initialization. Since this is just working memory, not needed
beyond the scope of the functions, fix this by adding a g_autofree
annotation.

Reported-by: Coverity (CID 1507979)
Fixes: 73064edfb864 ("hw/nvme: flexible data placement emulation")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ns.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index cfac960dcf39..547c0b154312 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -399,7 +399,8 @@ static bool nvme_ns_init_fdp(NvmeNamespace *ns, Error **errp)
     NvmeEnduranceGroup *endgrp = ns->endgrp;
     NvmeRuHandle *ruh;
     uint8_t lbafi = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
-    unsigned int *ruhid, *ruhids;
+    g_autofree unsigned int *ruhids = NULL;
+    unsigned int *ruhid;
     char *r, *p, *token;
     uint16_t *ph;
 
-- 
2.39.2

Re: [PATCH 1/2] hw/nvme: fix memory leak in fdp ruhid parsing

[Philippe Mathieu-Daudé]

On 11/4/23 21:04, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Coverity reports a memory leak of memory when parsing ruhids at
> namespace initialization. Since this is just working memory, not needed
> beyond the scope of the functions, fix this by adding a g_autofree
> annotation.
> 
> Reported-by: Coverity (CID 1507979)
> Fixes: 73064edfb864 ("hw/nvme: flexible data placement emulation")
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/nvme/ns.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/2] hw/nvme: fix memory leak in nvme_dsm

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

The iocb (and the allocated memory to hold LBA ranges) leaks if reading
the LBA ranges fails.

Fix this by adding a free and an unref of the iocb.

Reported-by: Coverity (CID 1508281)
Fixes: d7d1474fd85d ("hw/nvme: reimplement dsm to allow cancellation")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 8b7be1420912..ac24eeb5ed5a 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -2619,6 +2619,9 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
         status = nvme_h2c(n, (uint8_t *)iocb->range, sizeof(NvmeDsmRange) * nr,
                           req);
         if (status) {
+            g_free(iocb->range);
+            qemu_aio_unref(iocb);
+
             return status;
         }
 
-- 
2.39.2

Re: [PATCH 2/2] hw/nvme: fix memory leak in nvme_dsm

[Philippe Mathieu-Daudé]

On 11/4/23 21:04, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> The iocb (and the allocated memory to hold LBA ranges) leaks if reading
> the LBA ranges fails.
> 
> Fix this by adding a free and an unref of the iocb.
> 
> Reported-by: Coverity (CID 1508281)
> Fixes: d7d1474fd85d ("hw/nvme: reimplement dsm to allow cancellation")
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/nvme/ctrl.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 8b7be1420912..ac24eeb5ed5a 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -2619,6 +2619,9 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
>           status = nvme_h2c(n, (uint8_t *)iocb->range, sizeof(NvmeDsmRange) * nr,
>                             req);
>           if (status) {
> +            g_free(iocb->range);
> +            qemu_aio_unref(iocb);

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

I note the qemu_aio_FOO() functions are not documented.

> +
>               return status;
>           }
>   

Re: [PATCH 2/2] hw/nvme: fix memory leak in nvme_dsm

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

On Apr 12 11:54, Philippe Mathieu-Daudé wrote:
> On 11/4/23 21:04, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > The iocb (and the allocated memory to hold LBA ranges) leaks if reading
> > the LBA ranges fails.
> > 
> > Fix this by adding a free and an unref of the iocb.
> > 
> > Reported-by: Coverity (CID 1508281)
> > Fixes: d7d1474fd85d ("hw/nvme: reimplement dsm to allow cancellation")
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   hw/nvme/ctrl.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 8b7be1420912..ac24eeb5ed5a 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -2619,6 +2619,9 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
> >           status = nvme_h2c(n, (uint8_t *)iocb->range, sizeof(NvmeDsmRange) * nr,
> >                             req);
> >           if (status) {
> > +            g_free(iocb->range);
> > +            qemu_aio_unref(iocb);
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> I note the qemu_aio_FOO() functions are not documented.
> 

As-in, "you are not supposed to use them" or "this should be documented
at some point"?

Thanks for your reviews Philippe, you're a life-saver :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] hw/nvme: coverity fixes

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

On Apr 11 21:04, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Fix two issues reported by Coverity (CID 1507979 and 1508281).
> 
> Klaus Jensen (2):
>   hw/nvme: fix memory leak in fdp ruhid parsing
>   hw/nvme: fix memory leak in nvme_dsm
> 
>  hw/nvme/ctrl.c | 3 +++
>  hw/nvme/ns.c   | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 

Peter,

Are you willing to merge this for 8.0 without additional reviews?

I'm pretty confident on my fixes.


Thanks,
Klaus

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]