[Qemu-devel] [PATCH] pc-dimm: fix error handling in pc_dimm_check_memdev_is_busy()

[Qemu-devel] [PATCH] pc-dimm: fix error handling in pc_dimm_check_memdev_is_busy()

[Igor Mammedov]

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(-)

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)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] pc-dimm: fix error handling in pc_dimm_check_memdev_is_busy()

[Markus Armbruster]

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 :)

Re: [Qemu-devel] [PATCH] pc-dimm: fix error handling in pc_dimm_check_memdev_is_busy()

[Igor Mammedov]

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.