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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.06.2025 04:25, Stefano Stabellini wrote:
> On Tue, 10 Jun 2025, Jan Beulich wrote:
>> On 06.06.2025 22:49, Stefano Stabellini wrote:
>>> On Fri, 6 Jun 2025, Nicola Vetrini wrote:
>>>>>>> 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.
>>>
>>> Also looking at Andrew's reply, it seems that the preference is to
>>> deviate string literals. The change to docs/misra/rules.rst is easy
>>> enough, but I am not sure how to make the corresponding change to
>>> analysis.ecl.
>>>
>>> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
>>> index e1c26030e8..56b6e351df 100644
>>> --- a/docs/misra/rules.rst
>>> +++ b/docs/misra/rules.rst
>>> @@ -813,7 +813,7 @@ maintainers if you want to suggest a change.
>>>         shall point to either a pointer type, an essentially signed type,
>>>         an essentially unsigned type, an essentially Boolean type or an
>>>         essentially enum type
>>> -     - void* arguments are allowed
>>> +     - void* and string literals arguments are allowed
>>
>> Yet as per my earlier reply: This would go too far, wouldn't it?
> 
> You are suggesting:
> 
> "void* arguments are allowed. string literals arguments are allowed when
> the last argument passed for the comparison is equal to the size of the
> string."
> 
> Please suggest another wording if you prefer.

Just some marginal change:

"void* arguments are allowed. string literal arguments are allowed when
 the last argument passed for the comparison is less or equal to the size
 of the string."

Without the "less than" part I expect we'd run into issues when certain
signatures are checked. The size of the string literal includes the nul
terminator, whereas signatures typically don't.

Jan