[PATCH 0/2] hw/arm/xlnx-zynqmp.c: fix some error-handling code

[PATCH 0/2] hw/arm/xlnx-zynqmp.c: fix some error-handling code

[Peter Maydell]

This series fixes some minor bugs in error-handling paths
in the xlnx-zynqmp device. The first is a memory leak spotted
by Coverity; the second is some missing error-propagation code
that I noticed while writing the first patch.

thanks
-- PMM

Peter Maydell (2):
  hw/arm/xlnx-zynqmp.c: Avoid memory leak in error-return path
  hw/arm/xlnx-zynqmp.c: Add missing error-propagation code

 hw/arm/xlnx-zynqmp.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH 1/2] hw/arm/xlnx-zynqmp.c: Avoid memory leak in error-return path

[Peter Maydell]

In xlnx_zynqmp_realize() if the attempt to realize the SD
controller object fails then the error-return path will leak
the 'bus_name' string. Fix this by deferring the allocation
until after the realize has succeeded.

Fixes: Coverity CID 1421911
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/xlnx-zynqmp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 49f1c8d0de2..a13dbeeacec 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -520,7 +520,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->sata), 0, gic_spi[SATA_INTR]);
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
-        char *bus_name = g_strdup_printf("sd-bus%d", i);
+        char *bus_name;
         SysBusDevice *sbd = SYS_BUS_DEVICE(&s->sdhci[i]);
         Object *sdhci = OBJECT(&s->sdhci[i]);
 
@@ -541,6 +541,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         sysbus_connect_irq(sbd, 0, gic_spi[sdhci_intr[i]]);
 
         /* Alias controller SD bus to the SoC itself */
+        bus_name = g_strdup_printf("sd-bus%d", i);
         object_property_add_alias(OBJECT(s), bus_name, sdhci, "sd-bus",
                                   &error_abort);
         g_free(bus_name);
-- 
2.20.1

[PATCH 2/2] hw/arm/xlnx-zynqmp.c: Add missing error-propagation code

[Peter Maydell]

