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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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@bugseng.com>
>> >
>> > 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).

>> > Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
>> 
>> 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