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

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

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