In some places in xlnx_zynqmp_realize() we were putting an
error into our local Error*, but forgetting to check for
failure and pass it back to the caller. Add the missing code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Spotted while I was writing the previous patch.
---
 hw/arm/xlnx-zynqmp.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index a13dbeeacec..b84d153d56a 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -530,8 +530,20 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
          * - eMMC Specification Version 4.51
          */
         object_property_set_uint(sdhci, 3, "sd-spec-version", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
         object_property_set_uint(sdhci, SDHCI_CAPABILITIES, "capareg", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
         object_property_set_uint(sdhci, UHS_I, "uhs", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
         object_property_set_bool(sdhci, true, "realized", &err);
         if (err) {
             error_propagate(errp, err);
@@ -551,6 +563,10 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         gchar *bus_name;
 
         object_property_set_bool(OBJECT(&s->spi[i]), true, "realized", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
 
         sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi[i]), 0, spi_addr[i]);
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->spi[i]), 0,
@@ -565,6 +581,10 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
     }
 
     object_property_set_bool(OBJECT(&s->qspi), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->qspi), 0, QSPI_ADDR);
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->qspi), 1, LQSPI_ADDR);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->qspi), 0, gic_spi[QSPI_IRQ]);
@@ -619,6 +639,10 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_GDMA_CH; i++) {
         object_property_set_uint(OBJECT(&s->gdma[i]), 128, "bus-width", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
         object_property_set_bool(OBJECT(&s->gdma[i]), true, "realized", &err);
         if (err) {
             error_propagate(errp, err);
-- 
2.20.1

Re: [PATCH 1/2] hw/arm/xlnx-zynqmp.c: Avoid memory leak in error-return path

[Philippe Mathieu-Daudé]

On 3/24/20 2:49 PM, Peter Maydell wrote:
> In xlnx_zynqmp_realize() if the attempt to realize the SD
> controller object fails then the error-return path will leak
> the 'bus_name' string. Fix this by deferring the allocation
> until after the realize has succeeded.
> 
> Fixes: Coverity CID 1421911
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/arm/xlnx-zynqmp.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 49f1c8d0de2..a13dbeeacec 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -520,7 +520,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>       sysbus_connect_irq(SYS_BUS_DEVICE(&s->sata), 0, gic_spi[SATA_INTR]);
>   
>       for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
> -        char *bus_name = g_strdup_printf("sd-bus%d", i);
> +        char *bus_name;
>           SysBusDevice *sbd = SYS_BUS_DEVICE(&s->sdhci[i]);
>           Object *sdhci = OBJECT(&s->sdhci[i]);
>   
> @@ -541,6 +541,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>           sysbus_connect_irq(sbd, 0, gic_spi[sdhci_intr[i]]);
>   
>           /* Alias controller SD bus to the SoC itself */
> +        bus_name = g_strdup_printf("sd-bus%d", i);
>           object_property_add_alias(OBJECT(s), bus_name, sdhci, "sd-bus",
>                                     &error_abort);
>           g_free(bus_name);
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 2/2] hw/arm/xlnx-zynqmp.c: Add missing error-propagation code

[Philippe Mathieu-Daudé]

On 3/24/20 2:49 PM, Peter Maydell wrote:
> In some places in xlnx_zynqmp_realize() we were putting an
> error into our local Error*, but forgetting to check for
> failure and pass it back to the caller. Add the missing code.

This (incorrect) pattern is used in many places, so it might be easier 
to fix it once with a Coccinelle script...


i.e. a quick "git grep -3 object_property_set_uint":

hw/arm/bcm2835_peripherals.c:290
hw/arm/fsl-imx25.c:272
hw/arm/fsl-imx6.c:349
hw/i386/x86.c:110
hw/microblaze/xlnx-zynqmp-pmu.c:88
target/i386/cpu.c:5158

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Spotted while I was writing the previous patch.
> ---
>   hw/arm/xlnx-zynqmp.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index a13dbeeacec..b84d153d56a 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -530,8 +530,20 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>            * - eMMC Specification Version 4.51
>            */
>           object_property_set_uint(sdhci, 3, "sd-spec-version", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
>           object_property_set_uint(sdhci, SDHCI_CAPABILITIES, "capareg", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
>           object_property_set_uint(sdhci, UHS_I, "uhs", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
>           object_property_set_bool(sdhci, true, "realized", &err);
>           if (err) {
>               error_propagate(errp, err);
> @@ -551,6 +563,10 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>           gchar *bus_name;
>   
>           object_property_set_bool(OBJECT(&s->spi[i]), true, "realized", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
>   
>           sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi[i]), 0, spi_addr[i]);
>           sysbus_connect_irq(SYS_BUS_DEVICE(&s->spi[i]), 0,
> @@ -565,6 +581,10 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>       }
>   
>       object_property_set_bool(OBJECT(&s->qspi), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
>       sysbus_mmio_map(SYS_BUS_DEVICE(&s->qspi), 0, QSPI_ADDR);
>       sysbus_mmio_map(SYS_BUS_DEVICE(&s->qspi), 1, LQSPI_ADDR);
>       sysbus_connect_irq(SYS_BUS_DEVICE(&s->qspi), 0, gic_spi[QSPI_IRQ]);
> @@ -619,6 +639,10 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>   
>       for (i = 0; i < XLNX_ZYNQMP_NUM_GDMA_CH; i++) {
>           object_property_set_uint(OBJECT(&s->gdma[i]), 128, "bus-width", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
>           object_property_set_bool(OBJECT(&s->gdma[i]), true, "realized", &err);
>           if (err) {
>               error_propagate(errp, err);
> 

Re: [PATCH 0/2] hw/arm/xlnx-zynqmp.c: fix some error-handling code

[Edgar E. Iglesias]

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

On Tue, Mar 24, 2020 at 2:49 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> This series fixes some minor bugs in error-handling paths
> in the xlnx-zynqmp device. The first is a memory leak spotted
> by Coverity; the second is some missing error-propagation code
> that I noticed while writing the first patch.
>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


>
> thanks
> -- PMM
>
> Peter Maydell (2):
>   hw/arm/xlnx-zynqmp.c: Avoid memory leak in error-return path
>   hw/arm/xlnx-zynqmp.c: Add missing error-propagation code
>
>  hw/arm/xlnx-zynqmp.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> --
> 2.20.1
>
>

[-- Attachment #2: Type: text/html, Size: 1278 bytes --]

Re: [PATCH 1/2] hw/arm/xlnx-zynqmp.c: Avoid memory leak in error-return path

[Alistair Francis]

On Tue, Mar 24, 2020 at 6:50 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In xlnx_zynqmp_realize() if the attempt to realize the SD
> controller object fails then the error-return path will leak
> the 'bus_name' string. Fix this by deferring the allocation
> until after the realize has succeeded.
>
> Fixes: Coverity CID 1421911
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/xlnx-zynqmp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 49f1c8d0de2..a13dbeeacec 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -520,7 +520,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>      sysbus_connect_irq(SYS_BUS_DEVICE(&s->sata), 0, gic_spi[SATA_INTR]);
>
>      for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
> -        char *bus_name = g_strdup_printf("sd-bus%d", i);
> +        char *bus_name;
>          SysBusDevice *sbd = SYS_BUS_DEVICE(&s->sdhci[i]);
>          Object *sdhci = OBJECT(&s->sdhci[i]);
>
> @@ -541,6 +541,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>          sysbus_connect_irq(sbd, 0, gic_spi[sdhci_intr[i]]);
>
>          /* Alias controller SD bus to the SoC itself */
> +        bus_name = g_strdup_printf("sd-bus%d", i);
>          object_property_add_alias(OBJECT(s), bus_name, sdhci, "sd-bus",
>                                    &error_abort);
>          g_free(bus_name);
> --
> 2.20.1
>
>

Re: [PATCH 2/2] hw/arm/xlnx-zynqmp.c: Add missing error-propagation code

[Alistair Francis]

On Tue, Mar 24, 2020 at 6:50 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In some places in xlnx_zynqmp_realize() we were putting an
> error into our local Error*, but forgetting to check for
> failure and pass it back to the caller. Add the missing code.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Spotted while I was writing the previous patch.
> ---
>  hw/arm/xlnx-zynqmp.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index a13dbeeacec..b84d153d56a 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -530,8 +530,20 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>           * - eMMC Specification Version 4.51
>           */
>          object_property_set_uint(sdhci, 3, "sd-spec-version", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
>          object_property_set_uint(sdhci, SDHCI_CAPABILITIES, "capareg", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
>          object_property_set_uint(sdhci, UHS_I, "uhs", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
>          object_property_set_bool(sdhci, true, "realized", &err);
>          if (err) {
>              error_propagate(errp, err);
> @@ -551,6 +563,10 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>          gchar *bus_name;
>
>          object_property_set_bool(OBJECT(&s->spi[i]), true, "realized", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
>
>          sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi[i]), 0, spi_addr[i]);
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->spi[i]), 0,
> @@ -565,6 +581,10 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>      }
>
>      object_property_set_bool(OBJECT(&s->qspi), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->qspi), 0, QSPI_ADDR);
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->qspi), 1, LQSPI_ADDR);
>      sysbus_connect_irq(SYS_BUS_DEVICE(&s->qspi), 0, gic_spi[QSPI_IRQ]);
> @@ -619,6 +639,10 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>
>      for (i = 0; i < XLNX_ZYNQMP_NUM_GDMA_CH; i++) {
>          object_property_set_uint(OBJECT(&s->gdma[i]), 128, "bus-width", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
>          object_property_set_bool(OBJECT(&s->gdma[i]), true, "realized", &err);
>          if (err) {
>              error_propagate(errp, err);
> --
> 2.20.1
>
>