Re: [PATCH] x86: remove memcmp calls non-compliant with Rule 21.16.

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2025-06-06 09:26, Jan Beulich wrote:
> On 06.06.2025 09:12, Nicola Vetrini wrote:
> > On 2025-06-06 01:39, Stefano Stabellini wrote:
> > > On Thu, 5 Jun 2025, Jan Beulich wrote:
> > > > On 05.06.2025 01:35, Stefano Stabellini wrote:
> > > > > From: Alessandro Zucchelli <alessandro.zucchelli@xxxxxxxxxxx>
> > > > >
> > > > > MISRA C Rule 21.16 states the following: "The pointer arguments to
> > > > > the Standard Library function `memcmp' shall point to either a 
> > > > > pointer
> > > > > type, an essentially signed type, an essentially unsigned type, an
> > > > > essentially Boolean type or an essentially enum type".
> > > > >
> > > > > Comparing string literals with char arrays is more appropriately
> > > > > done via strncmp.
> > > > More appropriately - maybe. Yet less efficiently. IOW I view ...
> > > >
> > > > > No functional change.
> > > > ... this as at the edge of not being true.
> > Then our views of what constitutes a functional change clearly differ.
> > If you are concerned about performance the patch may be dropped, but
> > then does it make sense to apply the rule at all? An alternative
> > suggestion might be that of deviating the rule for memcmp applied to
> > string literals in either the first or second argument, or both).
> FTAOD (since Stefano also said it like this) - it's not just "string
> literal". The additional requirement is that the last argument passed
> must equal sizeof(<string literal>) for the comparison to work
> correctly.
>
> Jan
That is due to another (mandatory) rule to prevent UB: Rule 21.18 "The 
size_t argument passed to any function in <string.h> shall have an 
appropriate value", which is also true for memcmp
> > > > > Signed-off-by: Alessandro Zucchelli 
> > > > > <alessandro.zucchelli@xxxxxxxxxxx>
> > > > Missing your own S-o-b.
> > > >
> > > > Also (nit) may I ask that you drop the full stop from the patch
> > > > subject?
> > > I'll add the S-o-B and fix the subject
> > >
> > >
> > > > > --- a/xen/arch/x86/dmi_scan.c
> > > > > +++ b/xen/arch/x86/dmi_scan.c
> > > > > @@ -233,7 +233,7 @@ void __init dmi_efi_get_table(const void 
> > > > > *smbios, const void *smbios3)
> > > > >         const struct smbios_eps *eps = smbios;
> > > > >         const struct smbios3_eps *eps3 = smbios3;
> > > > >
> > > > > -       if (eps3 && memcmp(eps3->anchor, "_SM3_", 5) == 0 &&
> > > > > +       if (eps3 && strncmp(eps3->anchor, "_SM3_", 5) == 0 &&
> > > > Unlike the last example given in the doc, this does not pose the 
> > > > risk
> > > > of
> > > > false "not equal" returns. Considering there's no example there
> > > > exactly
> > > > matching this situation, I'm not convinced a change is actually
> > > > needed.
> > > > (Applies to all other changes here, too.)
> > > If we consider string literals "pointer types", then I think you are
> > > right that this would fall under what is permitted by 21.16. Nicola,
> > > what do you think?
> > While I agree that the result of the comparison is correct either way 
> > in
> > these cases, the rule is written to be simple to apply (i.e., not
> > limited only to those cases that may differ), and in particular in the
> > rationale it is indicated that using memcmp to compare string *may*
> > indicate a mistake. As written above, deviating the string literal
> > comparisons is an option, which can be justified with efficiency
> > concerns, but it goes a bit against the rationale of the rule itself.
> >
> > > > > @@ -302,7 +302,7 @@ const char *__init dmi_get_table(paddr_t *base, 
> > > > > u32 *len)
> > > > >                                 continue;
> > > > >                         memcpy_fromio(&eps.dmi + 1, q + sizeof(eps.dmi),
> > > > >                                       sizeof(eps.smbios3) - sizeof(eps.dmi));
> > > > > -                       if (!memcmp(eps.smbios3.anchor, "_SM3_", 5) &&
> > > > > +                       if (strncmp(eps.smbios3.anchor, "_SM3_", 5) == 0 &&
> > > > Here and below there's a further (style) change, moving from ! to 
> > > > "==
> > > > 0"
> > > > (or from implicit boolean to "!= 0"). As we use the original style 
> > > > in
> > > > many
> > > > other places, some justification for this extra change would be 
> > > > needed
> > > > in
> > > > the description (or these extra adjustments be dropped).
> > > The adjustments can be dropped
> > >
> > >
> > > > > @@ -720,10 +720,10 @@ static void __init efi_check_config(void)
> > > > >         __set_fixmap(FIX_EFI_MPF, PFN_DOWN(efi.mps), __PAGE_HYPERVISOR);
> > > > >         mpf = fix_to_virt(FIX_EFI_MPF) + ((long)efi.mps & (PAGE_SIZE-1));
> > > > >
> > > > > -       if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 &&
> > > > > -           mpf->mpf_length == 1 &&
> > > > > -           mpf_checksum((void *)mpf, 16) &&
> > > > > -	    (mpf->mpf_specification == 1 || mpf->mpf_specification == 4)) 
> > > > > {
> > > > > +       if (strncmp(mpf->mpf_signature, "_MP_", 4) == 0 &&
> > > > > +            mpf->mpf_length == 1 &&
> > > > > +            mpf_checksum((void *)mpf, 16) &&
> > > > > +            (mpf->mpf_specification == 1 || mpf->mpf_specification 
> > > > > == 4)) {
> > > > >                 smp_found_config = true;
> > > > >                 printk(KERN_INFO "SMP MP-table at %08lx\n", efi.mps);
> > > > >                 mpf_found = mpf;
> > > > There are extra (indentation) changes here which ought to be 
> > > > dropped.
> > > Yes
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253