Re: [Qemu-devel] [PATCH v4 2/3] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit()

[Auger Eric]

Hi Philippe,
On 06/25/2018 06:57 PM, Philippe Mathieu-Daudé wrote:
> Use error_report() + exit() instead of error_setg(&error_fatal),
> as suggested by the "qapi/error.h" documentation:
> 
>    Please don't error_setg(&error_fatal, ...), use error_report() and
>    exit(), because that's more obvious.
> 
> This fixes CID 1352173:
>     "Passing null pointer dt_name to qemu_fdt_node_path, which dereferences 
> it."
> 
> And this also fixes:
> 
>     hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 
> 'node_path') results in a null pointer dereference
>         if (node_path[1]) {
>             ^~~~~~~~~~~~
> 
> Fixes: Coverity CID 1352173 (Dereference after null check)
> Suggested-by: Eric Blake <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
Reviewed-by: Eric Auger <address@hidden>

Thanks

Eric
> ---
> v3:  display error if property required and returned -FDT_ERR_NOTFOUND
> 
>  hw/arm/sysbus-fdt.c | 53 +++++++++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 277ed872e7..0d4c75702c 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -92,16 +92,20 @@ static void copy_properties_from_host(HostProperty 
> *props, int nb_props,
>          r = qemu_fdt_getprop(host_fdt, node_path,
>                               props[i].name,
>                               &prop_len,
> -                             props[i].optional ? &err : &error_fatal);
> +                             &err);
>          if (r) {
>              qemu_fdt_setprop(guest_fdt, nodename,
>                               props[i].name, r, prop_len);
>          } else {
> -            if (prop_len != -FDT_ERR_NOTFOUND) {
> -                /* optional property not returned although property exists */
> -                error_report_err(err);
> -            } else {
> +            if (props[i].optional && prop_len == -FDT_ERR_NOTFOUND) {
> +                /* optional property does not exist */
>                  error_free(err);
> +            } else {
> +                error_report_err(err);
> +            }
> +            if (!props[i].optional) {
> +                /* mandatory property not found: bail out */
> +                exit(1);
>              }
>          }
>      }
> @@ -138,9 +142,9 @@ static void fdt_build_clock_node(void *host_fdt, void 
> *guest_fdt,
>  
>      node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle);
>      if (node_offset <= 0) {
> -        error_setg(&error_fatal,
> -                   "not able to locate clock handle %d in host device tree",
> -                   host_phandle);
> +        error_report("not able to locate clock handle %d in host device 
> tree",
> +                     host_phandle);
> +        exit(1);
>      }
>      node_path = g_malloc(path_len);
>      while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len))
> @@ -149,16 +153,16 @@ static void fdt_build_clock_node(void *host_fdt, void 
> *guest_fdt,
>          node_path = g_realloc(node_path, path_len);
>      }
>      if (ret < 0) {
> -        error_setg(&error_fatal,
> -                   "not able to retrieve node path for clock handle %d",
> -                   host_phandle);
> +        error_report("not able to retrieve node path for clock handle %d",
> +                     host_phandle);
> +        exit(1);
>      }
>  
>      r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
>                           &error_fatal);
>      if (strcmp(r, "fixed-clock")) {
> -        error_setg(&error_fatal,
> -                   "clock handle %d is not a fixed clock", host_phandle);
> +        error_report("clock handle %d is not a fixed clock", host_phandle);
> +        exit(1);
>      }
>  
>      nodename = strrchr(node_path, '/');
> @@ -301,34 +305,37 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, 
> void *opaque)
>  
>      dt_name = sysfs_to_dt_name(vbasedev->name);
>      if (!dt_name) {
> -        error_setg(&error_fatal, "%s incorrect sysfs device name %s",
> -                    __func__, vbasedev->name);
> +        error_report("%s incorrect sysfs device name %s",
> +                     __func__, vbasedev->name);
> +        exit(1);
>      }
>      node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
>                                     &error_fatal);
>      if (!node_path || !node_path[0]) {
> -        error_setg(&error_fatal, "%s unable to retrieve node path for %s/%s",
> -                   __func__, dt_name, vdev->compat);
> +        error_report("%s unable to retrieve node path for %s/%s",
> +                     __func__, dt_name, vdev->compat);
> +        exit(1);
>      }
>  
>      if (node_path[1]) {
> -        error_setg(&error_fatal, "%s more than one node matching %s/%s!",
> -                   __func__, dt_name, vdev->compat);
> +        error_report("%s more than one node matching %s/%s!",
> +                     __func__, dt_name, vdev->compat);
> +        exit(1);
>      }
>  
>      g_free(dt_name);
>  
>      if (vbasedev->num_regions != 5) {
> -        error_setg(&error_fatal, "%s Does the host dt node combine 
> XGBE/PHY?",
> -                   __func__);
> +        error_report("%s Does the host dt node combine XGBE/PHY?", __func__);
> +        exit(1);
>      }
>  
>      /* generate nodes for DMA_CLK and PTP_CLK */
>      r = qemu_fdt_getprop(host_fdt, node_path[0], "clocks",
>                           &prop_len, &error_fatal);
>      if (prop_len != 8) {
> -        error_setg(&error_fatal, "%s clocks property should contain 2 
> handles",
> -                   __func__);
> +        error_report("%s clocks property should contain 2 handles", 
> __func__);
> +        exit(1);
>      }
>      host_clock_phandles = (uint32_t *)r;
>      guest_clock_phandles[0] = qemu_fdt_alloc_phandle(guest_fdt);
>