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 <ebl...@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org> Reviewed-by: Eric Auger <eric.au...@redhat.com>
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); >