| Message ID | 2e893e6e83fdfb24c5f9c4d2da59114cba9a1df8.1725994633.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: address violations of MISRA C Rule 13.6 | expand |
On 10.09.2024 21:06, Federico Serafini wrote: > Refactor the code to improve readability I question this aspect. I'm not the maintainer of this code anymore, so my view probably doesn't matter much here. > and address violations of > MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall > not contain any expression which has potential side effect"). Where's the potential side effect? Since you move ... > --- a/xen/common/efi/runtime.c > +++ b/xen/common/efi/runtime.c > @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info) > info->cfg.addr = __pa(efi_ct); > info->cfg.nent = efi_num_ct; > break; > + > case XEN_FW_EFI_VENDOR: > + { > + XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name = > + guest_handle_cast(info->vendor.name, CHAR16); .. this out, it must be the one. I've looked at it, yet I can't spot anything: #define guest_handle_cast(hnd, type) ({ \ type *_x = (hnd).p; \ (XEN_GUEST_HANDLE_PARAM(type)) { _x }; \ }) As a rule of thumb, when things aren't obvious, please call out the specific aspect / property in descriptions of such patches. Jan
On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote: > On 10.09.2024 21:06, Federico Serafini wrote: > > Refactor the code to improve readability > > I question this aspect. I'm not the maintainer of this code anymore, so > my view probably doesn't matter much here. > > > and address violations of > > MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall > > not contain any expression which has potential side effect"). > > Where's the potential side effect? Since you move ... > > > --- a/xen/common/efi/runtime.c > > +++ b/xen/common/efi/runtime.c > > @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info) > > info->cfg.addr = __pa(efi_ct); > > info->cfg.nent = efi_num_ct; > > break; > > + > > case XEN_FW_EFI_VENDOR: > > + { > > + XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name = > > + guest_handle_cast(info->vendor.name, CHAR16); > > .. this out, it must be the one. I've looked at it, yet I can't spot > anything: > > #define guest_handle_cast(hnd, type) ({ \ > type *_x = (hnd).p; \ > (XEN_GUEST_HANDLE_PARAM(type)) { _x }; \ > }) > > As a rule of thumb, when things aren't obvious, please call out the > specific aspect / property in descriptions of such patches. I guess it's because guest_handle_cast() is a macro, yet it's lowercase so looks like a function? Wasn't there some other MISRA rule about lowercase/uppercase for macro names? And yes, I don't really see why this would violate the side effect rule either.
On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote: > On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote: >> On 10.09.2024 21:06, Federico Serafini wrote: >>> Refactor the code to improve readability >> >> I question this aspect. I'm not the maintainer of this code anymore, so >> my view probably doesn't matter much here. >> >>> and address violations of >>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall >>> not contain any expression which has potential side effect"). >> >> Where's the potential side effect? Since you move ... >> >>> --- a/xen/common/efi/runtime.c >>> +++ b/xen/common/efi/runtime.c >>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info) >>> info->cfg.addr = __pa(efi_ct); >>> info->cfg.nent = efi_num_ct; >>> break; >>> + >>> case XEN_FW_EFI_VENDOR: >>> + { >>> + XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name = >>> + guest_handle_cast(info->vendor.name, CHAR16); >> >> .. this out, it must be the one. I've looked at it, yet I can't spot >> anything: >> >> #define guest_handle_cast(hnd, type) ({ \ >> type *_x = (hnd).p; \ >> (XEN_GUEST_HANDLE_PARAM(type)) { _x }; \ >> }) >> >> As a rule of thumb, when things aren't obvious, please call out the >> specific aspect / property in descriptions of such patches. > > I guess it's because guest_handle_cast() is a macro, yet it's lowercase > so looks like a function? If Eclair didn't look at the macro-expanded code, it wouldn't even see the sizeof(). Hence I don't expect the thing to be mistaken for a function call. > Wasn't there some other MISRA rule about lowercase/uppercase for macro names? I can't recall having run into one, but I also haven't memorized them all. Jan
On 11/09/2024 3:10 pm, Jan Beulich wrote: > On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote: >> On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote: >>> On 10.09.2024 21:06, Federico Serafini wrote: >>>> Refactor the code to improve readability >>> I question this aspect. I'm not the maintainer of this code anymore, so >>> my view probably doesn't matter much here. >>> >>>> and address violations of >>>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall >>>> not contain any expression which has potential side effect"). >>> Where's the potential side effect? Since you move ... >>> >>>> --- a/xen/common/efi/runtime.c >>>> +++ b/xen/common/efi/runtime.c >>>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info) >>>> info->cfg.addr = __pa(efi_ct); >>>> info->cfg.nent = efi_num_ct; >>>> break; >>>> + >>>> case XEN_FW_EFI_VENDOR: >>>> + { >>>> + XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name = >>>> + guest_handle_cast(info->vendor.name, CHAR16); >>> .. this out, it must be the one. I've looked at it, yet I can't spot >>> anything: >>> >>> #define guest_handle_cast(hnd, type) ({ \ >>> type *_x = (hnd).p; \ >>> (XEN_GUEST_HANDLE_PARAM(type)) { _x }; \ >>> }) >>> >>> As a rule of thumb, when things aren't obvious, please call out the >>> specific aspect / property in descriptions of such patches. >> I guess it's because guest_handle_cast() is a macro, yet it's lowercase >> so looks like a function? > If Eclair didn't look at the macro-expanded code, it wouldn't even see > the sizeof(). Hence I don't expect the thing to be mistaken for a function > call. The complaint is a sizeof in guest_handle_okay() being given ({ ... }) to interpret. ({}) can have arbitrary side effects in it, hence the violation. ~Andrew
On 2024-09-11 16:10, Jan Beulich wrote: > On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote: >> On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote: >>> On 10.09.2024 21:06, Federico Serafini wrote: >>>> Refactor the code to improve readability >>> >>> I question this aspect. I'm not the maintainer of this code anymore, >>> so >>> my view probably doesn't matter much here. >>> >>>> and address violations of >>>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall >>>> not contain any expression which has potential side effect"). >>> >>> Where's the potential side effect? Since you move ... >>> >>>> --- a/xen/common/efi/runtime.c >>>> +++ b/xen/common/efi/runtime.c >>>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union >>>> xenpf_efi_info *info) >>>> info->cfg.addr = __pa(efi_ct); >>>> info->cfg.nent = efi_num_ct; >>>> break; >>>> + >>>> case XEN_FW_EFI_VENDOR: >>>> + { >>>> + XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name = >>>> + guest_handle_cast(info->vendor.name, CHAR16); >>> >>> .. this out, it must be the one. I've looked at it, yet I can't spot >>> anything: >>> >>> #define guest_handle_cast(hnd, type) ({ \ >>> type *_x = (hnd).p; \ >>> (XEN_GUEST_HANDLE_PARAM(type)) { _x }; \ >>> }) >>> >>> As a rule of thumb, when things aren't obvious, please call out the >>> specific aspect / property in descriptions of such patches. >> >> I guess it's because guest_handle_cast() is a macro, yet it's >> lowercase >> so looks like a function? > > If Eclair didn't look at the macro-expanded code, it wouldn't even see > the sizeof(). Hence I don't expect the thing to be mistaken for a > function > call. > Looking at the fully preprocessed code [1], there is an assignment to CHAR *_x inside a sizeof(), therefore compat_handle_cast is triggering the violation when used in such a way to be inside the sizeof(). if ( !((!!((((get_cpu_info()->current_vcpu))->domain)->arch.paging.mode & ((1 << 4) << 10))) || ( __builtin_expect(!!(((n)) < (~0U / (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) *)(full_ptr_t)(info-> vendor.name).c; (__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._)))),1) && ((unsigned long)((unsigned long)((void *)( full_ptr_t)(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) *)(full_ptr_t)(info->vendor.name).c; ( __compat_handle_CHAR16) { (full_ptr_t)_x }; })).c) + ((0 + ((n)) * (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info-> vendor.name)._) *)(full_ptr_t)(info->vendor.name).c; (__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) ? (0 + ((n)) * (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) *)(full_ptr_t)(info->vendor.name).c; ( __compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) - 1 : 0)) < ((void)(((get_cpu_info()->current_vcpu))->domain), 0))) ) ) [1] https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/staging/X86_64-BUGSENG/latest/PROJECT.ecd;/by_service/MC3R1.R13.6.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":false,"selector":{"enabled":true,"negated":true,"kind":0,"domain":"message","inputs":[{"enabled":true,"text":"^.*xen/common/efi/runtime\\.c:258\\.15-258\\.31: `sizeof' expression trait"}]}}} >> Wasn't there some other MISRA rule about lowercase/uppercase for macro >> names? > There isn't one imposing this restriction (at least in MISRA C:2012, I haven't checked earlier editions). > I can't recall having run into one, but I also haven't memorized them > all. > > Jan
On 11.09.2024 16:27, Andrew Cooper wrote: > On 11/09/2024 3:10 pm, Jan Beulich wrote: >> On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote: >>> On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote: >>>> On 10.09.2024 21:06, Federico Serafini wrote: >>>>> Refactor the code to improve readability >>>> I question this aspect. I'm not the maintainer of this code anymore, so >>>> my view probably doesn't matter much here. >>>> >>>>> and address violations of >>>>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall >>>>> not contain any expression which has potential side effect"). >>>> Where's the potential side effect? Since you move ... >>>> >>>>> --- a/xen/common/efi/runtime.c >>>>> +++ b/xen/common/efi/runtime.c >>>>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info) >>>>> info->cfg.addr = __pa(efi_ct); >>>>> info->cfg.nent = efi_num_ct; >>>>> break; >>>>> + >>>>> case XEN_FW_EFI_VENDOR: >>>>> + { >>>>> + XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name = >>>>> + guest_handle_cast(info->vendor.name, CHAR16); >>>> .. this out, it must be the one. I've looked at it, yet I can't spot >>>> anything: >>>> >>>> #define guest_handle_cast(hnd, type) ({ \ >>>> type *_x = (hnd).p; \ >>>> (XEN_GUEST_HANDLE_PARAM(type)) { _x }; \ >>>> }) >>>> >>>> As a rule of thumb, when things aren't obvious, please call out the >>>> specific aspect / property in descriptions of such patches. >>> I guess it's because guest_handle_cast() is a macro, yet it's lowercase >>> so looks like a function? >> If Eclair didn't look at the macro-expanded code, it wouldn't even see >> the sizeof(). Hence I don't expect the thing to be mistaken for a function >> call. > > The complaint is a sizeof in guest_handle_okay() being given ({ ... }) > to interpret. > > ({}) can have arbitrary side effects in it, hence the violation. I sincerely hope the tool actually looks inside the ({}). Jan
On 11.09.2024 16:27, Nicola Vetrini wrote: > On 2024-09-11 16:10, Jan Beulich wrote: >> On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote: >>> On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote: >>>> On 10.09.2024 21:06, Federico Serafini wrote: >>>>> Refactor the code to improve readability >>>> >>>> I question this aspect. I'm not the maintainer of this code anymore, >>>> so >>>> my view probably doesn't matter much here. >>>> >>>>> and address violations of >>>>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall >>>>> not contain any expression which has potential side effect"). >>>> >>>> Where's the potential side effect? Since you move ... >>>> >>>>> --- a/xen/common/efi/runtime.c >>>>> +++ b/xen/common/efi/runtime.c >>>>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union >>>>> xenpf_efi_info *info) >>>>> info->cfg.addr = __pa(efi_ct); >>>>> info->cfg.nent = efi_num_ct; >>>>> break; >>>>> + >>>>> case XEN_FW_EFI_VENDOR: >>>>> + { >>>>> + XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name = >>>>> + guest_handle_cast(info->vendor.name, CHAR16); >>>> >>>> .. this out, it must be the one. I've looked at it, yet I can't spot >>>> anything: >>>> >>>> #define guest_handle_cast(hnd, type) ({ \ >>>> type *_x = (hnd).p; \ >>>> (XEN_GUEST_HANDLE_PARAM(type)) { _x }; \ >>>> }) >>>> >>>> As a rule of thumb, when things aren't obvious, please call out the >>>> specific aspect / property in descriptions of such patches. >>> >>> I guess it's because guest_handle_cast() is a macro, yet it's >>> lowercase >>> so looks like a function? >> >> If Eclair didn't look at the macro-expanded code, it wouldn't even see >> the sizeof(). Hence I don't expect the thing to be mistaken for a >> function >> call. >> > > Looking at the fully preprocessed code [1], there is an assignment to > CHAR *_x inside a sizeof(), therefore compat_handle_cast is triggering > the violation when used in such a way to be inside the sizeof(). I can see a number of initializers, but no assignment. Jan > if ( !((!!((((get_cpu_info()->current_vcpu))->domain)->arch.paging.mode > & ((1 << 4) << 10))) || ( > __builtin_expect(!!(((n)) < (~0U / (sizeof(**(({ CHAR16 *_x = > (__typeof__(**(info->vendor.name)._) *)(full_ptr_t)(info-> > vendor.name).c; (__compat_handle_CHAR16) { (full_ptr_t)_x }; > }))._)))),1) && ((unsigned long)((unsigned long)((void *)( > full_ptr_t)(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) > *)(full_ptr_t)(info->vendor.name).c; ( > __compat_handle_CHAR16) { (full_ptr_t)_x }; })).c) + ((0 + ((n)) * > (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info-> > vendor.name)._) *)(full_ptr_t)(info->vendor.name).c; > (__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) ? (0 + ((n)) > * (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) > *)(full_ptr_t)(info->vendor.name).c; ( > __compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) - 1 : 0)) < > ((void)(((get_cpu_info()->current_vcpu))->domain), 0))) > ) )
On 11/09/24 16:57, Jan Beulich wrote: > On 11.09.2024 16:27, Nicola Vetrini wrote: >> On 2024-09-11 16:10, Jan Beulich wrote: >>> On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote: >>>> On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote: >>>>> On 10.09.2024 21:06, Federico Serafini wrote: >>>>>> Refactor the code to improve readability >>>>> >>>>> I question this aspect. I'm not the maintainer of this code anymore, >>>>> so >>>>> my view probably doesn't matter much here. >>>>> >>>>>> and address violations of >>>>>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall >>>>>> not contain any expression which has potential side effect"). >>>>> >>>>> Where's the potential side effect? Since you move ... >>>>> >>>>>> --- a/xen/common/efi/runtime.c >>>>>> +++ b/xen/common/efi/runtime.c >>>>>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union >>>>>> xenpf_efi_info *info) >>>>>> info->cfg.addr = __pa(efi_ct); >>>>>> info->cfg.nent = efi_num_ct; >>>>>> break; >>>>>> + >>>>>> case XEN_FW_EFI_VENDOR: >>>>>> + { >>>>>> + XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name = >>>>>> + guest_handle_cast(info->vendor.name, CHAR16); >>>>> >>>>> .. this out, it must be the one. I've looked at it, yet I can't spot >>>>> anything: >>>>> >>>>> #define guest_handle_cast(hnd, type) ({ \ >>>>> type *_x = (hnd).p; \ >>>>> (XEN_GUEST_HANDLE_PARAM(type)) { _x }; \ >>>>> }) >>>>> >>>>> As a rule of thumb, when things aren't obvious, please call out the >>>>> specific aspect / property in descriptions of such patches. >>>> >>>> I guess it's because guest_handle_cast() is a macro, yet it's >>>> lowercase >>>> so looks like a function? >>> >>> If Eclair didn't look at the macro-expanded code, it wouldn't even see >>> the sizeof(). Hence I don't expect the thing to be mistaken for a >>> function >>> call. >>> >> >> Looking at the fully preprocessed code [1], there is an assignment to >> CHAR *_x inside a sizeof(), therefore compat_handle_cast is triggering >> the violation when used in such a way to be inside the sizeof(). > > I can see a number of initializers, but no assignment. + Stefano in CC. MISRA considers the initialization (even of a local variable) a side effect. This is the reason of the violations. I will send a V2 with a better description. > >> if ( !((!!((((get_cpu_info()->current_vcpu))->domain)->arch.paging.mode >> & ((1 << 4) << 10))) || ( >> __builtin_expect(!!(((n)) < (~0U / (sizeof(**(({ CHAR16 *_x = >> (__typeof__(**(info->vendor.name)._) *)(full_ptr_t)(info-> >> vendor.name).c; (__compat_handle_CHAR16) { (full_ptr_t)_x }; >> }))._)))),1) && ((unsigned long)((unsigned long)((void *)( >> full_ptr_t)(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) >> *)(full_ptr_t)(info->vendor.name).c; ( >> __compat_handle_CHAR16) { (full_ptr_t)_x }; })).c) + ((0 + ((n)) * >> (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info-> >> vendor.name)._) *)(full_ptr_t)(info->vendor.name).c; >> (__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) ? (0 + ((n)) >> * (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._) >> *)(full_ptr_t)(info->vendor.name).c; ( >> __compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) - 1 : 0)) < >> ((void)(((get_cpu_info()->current_vcpu))->domain), 0))) >> ) ) >
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c index d03e5c90ce..acf08dcaa3 100644 --- a/xen/common/efi/runtime.c +++ b/xen/common/efi/runtime.c @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info) info->cfg.addr = __pa(efi_ct); info->cfg.nent = efi_num_ct; break; + case XEN_FW_EFI_VENDOR: + { + XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name = + guest_handle_cast(info->vendor.name, CHAR16); + if ( !efi_fw_vendor ) return -EOPNOTSUPP; + info->vendor.revision = efi_fw_revision; n = info->vendor.bufsz / sizeof(*efi_fw_vendor); - if ( !guest_handle_okay(guest_handle_cast(info->vendor.name, - CHAR16), n) ) + if ( !guest_handle_okay(vendor_name, n) ) return -EFAULT; + for ( i = 0; i < n; ++i ) { if ( __copy_to_guest_offset(info->vendor.name, i, @@ -267,6 +273,8 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info) break; } break; + } + case XEN_FW_EFI_MEM_INFO: for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) {
Refactor the code to improve readability and address violations of MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall not contain any expression which has potential side effect"). No functional change. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- xen/common/efi/runtime.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)