| Message ID | bc3a28abf9f00bf67cf5ee64bd89e7d38e321c06.1690449587.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | address violations of MISRA C:2012 Rule 5.3 on x86 | expand |
On 27.07.2023 12:48, Nicola Vetrini wrote: > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -2024,15 +2024,15 @@ static int cf_check hvmemul_rep_stos( > > switch ( p2mt ) > { > - unsigned long bytes; > char *buf; > > default: > /* Allocate temporary buffer. */ > for ( ; ; ) > { > - bytes = *reps * bytes_per_rep; > - buf = xmalloc_bytes(bytes); > + unsigned long bytes_tmp; > + bytes_tmp = *reps * bytes_per_rep; > + buf = xmalloc_bytes(bytes_tmp); > if ( buf || *reps <= 1 ) > break; > *reps >>= 1; This wants dealing with differently - the outer scope variable is unused (only written to) afaics. Eliminating it will, aiui, address another violation at the same time. And then the same in hvmemul_rep_movs(), just that there the variable itself needs to survive. I guess I'll make a patch ... Jan
On 27/07/23 17:06, Jan Beulich wrote: > On 27.07.2023 12:48, Nicola Vetrini wrote: >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -2024,15 +2024,15 @@ static int cf_check hvmemul_rep_stos( >> >> switch ( p2mt ) >> { >> - unsigned long bytes; >> char *buf; >> >> default: >> /* Allocate temporary buffer. */ >> for ( ; ; ) >> { >> - bytes = *reps * bytes_per_rep; >> - buf = xmalloc_bytes(bytes); >> + unsigned long bytes_tmp; >> + bytes_tmp = *reps * bytes_per_rep; >> + buf = xmalloc_bytes(bytes_tmp); >> if ( buf || *reps <= 1 ) >> break; >> *reps >>= 1; > > This wants dealing with differently - the outer scope variable is unused > (only written to) afaics. Eliminating it will, aiui, address another > violation at the same time. And then the same in hvmemul_rep_movs(), just > that there the variable itself needs to survive. I guess I'll make a > patch ... > > Jan Wouldn't this code at line ~2068 be possibly affected by writing to bytes, if the outer variable is used? /* Adjust address for reverse store. */ if ( df ) gpa -= bytes - bytes_per_rep; rc = hvm_copy_to_guest_phys(gpa, buf, bytes, curr); You're right about the other violation (R2.1)
On 27.07.2023 17:22, Nicola Vetrini wrote: > > > On 27/07/23 17:06, Jan Beulich wrote: >> On 27.07.2023 12:48, Nicola Vetrini wrote: >>> --- a/xen/arch/x86/hvm/emulate.c >>> +++ b/xen/arch/x86/hvm/emulate.c >>> @@ -2024,15 +2024,15 @@ static int cf_check hvmemul_rep_stos( >>> >>> switch ( p2mt ) >>> { >>> - unsigned long bytes; >>> char *buf; >>> >>> default: >>> /* Allocate temporary buffer. */ >>> for ( ; ; ) >>> { >>> - bytes = *reps * bytes_per_rep; >>> - buf = xmalloc_bytes(bytes); >>> + unsigned long bytes_tmp; >>> + bytes_tmp = *reps * bytes_per_rep; >>> + buf = xmalloc_bytes(bytes_tmp); >>> if ( buf || *reps <= 1 ) >>> break; >>> *reps >>= 1; >> >> This wants dealing with differently - the outer scope variable is unused >> (only written to) afaics. Eliminating it will, aiui, address another >> violation at the same time. And then the same in hvmemul_rep_movs(), just >> that there the variable itself needs to survive. I guess I'll make a >> patch ... > > Wouldn't this code at line ~2068 be possibly affected by writing to > bytes, if the outer variable is used? Which outer variable? I'm suggesting to drop that (see the patch that I've sent already). Jan > /* Adjust address for reverse store. */ > if ( df ) > gpa -= bytes - bytes_per_rep; > > rc = hvm_copy_to_guest_phys(gpa, buf, bytes, curr); > > You're right about the other violation (R2.1) >
On 27/07/23 17:31, Jan Beulich wrote: > On 27.07.2023 17:22, Nicola Vetrini wrote: >> >> >> On 27/07/23 17:06, Jan Beulich wrote: >>> On 27.07.2023 12:48, Nicola Vetrini wrote: >>>> --- a/xen/arch/x86/hvm/emulate.c >>>> +++ b/xen/arch/x86/hvm/emulate.c >>>> @@ -2024,15 +2024,15 @@ static int cf_check hvmemul_rep_stos( >>>> >>>> switch ( p2mt ) >>>> { >>>> - unsigned long bytes; >>>> char *buf; >>>> >>>> default: >>>> /* Allocate temporary buffer. */ >>>> for ( ; ; ) >>>> { >>>> - bytes = *reps * bytes_per_rep; >>>> - buf = xmalloc_bytes(bytes); >>>> + unsigned long bytes_tmp; >>>> + bytes_tmp = *reps * bytes_per_rep; >>>> + buf = xmalloc_bytes(bytes_tmp); >>>> if ( buf || *reps <= 1 ) >>>> break; >>>> *reps >>= 1; >>> >>> This wants dealing with differently - the outer scope variable is unused >>> (only written to) afaics. Eliminating it will, aiui, address another >>> violation at the same time. And then the same in hvmemul_rep_movs(), just >>> that there the variable itself needs to survive. I guess I'll make a >>> patch ... >> >> Wouldn't this code at line ~2068 be possibly affected by writing to >> bytes, if the outer variable is used? > > Which outer variable? I'm suggesting to drop that (see the patch that > I've sent already). > > Jan > >> /* Adjust address for reverse store. */ >> if ( df ) >> gpa -= bytes - bytes_per_rep; >> >> rc = hvm_copy_to_guest_phys(gpa, buf, bytes, curr); >> >> You're right about the other violation (R2.1) >> > I see, sorry for the noise.
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 75ee98a73b..0d41928ff3 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -2024,15 +2024,15 @@ static int cf_check hvmemul_rep_stos( switch ( p2mt ) { - unsigned long bytes; char *buf; default: /* Allocate temporary buffer. */ for ( ; ; ) { - bytes = *reps * bytes_per_rep; - buf = xmalloc_bytes(bytes); + unsigned long bytes_tmp; + bytes_tmp = *reps * bytes_per_rep; + buf = xmalloc_bytes(bytes_tmp); if ( buf || *reps <= 1 ) break; *reps >>= 1;
The declaration of local variable 'bytes' in 'hvmemul_rep_stos' causes the shadowing of the same variable defined in the enclosing scope, hence the declaration has been moved inside the scope where it's used, with a different name. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/arch/x86/hvm/emulate.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)