| Message ID | 20180621114015.9207-3-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | qapi/error: converts error_setg(&error_fatal) to error_report() + exit() | expand |
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 <eblake@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > --- > 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? 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); >
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 <eblake@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> --- >> 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); >> >
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); + } } } } @@ -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);
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 <eblake@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Reviewed-by: Markus Armbruster <armbru@redhat.com> --- hw/arm/sysbus-fdt.c | 45 ++++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 19 deletions(-)