Message ID | 20240221094317.994454-2-zhao1.liu@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Fix missing ERRP_GUARD() when dereference @errp | expand |
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 machine_set_cfmw() - the caller of > cxl_fixed_memory_window_config() - doesn't get the NULL errp parameter > as the "set" method of object property, cxl_fixed_memory_window_config() > doesn't trigger the dereference issue. > > To follow the requirement of errp, add missing ERRP_GUARD() in > cxl_fixed_memory_window_config(). > > 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/cxl/cxl-host.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c > index 2aa776c79c74..c5f5fcfd64d0 100644 > --- a/hw/cxl/cxl-host.c > +++ b/hw/cxl/cxl-host.c > @@ -26,6 +26,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state, > CXLFixedMemoryWindowOptions *object, > Error **errp) > { > + ERRP_GUARD(); > g_autofree CXLFixedWindow *fw = g_malloc0(sizeof(*fw)); > strList *target; > int i; The dereferences are fw->enc_int_ways = cxl_interleave_ways_enc(fw->num_targets, errp); if (*errp) { return; } and fw->enc_int_gran = cxl_interleave_granularity_enc(object->interleave_granularity, errp); if (*errp) { return; } We check *errp, because neither function returns a suitable error code. Reviewed-by: Markus Armbruster <armbru@redhat.com>
On Wed, Feb 21, 2024 at 12:31:06PM +0100, Markus Armbruster wrote: > Date: Wed, 21 Feb 2024 12:31:06 +0100 > From: Markus Armbruster <armbru@redhat.com> > Subject: Re: [PATCH 1/6] hw/cxl/cxl-host: Fix missing ERRP_GUARD() in > cxl_fixed_memory_window_config() > > 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 machine_set_cfmw() - the caller of > > cxl_fixed_memory_window_config() - doesn't get the NULL errp parameter > > as the "set" method of object property, cxl_fixed_memory_window_config() > > doesn't trigger the dereference issue. > > > > To follow the requirement of errp, add missing ERRP_GUARD() in > > cxl_fixed_memory_window_config(). > > > > 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/cxl/cxl-host.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c > > index 2aa776c79c74..c5f5fcfd64d0 100644 > > --- a/hw/cxl/cxl-host.c > > +++ b/hw/cxl/cxl-host.c > > @@ -26,6 +26,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state, > > CXLFixedMemoryWindowOptions *object, > > Error **errp) > > { > > + ERRP_GUARD(); > > g_autofree CXLFixedWindow *fw = g_malloc0(sizeof(*fw)); > > strList *target; > > int i; > > The dereferences are > > fw->enc_int_ways = cxl_interleave_ways_enc(fw->num_targets, errp); > if (*errp) { > return; > } > > and > > fw->enc_int_gran = > cxl_interleave_granularity_enc(object->interleave_granularity, > errp); > if (*errp) { > return; > } > > We check *errp, because neither function returns a suitable error code. > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > Thanks! I'll attach your description to the commit message. Regards, Zhao
On Wed, 21 Feb 2024 23:04:23 +0800 Zhao Liu <zhao1.liu@linux.intel.com> wrote: > On Wed, Feb 21, 2024 at 12:31:06PM +0100, Markus Armbruster wrote: > > Date: Wed, 21 Feb 2024 12:31:06 +0100 > > From: Markus Armbruster <armbru@redhat.com> > > Subject: Re: [PATCH 1/6] hw/cxl/cxl-host: Fix missing ERRP_GUARD() in > > cxl_fixed_memory_window_config() > > > > 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 machine_set_cfmw() - the caller of > > > cxl_fixed_memory_window_config() - doesn't get the NULL errp parameter > > > as the "set" method of object property, cxl_fixed_memory_window_config() > > > doesn't trigger the dereference issue. > > > > > > To follow the requirement of errp, add missing ERRP_GUARD() in > > > cxl_fixed_memory_window_config(). > > > > > > 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/cxl/cxl-host.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c > > > index 2aa776c79c74..c5f5fcfd64d0 100644 > > > --- a/hw/cxl/cxl-host.c > > > +++ b/hw/cxl/cxl-host.c > > > @@ -26,6 +26,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state, > > > CXLFixedMemoryWindowOptions *object, > > > Error **errp) > > > { > > > + ERRP_GUARD(); > > > g_autofree CXLFixedWindow *fw = g_malloc0(sizeof(*fw)); > > > strList *target; > > > int i; > > > > The dereferences are > > > > fw->enc_int_ways = cxl_interleave_ways_enc(fw->num_targets, errp); > > if (*errp) { > > return; > > } > > > > and > > > > fw->enc_int_gran = > > cxl_interleave_granularity_enc(object->interleave_granularity, > > errp); > > if (*errp) { > > return; > > } > > > > We check *errp, because neither function returns a suitable error code. > > > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > > > Thanks! I'll attach your description to the commit message. Thanks for the explanation in the commit message and to Markus for the cases it affected. Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Regards, > Zhao >
diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c index 2aa776c79c74..c5f5fcfd64d0 100644 --- a/hw/cxl/cxl-host.c +++ b/hw/cxl/cxl-host.c @@ -26,6 +26,7 @@ static void cxl_fixed_memory_window_config(CXLState *cxl_state, CXLFixedMemoryWindowOptions *object, Error **errp) { + ERRP_GUARD(); g_autofree CXLFixedWindow *fw = g_malloc0(sizeof(*fw)); strList *target; int i;