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

[Peter Maydell]

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
-- PMM