| Message ID | 20231120224912.1421916-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Enable -Wwrite-strings | expand |
On Mon, 20 Nov 2023, 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. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
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. > --- 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 ... > @@ -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.) Jan
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
On 21.11.2023 18:23, Andrew Cooper wrote: > On 21/11/2023 8:21 am, Jan Beulich wrote: >> On 20.11.2023 23:49, Andrew Cooper wrote: >>> --- 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 it is taken when the (remaining) caller passes in NULL explicitly (from the conditional operator ahead of the function invocation). But anyway, I'll go look at v2. Jan
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index c41dfdb2bdf8..a06a241943f6 100644 --- 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 ""; /* Strip leading whitespace. */ while ( *p == ' ' ) @@ -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 ) { /*
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. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> I don't particularly like this, but it's the best I can come up with. --- xen/arch/x86/setup.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)