| Message ID | 54949b0561263b9f18da500255836d89ca8838ba.1719308599.git.alessandro.zucchelli@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | address violation of MISRA C Rule 13.6 | expand |
On 25.06.2024 12:14, Alessandro Zucchelli wrote: > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -660,14 +660,15 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > case XENVER_guest_handle: > { > + struct domain *d = current->domain; Can a (new) variable thus initialized please be consistently named currd? > xen_domain_handle_t hdl; > > if ( deny ) > memset(&hdl, 0, ARRAY_SIZE(hdl)); > > - BUILD_BUG_ON(ARRAY_SIZE(current->domain->handle) != ARRAY_SIZE(hdl)); > + BUILD_BUG_ON(ARRAY_SIZE(d->handle) != ARRAY_SIZE(hdl)); Wasn't there the intention to exclude BUILD_BUG_ON() for specifically this (and any other similar) rule? Jan > - if ( copy_to_guest(arg, deny ? hdl : current->domain->handle, > + if ( copy_to_guest(arg, deny ? hdl : d->handle, > ARRAY_SIZE(hdl) ) ) > return -EFAULT; > return 0;
On Tue, 25 Jun 2024, Jan Beulich wrote: > On 25.06.2024 12:14, Alessandro Zucchelli wrote: > > --- a/xen/common/kernel.c > > +++ b/xen/common/kernel.c > > @@ -660,14 +660,15 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > > > case XENVER_guest_handle: > > { > > + struct domain *d = current->domain; > > Can a (new) variable thus initialized please be consistently named currd? > > > xen_domain_handle_t hdl; > > > > if ( deny ) > > memset(&hdl, 0, ARRAY_SIZE(hdl)); > > > > - BUILD_BUG_ON(ARRAY_SIZE(current->domain->handle) != ARRAY_SIZE(hdl)); > > + BUILD_BUG_ON(ARRAY_SIZE(d->handle) != ARRAY_SIZE(hdl)); > > Wasn't there the intention to exclude BUILD_BUG_ON() for specifically this > (and any other similar) rule? +1 I think if we could do that it would be ideal because those are the difficult cases are only meant are build checks so there is no point in applying to MISRA to them. > > - if ( copy_to_guest(arg, deny ? hdl : current->domain->handle, > > + if ( copy_to_guest(arg, deny ? hdl : d->handle, > > ARRAY_SIZE(hdl) ) ) > > return -EFAULT; > > return 0; >
On 2024-06-26 03:13, Stefano Stabellini wrote: Hi, > On Tue, 25 Jun 2024, Jan Beulich wrote: >> On 25.06.2024 12:14, Alessandro Zucchelli wrote: >> > --- a/xen/common/kernel.c >> > +++ b/xen/common/kernel.c >> > @@ -660,14 +660,15 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> > >> > case XENVER_guest_handle: >> > { >> > + struct domain *d = current->domain; >> >> Can a (new) variable thus initialized please be consistently named >> currd? >> >> > xen_domain_handle_t hdl; >> > >> > if ( deny ) >> > memset(&hdl, 0, ARRAY_SIZE(hdl)); >> > >> > - BUILD_BUG_ON(ARRAY_SIZE(current->domain->handle) != ARRAY_SIZE(hdl)); >> > + BUILD_BUG_ON(ARRAY_SIZE(d->handle) != ARRAY_SIZE(hdl)); >> >> Wasn't there the intention to exclude BUILD_BUG_ON() for specifically >> this >> (and any other similar) rule? > > +1 Yes, this macro will be deviated, you may ignore this patch. > > I think if we could do that it would be ideal because those are the > difficult cases are only meant are build checks so there is no point in > applying to MISRA to them. > > >> > - if ( copy_to_guest(arg, deny ? hdl : current->domain->handle, >> > + if ( copy_to_guest(arg, deny ? hdl : d->handle, >> > ARRAY_SIZE(hdl) ) ) >> > return -EFAULT; >> > return 0; >>
diff --git a/xen/common/kernel.c b/xen/common/kernel.c index b44b2439ca..76b4f27aef 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -660,14 +660,15 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) case XENVER_guest_handle: { + struct domain *d = current->domain; xen_domain_handle_t hdl; if ( deny ) memset(&hdl, 0, ARRAY_SIZE(hdl)); - BUILD_BUG_ON(ARRAY_SIZE(current->domain->handle) != ARRAY_SIZE(hdl)); + BUILD_BUG_ON(ARRAY_SIZE(d->handle) != ARRAY_SIZE(hdl)); - if ( copy_to_guest(arg, deny ? hdl : current->domain->handle, + if ( copy_to_guest(arg, deny ? hdl : d->handle, ARRAY_SIZE(hdl) ) ) return -EFAULT; return 0;
In the file common/kernel.c macro ARRAY_SIZE is called with argument current->domain->handle. Once expanded, this ARRAY_SIZE's argument is used in sizeof operations and thus 'current', being a macro that expands to a function call with potential side effects, generates a violation. To address this violation the value of current->domain is therefore stored in a variable called 'd' before passing it to macro ARRAY_SIZE. No functional change. Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> --- xen/common/kernel.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)