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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

>>>> 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
>