| Message ID | 1456750898-152238-1-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Igor Mammedov <imammedo@redhat.com> writes: > if host_memory_backend_get_memory() were to return error and Start sentences with a capital letter, please. > NULL MemoryRegion, pc_dimm_check_memdev_is_busy() would crash > dereferrencing null pointer in memory_region_is_mapped() dereferencing > > Also pc_dimm_check_memdev_is_busy():error_setg() would assert > if caller passes NULL errp, but assert shouldn't happen as > the check is typically performed during hotplug. Huh? > > To avoid above issues use typical error handling pattern > for property setters: > > Error *local_error = NULL; > ... > error_propagate(errp, local_err); > > Reported-by: Markus Armbruster <armbru@redhat.com> The latent bug I reported was actually that if host_memory_backend_get_memory() sets an error and we then reach error_setg(), we fail the "error already set" assertion in error_setv() unless errp is null. > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/mem/pc-dimm.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index 650f0f8..973bf20 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -364,15 +364,22 @@ static void pc_dimm_check_memdev_is_busy(Object *obj, const char *name, > Object *val, Error **errp) > { > MemoryRegion *mr; > + Error *local_err = NULL; > > - mr = host_memory_backend_get_memory(MEMORY_BACKEND(val), errp); > + mr = host_memory_backend_get_memory(MEMORY_BACKEND(val), &local_err); > + if (local_err) { > + goto out; > + } > if (memory_region_is_mapped(mr)) { > char *path = object_get_canonical_path_component(val); > - error_setg(errp, "can't use already busy memdev: %s", path); > + error_setg(&local_err, "can't use already busy memdev: %s", path); > g_free(path); > } else { > - qdev_prop_allow_set_link_before_realize(obj, name, val, errp); > + qdev_prop_allow_set_link_before_realize(obj, name, val, &local_err); > } > + > +out: > + error_propagate(errp, local_err); > } > > static void pc_dimm_init(Object *obj) I'd error_propagate() + return instead of goto. But your version isn't wrong, so: Reviewed-by: Markus Armbruster <armbru@redhat.com> Preferably with an improved commit message, of course :)
On Mon, 29 Feb 2016 19:33:15 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Igor Mammedov <imammedo@redhat.com> writes: > > > if host_memory_backend_get_memory() were to return error and > > Start sentences with a capital letter, please. > > > NULL MemoryRegion, pc_dimm_check_memdev_is_busy() would crash > > dereferrencing null pointer in memory_region_is_mapped() > > dereferencing > > > > > Also pc_dimm_check_memdev_is_busy():error_setg() would assert > > if caller passes NULL errp, but assert shouldn't happen as > > the check is typically performed during hotplug. > > Huh? Yep, this paragraph is wrong, I'll drop it. > > > > > To avoid above issues use typical error handling pattern > > for property setters: > > > > Error *local_error = NULL; > > ... > > error_propagate(errp, local_err); > > > > Reported-by: Markus Armbruster <armbru@redhat.com> > > The latent bug I reported was actually that if > host_memory_backend_get_memory() sets an error and we then reach > error_setg(), we fail the "error already set" assertion in error_setv() > unless errp is null. > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/mem/pc-dimm.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > > index 650f0f8..973bf20 100644 > > --- a/hw/mem/pc-dimm.c > > +++ b/hw/mem/pc-dimm.c > > @@ -364,15 +364,22 @@ static void pc_dimm_check_memdev_is_busy(Object *obj, const char *name, > > Object *val, Error **errp) > > { > > MemoryRegion *mr; > > + Error *local_err = NULL; > > > > - mr = host_memory_backend_get_memory(MEMORY_BACKEND(val), errp); > > + mr = host_memory_backend_get_memory(MEMORY_BACKEND(val), &local_err); > > + if (local_err) { > > + goto out; > > + } > > if (memory_region_is_mapped(mr)) { > > char *path = object_get_canonical_path_component(val); > > - error_setg(errp, "can't use already busy memdev: %s", path); > > + error_setg(&local_err, "can't use already busy memdev: %s", path); > > g_free(path); > > } else { > > - qdev_prop_allow_set_link_before_realize(obj, name, val, errp); > > + qdev_prop_allow_set_link_before_realize(obj, name, val, &local_err); > > } > > + > > +out: > > + error_propagate(errp, local_err); > > } > > > > static void pc_dimm_init(Object *obj) > > I'd error_propagate() + return instead of goto. But your version isn't > wrong, so: > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > Preferably with an improved commit message, of course :) Thanks, I'll respin v2 with fixed commit message.
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index 650f0f8..973bf20 100644 --- a/hw/mem/pc-dimm.c +++ b/hw/mem/pc-dimm.c @@ -364,15 +364,22 @@ static void pc_dimm_check_memdev_is_busy(Object *obj, const char *name, Object *val, Error **errp) { MemoryRegion *mr; + Error *local_err = NULL; - mr = host_memory_backend_get_memory(MEMORY_BACKEND(val), errp); + mr = host_memory_backend_get_memory(MEMORY_BACKEND(val), &local_err); + if (local_err) { + goto out; + } if (memory_region_is_mapped(mr)) { char *path = object_get_canonical_path_component(val); - error_setg(errp, "can't use already busy memdev: %s", path); + error_setg(&local_err, "can't use already busy memdev: %s", path); g_free(path); } else { - qdev_prop_allow_set_link_before_realize(obj, name, val, errp); + qdev_prop_allow_set_link_before_realize(obj, name, val, &local_err); } + +out: + error_propagate(errp, local_err); } static void pc_dimm_init(Object *obj)
if host_memory_backend_get_memory() were to return error and NULL MemoryRegion, pc_dimm_check_memdev_is_busy() would crash dereferrencing null pointer in memory_region_is_mapped() Also pc_dimm_check_memdev_is_busy():error_setg() would assert if caller passes NULL errp, but assert shouldn't happen as the check is typically performed during hotplug. To avoid above issues use typical error handling pattern for property setters: Error *local_error = NULL; ... error_propagate(errp, local_err); Reported-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/mem/pc-dimm.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)