xen/arch/x86/dmi_scan.c | 20 ++++++++++---------- xen/arch/x86/mpparse.c | 10 +++++----- 2 files changed, 15 insertions(+), 15 deletions(-)
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.
No functional change.
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
---
xen/arch/x86/dmi_scan.c | 20 ++++++++++----------
xen/arch/x86/mpparse.c | 10 +++++-----
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
index eb65bc86bb..b6edd7a965 100644
--- 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 &&
eps3->length >= sizeof(*eps3) &&
dmi_checksum(eps3, eps3->length)) {
efi_smbios3_address = eps3->address;
@@ -241,13 +241,13 @@ void __init dmi_efi_get_table(const void *smbios, const void *smbios3)
return;
}
- if (eps && memcmp(eps->anchor, "_SM_", 4) == 0 &&
+ if (eps && strncmp(eps->anchor, "_SM_", 4) == 0 &&
eps->length >= sizeof(*eps) &&
dmi_checksum(eps, eps->length)) {
efi_smbios_address = (u32)(long)eps;
efi_smbios_size = eps->length;
- if (memcmp(eps->dmi.anchor, "_DMI_", 5) == 0 &&
+ if (strncmp(eps->dmi.anchor, "_DMI_", 5) == 0 &&
dmi_checksum(&eps->dmi, sizeof(eps->dmi))) {
efi_dmi_address = eps->dmi.address;
efi_dmi_size = eps->dmi.size;
@@ -288,7 +288,7 @@ const char *__init dmi_get_table(paddr_t *base, u32 *len)
for (q = p; q <= p + 0x10000 - sizeof(eps.dmi); q += 16) {
memcpy_fromio(&eps, q, sizeof(eps.dmi));
if (!(instance & 1) &&
- memcmp(eps.dmi.anchor, "_DMI_", 5) == 0 &&
+ strncmp(eps.dmi.anchor, "_DMI_", 5) == 0 &&
dmi_checksum(&eps.dmi, sizeof(eps.dmi))) {
*base = eps.dmi.address;
*len = eps.dmi.size;
@@ -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 &&
eps.smbios3.length >= sizeof(eps.smbios3) &&
q <= p + 0x10000 - eps.smbios3.length &&
dmi_checksum(q, eps.smbios3.length)) {
@@ -370,14 +370,14 @@ static int __init dmi_iterate(void (*decode)(const struct dmi_header *))
for (q = p; q < p + 0x10000; q += 16) {
if (!dmi.size) {
memcpy_fromio(&dmi, q, sizeof(dmi));
- if (memcmp(dmi.anchor, "_DMI_", 5) ||
+ if (strncmp(dmi.anchor, "_DMI_", 5) != 0 ||
!dmi_checksum(&dmi, sizeof(dmi)))
dmi.size = 0;
}
if (!smbios3.length &&
q <= p + 0x10000 - sizeof(smbios3)) {
memcpy_fromio(&smbios3, q, sizeof(smbios3));
- if (memcmp(smbios3.anchor, "_SM3_", 5) ||
+ if (strncmp(smbios3.anchor, "_SM3_", 5) != 0 ||
smbios3.length < sizeof(smbios3) ||
q > p + 0x10000 - smbios3.length ||
!dmi_checksum(q, smbios3.length))
@@ -406,7 +406,7 @@ static int __init dmi_efi_iterate(void (*decode)(const struct dmi_header *))
memcpy_fromio(&eps, p, sizeof(eps));
bt_iounmap(p, sizeof(eps));
- if (memcmp(eps.anchor, "_SM3_", 5) ||
+ if (strncmp(eps.anchor, "_SM3_", 5) != 0 ||
eps.length < sizeof(eps))
break;
@@ -429,7 +429,7 @@ static int __init dmi_efi_iterate(void (*decode)(const struct dmi_header *))
memcpy_fromio(&eps, p, sizeof(eps));
bt_iounmap(p, sizeof(eps));
- if (memcmp(eps.anchor, "_SM_", 4) ||
+ if (strncmp(eps.anchor, "_SM_", 4) != 0 ||
eps.length < sizeof(eps))
return -1;
@@ -437,7 +437,7 @@ static int __init dmi_efi_iterate(void (*decode)(const struct dmi_header *))
if (!p)
return -1;
if (dmi_checksum(p, eps.length) &&
- memcmp(eps.dmi.anchor, "_DMI_", 5) == 0 &&
+ strncmp(eps.dmi.anchor, "_DMI_", 5) == 0 &&
dmi_checksum(&eps.dmi, sizeof(eps.dmi))) {
printk(KERN_INFO "SMBIOS %d.%d present.\n",
eps.major, eps.minor);
diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c
index e74a714f50..c86c38f191 100644
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -303,7 +303,7 @@ static int __init smp_read_mpc(struct mp_config_table *mpc)
int count=sizeof(*mpc);
unsigned char *mpt=((unsigned char *)mpc)+count;
- if (memcmp(mpc->mpc_signature,MPC_SIGNATURE,4)) {
+ if (strncmp(mpc->mpc_signature,MPC_SIGNATURE,4)) {
printk(KERN_ERR "SMP mptable: bad signature [%#x]!\n",
*(u32 *)mpc->mpc_signature);
return 0;
@@ -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;
--
2.25.1On 05/06/2025 12:35 am, 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.
>
> No functional change.
>
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
> ---
> xen/arch/x86/dmi_scan.c | 20 ++++++++++----------
> xen/arch/x86/mpparse.c | 10 +++++-----
> 2 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
> index eb65bc86bb..b6edd7a965 100644
> --- 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 &&
> eps3->length >= sizeof(*eps3) &&
> dmi_checksum(eps3, eps3->length)) {
> efi_smbios3_address = eps3->address;
This is a good example where MISRAs dictats make the code worse rather
than better.
The anchor is a magic number, and memcmp() is the correct check. It
really is "is this byte pattern identical?"
It's just that the byte pattern is chosen to be coherent and logic in
ASCII, so the use of a "string" is also correct.
Previously 4cd66fb56dc697 was done, but that was on the belief that
those where the only two examples.
What variety of compilers has this been tried on? Both Clang and GCC
have warnings about str*() functions on arrays and overflows, and
switching to mem*() was the solution.
~Andrew
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.
> 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?
> --- 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.)
> @@ -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).
> @@ -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.
Jan
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.
>
> > 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?
> > @@ -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
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).
>> > 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
On Fri, 6 Jun 2025, Nicola Vetrini wrote:
> > > > 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.
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
* - `Rule 21.17 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_17.c>`_
- Mandatory
On 06.06.2025 22:49, Stefano Stabellini wrote: > On Fri, 6 Jun 2025, Nicola Vetrini wrote: >>>>> 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. > > 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? Jan
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@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. > > > > 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. From an ECLAIR perspective, I don't know if we can be so selective, and maybe we just need to allow any string literal as argument, in addition to void*. Nicola, would you be able to provide an ECLAIR configuration that would allow string literals as arguments? Is that supported by ECLAIR? this?
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@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. >>> >>> 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
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
>>>> 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
>
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
On 6/5/25 02:31, 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.
>
>> 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?
>
>> --- 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.)
>
>> @@ -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).
>
>> @@ -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.
What about using an array of const uint8_t[] instead of a string literal?
--
Sincerely,
Demi Marie Obenour (she/her/hers)
© 2016 - 2025 Red Hat, Inc.