[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Fri, 6 Jun 2025 09:26:52 +0200
  • Autocrypt: addr=jbeulich@xxxxxxxx; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL
  • Cc: Stefano Stabellini <stefano.stabellini@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Alessandro Zucchelli <alessandro.zucchelli@xxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, federico.serafini@xxxxxxxxxxx
  • Delivery-date: Fri, 06 Jun 2025 07:27:10 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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
> 




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.