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
