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