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

[Philippe Mathieu-Daudé]

Hi Eric,

On 06/21/2018 10:02 AM, Auger Eric wrote:
> Hi Philippe,
> On 06/21/2018 01:40 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: Markus Armbruster <address@hidden>
>> ---
>>  hw/arm/sysbus-fdt.c | 45 ++++++++++++++++++++++++++-------------------
>>  1 file changed, 26 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index 277ed872e7..63b74a353e 100644
>> --- a/hw/arm/sysbus-fdt.c
>> +++ b/hw/arm/sysbus-fdt.c
>> @@ -92,7 +92,7 @@ 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);
>> @@ -103,6 +103,10 @@ static void copy_properties_from_host(HostProperty 
>> *props, int nb_props,
>>              } else {
>>                  error_free(err);
>>              }
>> +            if (!props[i].optional) {
>> +                /* mandatory property not found: bail out */
>> +                exit(1);
> reading that code again, I am not sure this does the same thing as
> before. In case case the property is tagged not optional and is not
> found in the host dt, qemu_fdt_getprop will fill err with error_setg.
> However prop_len == -FDT_ERR_NOTFOUND so we are going to call error_free
> and no trace will be output to the user. Before the
> error_setg(&error_fatal) printed the cause of error error and exited. Do
> I miss something?

You are correct something is wrong here, I'll respin.

Thanks for your review!

> 
> Thanks
> 
> Eric
>> +            }
>>          }
>>      }
>>  }
>> @@ -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);
>>
>