| Message ID | 20240221094317.994454-5-zhao1.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix missing ERRP_GUARD() when dereference @errp | expand |
On Wed, Feb 21, 2024 at 12:47:33PM +0100, Markus Armbruster wrote: > Date: Wed, 21 Feb 2024 12:47:33 +0100 > From: Markus Armbruster <armbru@redhat.com> > Subject: Re: [PATCH 4/6] hw/misc/xlnx-versal-trng: Fix missing ERRP_GUARD() > in trng_prop_fault_event_set() > > Zhao Liu <zhao1.liu@linux.intel.com> writes: > > > From: Zhao Liu <zhao1.liu@intel.com> > > > > As the comment in qapi/error, dereferencing @errp requires > > ERRP_GUARD(): > > > > * = Why, when and how to use ERRP_GUARD() = > > * > > * Without ERRP_GUARD(), use of the @errp parameter is restricted: > > * - It must not be dereferenced, because it may be null. > > * - It should not be passed to error_prepend() or > > * error_append_hint(), because that doesn't work with &error_fatal. > > * ERRP_GUARD() lifts these restrictions. > > * > > * To use ERRP_GUARD(), add it right at the beginning of the function. > > * @errp can then be used without worrying about the argument being > > * NULL or &error_fatal. > > * > > * Using it when it's not needed is safe, but please avoid cluttering > > * the source with useless code. > > > > Currently, since trng_prop_fault_event_set() doesn't get the NULL errp > > parameter as a "set" method of object property, it doesn't trigger the > > dereference issue. > > > > To follow the requirement of errp, add missing ERRP_GUARD() in > > trng_prop_fault_event_set(). > > > > Suggested-by: Markus Armbruster <armbru@redhat.com> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > --- > > Suggested by credit: > > Markus: Referred his explanation about ERRP_GUARD(). > > --- > > hw/misc/xlnx-versal-trng.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/hw/misc/xlnx-versal-trng.c b/hw/misc/xlnx-versal-trng.c > > index b8111b8b6626..3579348a9d17 100644 > > --- a/hw/misc/xlnx-versal-trng.c > > +++ b/hw/misc/xlnx-versal-trng.c > > @@ -33,6 +33,7 @@ > > #include "qemu/error-report.h" > > #include "qemu/guest-random.h" > > #include "qemu/timer.h" > > +#include "qapi/error.h" > > #include "qapi/visitor.h" > > #include "migration/vmstate.h" > > #include "hw/qdev-properties.h" > > @@ -641,6 +642,7 @@ static void trng_prop_fault_event_set(Object *obj, Visitor *v, > > const char *name, void *opaque, > > Error **errp) > > { > > + ERRP_GUARD(); > > Property *prop = opaque; > > uint32_t *events = object_field_prop_ptr(obj, prop); > > visit_type_uint32(v, name, events, errp); > if (*errp) { > return; > } > > Please do this instead: > > diff --git a/hw/misc/xlnx-versal-trng.c b/hw/misc/xlnx-versal-trng.c > index b8111b8b66..6495188dc7 100644 > --- a/hw/misc/xlnx-versal-trng.c > +++ b/hw/misc/xlnx-versal-trng.c > @@ -644,8 +644,7 @@ static void trng_prop_fault_event_set(Object *obj, Visitor *v, > Property *prop = opaque; > uint32_t *events = object_field_prop_ptr(obj, prop); > > - visit_type_uint32(v, name, events, errp); > - if (*errp) { > + if (!visit_type_uint32(v, name, events, errp)) { > return; > } > Thanks! I didn't think of that. Will do this. Regards, Zhao
diff --git a/hw/misc/xlnx-versal-trng.c b/hw/misc/xlnx-versal-trng.c index b8111b8b6626..3579348a9d17 100644 --- a/hw/misc/xlnx-versal-trng.c +++ b/hw/misc/xlnx-versal-trng.c @@ -33,6 +33,7 @@ #include "qemu/error-report.h" #include "qemu/guest-random.h" #include "qemu/timer.h" +#include "qapi/error.h" #include "qapi/visitor.h" #include "migration/vmstate.h" #include "hw/qdev-properties.h" @@ -641,6 +642,7 @@ static void trng_prop_fault_event_set(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { + ERRP_GUARD(); Property *prop = opaque; uint32_t *events = object_field_prop_ptr(obj, prop);