[15/21] s390x/cpu_models: Fix latent feature property error handling bugs
diff mbox series

Message ID 20191130194240.10517-16-armbru@redhat.com
State New
Headers show
Series
  • Error handling fixes, may contain 4.2 material
Related show

Commit Message

Markus Armbruster Nov. 30, 2019, 7:42 p.m. UTC
s390x-cpu property setters set_feature() and set_feature_group() crash
when the visitor fails and its @errp argument is null.  Messed up in
commit 0754f60429 "s390x/cpumodel: expose features and feature groups
as properties".

The bug can't bite as no caller actually passes null.  Fix it anyway.

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 target/s390x/cpu_models.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

David Hildenbrand Dec. 2, 2019, 9:57 a.m. UTC | #1
On 30.11.19 20:42, Markus Armbruster wrote:
> s390x-cpu property setters set_feature() and set_feature_group() crash
> when the visitor fails and its @errp argument is null.  Messed up in
> commit 0754f60429 "s390x/cpumodel: expose features and feature groups
> as properties".

Same comment as to the other patches :)

I think we usually use "s390x/cpumodels", but that's just nitpicking.

Reviewed-by: David Hildenbrand <david@redhat.com>

> 
> The bug can't bite as no caller actually passes null.  Fix it anyway.
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  target/s390x/cpu_models.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 7e92fb2e15..6a29fd3ab1 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -987,6 +987,7 @@ static void get_feature(Object *obj, Visitor *v, const char *name,
>  static void set_feature(Object *obj, Visitor *v, const char *name,
>                          void *opaque, Error **errp)
>  {
> +    Error *err = NULL;
>      S390Feat feat = (S390Feat) opaque;
>      DeviceState *dev = DEVICE(obj);
>      S390CPU *cpu = S390_CPU(obj);
> @@ -1002,8 +1003,9 @@ static void set_feature(Object *obj, Visitor *v, const char *name,
>          return;
>      }
>  
> -    visit_type_bool(v, name, &value, errp);
> -    if (*errp) {
> +    visit_type_bool(v, name, &value, &err);
> +    if (err) {
> +        error_propagate(errp, err);
>          return;
>      }
>      if (value) {
> @@ -1043,6 +1045,7 @@ static void get_feature_group(Object *obj, Visitor *v, const char *name,
>  static void set_feature_group(Object *obj, Visitor *v, const char *name,
>                                void *opaque, Error **errp)
>  {
> +    Error *err = NULL;
>      S390FeatGroup group = (S390FeatGroup) opaque;
>      const S390FeatGroupDef *def = s390_feat_group_def(group);
>      DeviceState *dev = DEVICE(obj);
> @@ -1059,8 +1062,9 @@ static void set_feature_group(Object *obj, Visitor *v, const char *name,
>          return;
>      }
>  
> -    visit_type_bool(v, name, &value, errp);
> -    if (*errp) {
> +    visit_type_bool(v, name, &value, &err);
> +    if (err) {
> +        error_propagate(errp, err);
>          return;
>      }
>      if (value) {
>
Markus Armbruster Dec. 3, 2019, 7:22 a.m. UTC | #2
David Hildenbrand <david@redhat.com> writes:

> On 30.11.19 20:42, Markus Armbruster wrote:
>> s390x-cpu property setters set_feature() and set_feature_group() crash
>> when the visitor fails and its @errp argument is null.  Messed up in
>> commit 0754f60429 "s390x/cpumodel: expose features and feature groups
>> as properties".
>
> Same comment as to the other patches :)
>
> I think we usually use "s390x/cpumodels", but that's just nitpicking.

$ git-log --oneline target/s390x/cpu_models.c | awk '$2 ~ /:$/ { print $2 }' | sort | uniq -c
      1 S390:
      6 qapi:
      1 qemu-common:
      1 qmp:
      2 qobject:
      1 qom:
      1 s390/cpumodel:
      1 s390x/ccw:
     21 s390x/cpumodel:
      1 s390x/cpumodels:
      1 s390x/kvm:
      4 s390x/tcg:
      7 s390x:
      1 target/s390x/cpu_models:
     17 target/s390x:
      1 target:

You're right, except for the plural vs. singular.  I should've browsed
git-log.

> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!

Patch
diff mbox series

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 7e92fb2e15..6a29fd3ab1 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -987,6 +987,7 @@  static void get_feature(Object *obj, Visitor *v, const char *name,
 static void set_feature(Object *obj, Visitor *v, const char *name,
                         void *opaque, Error **errp)
 {
+    Error *err = NULL;
     S390Feat feat = (S390Feat) opaque;
     DeviceState *dev = DEVICE(obj);
     S390CPU *cpu = S390_CPU(obj);
@@ -1002,8 +1003,9 @@  static void set_feature(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    visit_type_bool(v, name, &value, errp);
-    if (*errp) {
+    visit_type_bool(v, name, &value, &err);
+    if (err) {
+        error_propagate(errp, err);
         return;
     }
     if (value) {
@@ -1043,6 +1045,7 @@  static void get_feature_group(Object *obj, Visitor *v, const char *name,
 static void set_feature_group(Object *obj, Visitor *v, const char *name,
                               void *opaque, Error **errp)
 {
+    Error *err = NULL;
     S390FeatGroup group = (S390FeatGroup) opaque;
     const S390FeatGroupDef *def = s390_feat_group_def(group);
     DeviceState *dev = DEVICE(obj);
@@ -1059,8 +1062,9 @@  static void set_feature_group(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    visit_type_bool(v, name, &value, errp);
-    if (*errp) {
+    visit_type_bool(v, name, &value, &err);
+    if (err) {
+        error_propagate(errp, err);
         return;
     }
     if (value) {