Remaining violations of MISRA Rule 7.4

Remaining violations of MISRA Rule 7.4

[Nicola Vetrini]

Hi everyone,

I was looking at leftover violations for MISRA Rule 7.4:
'A string literal shall not be assigned to an object unless the object's 
type
is "pointer to const-qualified char" '

You can see the referenced violations at [1] and [2].

I think the ones in x86/setup.c can be taken care of either by making an 
early return
from cmdline_cook, given that one caller never supplies a NULL cmdline, 
while the other
properly takes care of the possibility of returning NULL, afaict.

  static char * __init cmdline_cook(char *p, const char *loader_name)
  {
-    p = p ? : "";
+    if ( p == NULL )
+        return NULL;

or changing the type of "loader" to const char*

  void __init noreturn __start_xen(unsigned long mbi_p)
  {
-    const char *memmap_type = NULL;
-    char *cmdline, *kextra, *loader;
+    const char *memmap_type = NULL, *loader = NULL;
+    char *cmdline, *kextra;;

as, as far as I can tell, it's never changed after

     loader = (mbi->flags & MBI_LOADERNAME)
         ? (char *)__va(mbi->boot_loader_name) : "unknown";

However, the one in xen/arch/arm/efi/efi-boot.h

name.s = "xen";

does not look to have a clear resolution
path, therefore I propose to deviate this with a SAF textual deviation, 
whose justification
relies on the fact that the string is never modified afterwards.

For the one in arm-uart.c from the discussion, I'm testing possible 
solution with no code
changes, but if that doesn't work out, then I'm inclined towards a 
deviation, as options
is never modified afterwards.

What do you think?

[1] 
https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set1/376/PROJECT.ecd;/by_service/MC3R1.R7.4.html

[2] 
https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/x86_64/staging/X86_64-Set1/376/PROJECT.ecd;/by_service/MC3R1.R7.4.html

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: Remaining violations of MISRA Rule 7.4

[Andrew Cooper]

On 08/11/2023 4:24 pm, Nicola Vetrini wrote:
> Hi everyone,
>
> I was looking at leftover violations for MISRA Rule 7.4:
> 'A string literal shall not be assigned to an object unless the
> object's type
> is "pointer to const-qualified char" '
>
> You can see the referenced violations at [1] and [2].
>
> I think the ones in x86/setup.c can be taken care of either by making
> an early return
> from cmdline_cook, given that one caller never supplies a NULL
> cmdline, while the other
> properly takes care of the possibility of returning NULL, afaict.
>
>  static char * __init cmdline_cook(char *p, const char *loader_name)
>  {
> -    p = p ? : "";
> +    if ( p == NULL )
> +        return NULL;
>
> or changing the type of "loader" to const char*
>
>  void __init noreturn __start_xen(unsigned long mbi_p)
>  {
> -    const char *memmap_type = NULL;
> -    char *cmdline, *kextra, *loader;
> +    const char *memmap_type = NULL, *loader = NULL;
> +    char *cmdline, *kextra;;
>
> as, as far as I can tell, it's never changed after
>
>     loader = (mbi->flags & MBI_LOADERNAME)
>         ? (char *)__va(mbi->boot_loader_name) : "unknown";
>
> However, the one in xen/arch/arm/efi/efi-boot.h
>
> name.s = "xen";
>
> does not look to have a clear resolution
> path, therefore I propose to deviate this with a SAF textual
> deviation, whose justification
> relies on the fact that the string is never modified afterwards.
>
> For the one in arm-uart.c from the discussion, I'm testing possible
> solution with no code
> changes, but if that doesn't work out, then I'm inclined towards a
> deviation, as options
> is never modified afterwards.
>
> What do you think?

I've just rebased and pushed the residual from the past work (although I
missed the ARM EFI fix.)

https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=0f06bab762f5201f3e00aaaee704c3c01f516b51
https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1065699873

I'm going to make a firm request that we fix this by activating
-Wwrite-strings Xen wide, because that's by far and away the best way to
stop regressions creeping back in.

In start_xen(), basically whatever goes.  All that's doing is processing
of one command line into another, and your version looks a bit neater
than mine.

The name.s cases (it's duplicated in x86 and ARM) are more tricky.  The
compiler warning can be silenced by swapping name.s for name.cs but I
have no idea whether Eclair can see through that piece of blatent lying.

~Andrew

Re: Remaining violations of MISRA Rule 7.4

[Nicola Vetrini]

On 2023-11-08 19:45, Andrew Cooper wrote:
> On 08/11/2023 4:24 pm, Nicola Vetrini wrote:
>> Hi everyone,
>> 
>> I was looking at leftover violations for MISRA Rule 7.4:
>> 'A string literal shall not be assigned to an object unless the
>> object's type
>> is "pointer to const-qualified char" '
>> 
>> You can see the referenced violations at [1] and [2].
>> 
>> I think the ones in x86/setup.c can be taken care of either by making
>> an early return
>> from cmdline_cook, given that one caller never supplies a NULL
>> cmdline, while the other
>> properly takes care of the possibility of returning NULL, afaict.
>> 
>>  static char * __init cmdline_cook(char *p, const char *loader_name)
>>  {
>> -    p = p ? : "";
>> +    if ( p == NULL )
>> +        return NULL;
>> 
>> or changing the type of "loader" to const char*
>> 
>>  void __init noreturn __start_xen(unsigned long mbi_p)
>>  {
>> -    const char *memmap_type = NULL;
>> -    char *cmdline, *kextra, *loader;
>> +    const char *memmap_type = NULL, *loader = NULL;
>> +    char *cmdline, *kextra;;
>> 
>> as, as far as I can tell, it's never changed after
>> 
>>     loader = (mbi->flags & MBI_LOADERNAME)
>>         ? (char *)__va(mbi->boot_loader_name) : "unknown";
>> 
>> However, the one in xen/arch/arm/efi/efi-boot.h
>> 
>> name.s = "xen";
>> 
>> does not look to have a clear resolution
>> path, therefore I propose to deviate this with a SAF textual
>> deviation, whose justification
>> relies on the fact that the string is never modified afterwards.
>> 
>> For the one in arm-uart.c from the discussion, I'm testing possible
>> solution with no code
>> changes, but if that doesn't work out, then I'm inclined towards a
>> deviation, as options
>> is never modified afterwards.
>> 
>> What do you think?
> 
> I've just rebased and pushed the residual from the past work (although 
> I
> missed the ARM EFI fix.)
> 
> https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=0f06bab762f5201f3e00aaaee704c3c01f516b51
> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1065699873
> 
> I'm going to make a firm request that we fix this by activating
> -Wwrite-strings Xen wide, because that's by far and away the best way 
> to
> stop regressions creeping back in.
> 

Yes, it seems the best solution. I forgot about that patch, sorry.

   drivers/char/arm-uart.c: In function 'dt_uart_init':
   drivers/char/arm-uart.c:81:17: error: assignment discards 'const' 
qualifier from pointer target type [-Werror=discarded-qualifiers]
      81 |         options = "";
         |                 ^

Here I was thinking about

      options = strchr(opt_dtuart, ':');
      if ( options != NULL )
          *(options++) = '\0';
-    else
-        options = "";

-    printk("Looking for dtuart at \"%s\", options \"%s\"\n", devpath, 
options);
+    printk("Looking for dtuart at \"%s\", options \"%s\"\n", devpath,
+           options ? options : "");

> In start_xen(), basically whatever goes.  All that's doing is 
> processing
> of one command line into another, and your version looks a bit neater
> than mine.
> 
> The name.s cases (it's duplicated in x86 and ARM) are more tricky.  The
> compiler warning can be silenced by swapping name.s for name.cs but I
> have no idea whether Eclair can see through that piece of blatent 
> lying.
> 

No it doesn't, because the type of the lhs is checked, and if it's 
constant then it's enough for the Rule.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: Remaining violations of MISRA Rule 7.4

[Nicola Vetrini]

> I've just rebased and pushed the residual from the past work (although 
> I
> missed the ARM EFI fix.)
> 
> https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=0f06bab762f5201f3e00aaaee704c3c01f516b51
> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1065699873
> 
> I'm going to make a firm request that we fix this by activating
> -Wwrite-strings Xen wide, because that's by far and away the best way 
> to
> stop regressions creeping back in.
> 
> In start_xen(), basically whatever goes.  All that's doing is 
> processing
> of one command line into another, and your version looks a bit neater
> than mine.
> 
> The name.s cases (it's duplicated in x86 and ARM) are more tricky.  The
> compiler warning can be silenced by swapping name.s for name.cs but I
> have no idea whether Eclair can see through that piece of blatent 
> lying.
> 
> ~Andrew

Just to avoid any misunderstanding: do you have the intention of 
evaluating and then perhaps integrate the fixes that at the moment block 
the introduction of -Wwrite-strings and then respin your patch, or 
should I do something specifically?

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: Remaining violations of MISRA Rule 7.4

[Andrew Cooper]

On 20/11/2023 4:40 pm, Nicola Vetrini wrote:
>> I've just rebased and pushed the residual from the past work (although I
>> missed the ARM EFI fix.)
>>
>> https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=0f06bab762f5201f3e00aaaee704c3c01f516b51
>>
>> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1065699873
>>
>> I'm going to make a firm request that we fix this by activating
>> -Wwrite-strings Xen wide, because that's by far and away the best way to
>> stop regressions creeping back in.
>>
>> In start_xen(), basically whatever goes.  All that's doing is processing
>> of one command line into another, and your version looks a bit neater
>> than mine.
>>
>> The name.s cases (it's duplicated in x86 and ARM) are more tricky.  The
>> compiler warning can be silenced by swapping name.s for name.cs but I
>> have no idea whether Eclair can see through that piece of blatent lying.
>>
>> ~Andrew
>
> Just to avoid any misunderstanding: do you have the intention of
> evaluating and then perhaps integrate the fixes that at the moment
> block the introduction of -Wwrite-strings and then respin your patch,
> or should I do something specifically?
>

Hopefully you'll find
https://lore.kernel.org/xen-devel/20231120224912.1421916-1-andrew.cooper3@citrix.com/T/#u
to your liking.

~Andrew

Re: Remaining violations of MISRA Rule 7.4

[Nicola Vetrini]

On 2023-11-21 00:08, Andrew Cooper wrote:
> On 20/11/2023 4:40 pm, Nicola Vetrini wrote:
>>> I've just rebased and pushed the residual from the past work 
>>> (although I
>>> missed the ARM EFI fix.)
>>> 
>>> https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=0f06bab762f5201f3e00aaaee704c3c01f516b51
>>> 
>>> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1065699873
>>> 
>>> I'm going to make a firm request that we fix this by activating
>>> -Wwrite-strings Xen wide, because that's by far and away the best way 
>>> to
>>> stop regressions creeping back in.
>>> 
>>> In start_xen(), basically whatever goes.  All that's doing is 
>>> processing
>>> of one command line into another, and your version looks a bit neater
>>> than mine.
>>> 
>>> The name.s cases (it's duplicated in x86 and ARM) are more tricky.  
>>> The
>>> compiler warning can be silenced by swapping name.s for name.cs but I
>>> have no idea whether Eclair can see through that piece of blatent 
>>> lying.
>>> 
>>> ~Andrew
>> 
>> Just to avoid any misunderstanding: do you have the intention of
>> evaluating and then perhaps integrate the fixes that at the moment
>> block the introduction of -Wwrite-strings and then respin your patch,
>> or should I do something specifically?
>> 
> 
> Hopefully you'll find
> https://lore.kernel.org/xen-devel/20231120224912.1421916-1-andrew.cooper3@citrix.com/T/#u
> to your liking.
> 
> ~Andrew

Definitely. Thank you for the effort you put on this.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)