Re: [PATCH 2/6] x86/setup: Rework cmdline_cook() to be compatible with -Wwrite-strings

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/11/2023 8:21 am, Jan Beulich wrote:
> On 20.11.2023 23:49, Andrew Cooper wrote:
>> Constify both cmdline variables in create_dom0() and __start_xen().
>> Initialise Xen's variable to the empty string to simplify the parsing logic.
>>
>> Update cmdline_cook() to take and return const pointers, changing it to have
>> an early exit for a NULL input (which can happen if the mbi-> pointers happen
>> to be 0).
>>
>> Note this only compiles because strstr() launders the const off the pointer
>> when assigning to the mutable kextra, but that logic only mutates the
>> mbi->cmdline buffer.
> And a good static analyzer would spot this. At the very least I think this
> warrants a comment next to that code. But really I'm inclined to re-write
> this to eliminate the issue altogether; I'll try to remember to do so once
> your change has gone in.

This string handling leaves a lot to be desired.

>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -837,9 +837,10 @@ static bool __init loader_is_grub2(const char 
>> *loader_name)
>>      return (p != NULL) && (p[5] != '0');
>>  }
>>  
>> -static char * __init cmdline_cook(char *p, const char *loader_name)
>> +static const char *__init cmdline_cook(const char *p, const char 
>> *loader_name)
>>  {
>> -    p = p ? : "";
>> +    if ( !p )
>> +        return "";
> This change is now needed only for create_dom0(), whereas the call site
> change to __start_xen() is redundant with the change here. Did you
> consider doing a similar transformation in create_dom0(), thus
> eliminating the need for this check altogether? Alternatively I'd like
> to ask that ...

It occurs to me that __va(0) != 0, so this path isn't actually taken,
even when there is a bad mbi-> pointer.

But the mbi information is already processed by us earlier on boot so we
have reasonable expectation that the pointer is good if MBI_CMDLINE is set.

>
>> @@ -885,7 +886,7 @@ static struct domain *__init create_dom0(const module_t 
>> *image,
>>          },
>>      };
>>      struct domain *d;
>> -    char *cmdline;
>> +    const char *cmdline;
>>      domid_t domid;
>>  
>>      if ( opt_dom0_pvh )
>> @@ -971,8 +972,8 @@ static struct domain *__init create_dom0(const module_t 
>> *image,
>>  /* SAF-1-safe */
>>  void __init noreturn __start_xen(unsigned long mbi_p)
>>  {
>> -    const char *memmap_type = NULL, *loader;
>> -    char *cmdline, *kextra;
>> +    const char *memmap_type = NULL, *loader, *cmdline = "";
>> +    char *kextra;
>>      void *bsp_stack;
>>      struct cpu_info *info = get_cpu_info(), *bsp_info;
>>      unsigned int initrdidx, num_parked = 0;
>> @@ -1027,9 +1028,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>                                             : "unknown";
>>  
>>      /* Parse the command-line options. */
>> -    cmdline = cmdline_cook((mbi->flags & MBI_CMDLINE) ?
>> -                           __va(mbi->cmdline) : NULL,
>> -                           loader);
>> +    if ( mbi->flags & MBI_CMDLINE )
>> +        cmdline = cmdline_cook(__va(mbi->cmdline), loader);
>> +
>>      if ( (kextra = strstr(cmdline, " -- ")) != NULL )
>>      {
>>          /*
> ... this last hunk be dropped, along with cmdline's initializer. No need
> for extra code churn when not gaining us anything. (Also but not only
> because the reformatting here is actually beneficial from a readability
> pov imo, the variant with applying the same transformation to create_dom0()
> would seem preferable to me.)

I'll see what I can do.  I definitely do prefer this form, from a
clarity point of view.

~Andrew