Re: [PULL 24/30] spapr_pci: populate ibm,loc-code

[David Gibson <david@gibson.dropbear.id.au>]

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

On Mon, Aug 09, 2021 at 10:57:00AM +0100, Peter Maydell wrote:
> On Tue, 7 Jul 2015 at 16:49, Alexander Graf <agraf@suse.de> wrote:
> >
> > From: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> >
> > Each hardware instance has a platform unique location code.  The OF
> > device tree that describes a part of a hardware entity must include
> > the “ibm,loc-code” property with a value that represents the location
> > code for that hardware entity.
> >
> > Populate ibm,loc-code.
> 
> Ancient patch, but Coverity has just noticed a bug in it
> which is still present in current QEMU (CID 1460454):
> 
> > +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev)
> > +{
> > +    char *path = NULL, *buf = NULL, *host = NULL;
> > +
> > +    /* Get the PCI VFIO host id */
> > +    host = object_property_get_str(OBJECT(pdev), "host", NULL);
> > +    if (!host) {
> > +        goto err_out;
> > +    }
> > +
> > +    /* Construct the path of the file that will give us the DT location */
> > +    path = g_strdup_printf("/sys/bus/pci/devices/%s/devspec", host);
> > +    g_free(host);
> > +    if (!path || !g_file_get_contents(path, &buf, NULL, NULL)) {
> > +        goto err_out;
> > +    }
> > +    g_free(path);
> 
> Here we create a 'path' string, use it as the argument to
> g_file_get_contents() and then free it (either here or in the err_out path)...
> 
> > +
> > +    /* Construct and read from host device tree the loc-code */
> > +    path = g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf);
> > +    g_free(buf);
> > +    if (!path || !g_file_get_contents(path, &buf, NULL, NULL)) {
> > +        goto err_out;
> > +    }
> > +    return buf;
> 
> ...but here we forget to free it before returning in the success case.
> 
> > +
> > +err_out:
> > +    g_free(path);
> > +    return NULL;
> > +}
> 
> Cleanest fix would be to declare 'path' and 'host' as
>    g_autofree char *path = NULL;
>    g_autofree char *host = NULL;
> and then you can remove all the manual g_free(path) and g_free(host) calls.

Thanks for the report.  I've committed the fix (I hope) below to ppc-for-6.1:

From 70ae61b510dc571c407b28c46498cae60e60ca66 Mon Sep 17 00:00:00 2001
From: David Gibson <david@gibson.dropbear.id.au>
Date: Tue, 10 Aug 2021 14:28:19 +1000
Subject: [PATCH] spapr_pci: Fix leak in spapr_phb_vfio_get_loc_code() with
 g_autofree

This uses g_autofree to simplify logic in spapr_phb_vfio_get_loc_code(),
in the process fixing a leak in one of the paths.  I'm told this fixes
Coverity error CID 1460454

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Fixes: 16b0ea1d852 ("spapr_pci: populate ibm,loc-code")
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7a725855f9..13d806f390 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -782,33 +782,28 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
 
 static char *spapr_phb_vfio_get_loc_code(SpaprPhbState *sphb,  PCIDevice *pdev)
 {
-    char *path = NULL, *buf = NULL, *host = NULL;
+    g_autofree char *path = NULL;
+    g_autofree char *host = NULL;
+    char *buf = NULL;
 
     /* Get the PCI VFIO host id */
     host = object_property_get_str(OBJECT(pdev), "host", NULL);
     if (!host) {
-        goto err_out;
+        return NULL;
     }
 
     /* Construct the path of the file that will give us the DT location */
     path = g_strdup_printf("/sys/bus/pci/devices/%s/devspec", host);
-    g_free(host);
     if (!g_file_get_contents(path, &buf, NULL, NULL)) {
-        goto err_out;
+        return NULL;
     }
-    g_free(path);
 
     /* Construct and read from host device tree the loc-code */
     path = g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf);
-    g_free(buf);
     if (!g_file_get_contents(path, &buf, NULL, NULL)) {
-        goto err_out;
+        return NULL;
     }
     return buf;
-
-err_out:
-    g_free(path);
-    return NULL;
 }
 
 static char *spapr_phb_get_loc_code(SpaprPhbState *sphb, PCIDevice *pdev)
-- 
2.31.1



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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