Re: [PATCH] xen/cmdline: Fix buggy strncmp(s, LITERAL, ss - s) construct

["Jan Beulich" <JBeulich@suse.com>]

>>> On 31.12.18 at 18:35, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -61,42 +61,31 @@ static unsigned vpmu_count;
>  
>  static DEFINE_PER_CPU(struct vcpu *, last_vcpu);
>  
> -static int parse_vpmu_param(const char *s, unsigned int len)
> -{
> -    if ( !*s || !len )
> -        return 0;
> -    if ( !strncmp(s, "bts", len) )
> -        vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
> -    else if ( !strncmp(s, "ipc", len) )
> -        vpmu_features |= XENPMU_FEATURE_IPC_ONLY;
> -    else if ( !strncmp(s, "arch", len) )
> -        vpmu_features |= XENPMU_FEATURE_ARCH_ONLY;
> -    else
> -        return 1;
> -    return 0;
> -}
> -
>  static int __init parse_vpmu_params(const char *s)
>  {
> -    const char *sep, *p = s;
> +    const char *ss;
>  
>      switch ( parse_bool(s, NULL) )
>      {
>      case 0:
>          break;
>      default:
> -        for ( ; ; )
> -        {
> -            sep = strchr(p, ',');
> -            if ( sep == NULL )
> -                sep = strchr(p, 0);
> -            if ( parse_vpmu_param(p, sep - p) )
> -                goto error;
> -            if ( !*sep )
> -                /* reached end of flags */
> -                break;
> -            p = sep + 1;
> -        }
> +        do {
> +            ss = strchr(s, ',');
> +            if ( !ss )
> +                ss = strchr(s, '\0');
> +
> +            if ( !cmdline_strcmp(s, "bts") )
> +                vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
> +            else if ( !cmdline_strcmp(s, "ipc") )
> +                vpmu_features |= XENPMU_FEATURE_IPC_ONLY;
> +            else if ( !cmdline_strcmp(s, "arch") )
> +                vpmu_features |= XENPMU_FEATURE_ARCH_ONLY;
> +            else
> +                return -EINVAL;
> +
> +            s = ss + 1;
> +        } while ( *ss );

While presumably also applicable elsewhere, the issue is more
noticeable here because you introduce "ss" anew: It is now
unhelpful (in terms of generated code) to calculate ss before
the various cmdline_strcmp() calls, as the compiler can't know
(despite the const) that what s points to won't change across
those calls, and hence has to calculate ss early (and put it into
a callee saved register or on the stack), as written. If the
calculation was pulled down, only scratch registers would
suffice for the compiler to carry out the calculation.

That said - all of this is boot time only code, so not really
performance critical. It's just that this general structure will
then further proliferate, and the overall binary size is likely
going to be (slightly) larger this way.

> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -227,19 +227,49 @@ int parse_bool(const char *s, const char *e)
>      if ( !len )
>          return -1;
>  
> -    if ( !strncmp("no", s, len) ||
> -         !strncmp("off", s, len) ||
> -         !strncmp("false", s, len) ||
> -         !strncmp("disable", s, len) ||
> -         !strncmp("0", s, len) )
> -        return 0;
> +    switch ( len )
> +    {
> +    case 1:
> +        if ( *s == '1' )
> +            return 1;
> +        else if ( *s == '0' )

The "else" here is pointless (also further down).

> @@ -271,6 +301,29 @@ int parse_boolean(const char *name, const char *s, const char *e)
>      return -1;
>  }
>  
> +int cmdline_strcmp(const char *frag, const char *name)
> +{
> +    while ( 1 )

Could I talk you into using "for ( ; ; )" instead (and then perhaps
moving the two increments up here)? I know gcc doesn't do this,
but in the general case a compiler warning about such constant
conditionals is not an entirely bad or wrong thing, so I prefer to
see such constructs avoided where we reasonably can.

> +    {
> +        int res = (*frag - *name);

With the result of this being implementation defined (due to plain
char's implementation defined - often command line controlled
with an implementation defined default - signedness) I wonder if
this function can really usefully return "int" rather than "bool".

> +        if ( res || *name == '\0' )
> +        {
> +            /*
> +             * NUL in 'name' matching a comma or colon in 'frag' implies
> +             * success.
> +             */
> +            if ( *name == '\0' && (*frag == ',' || *frag == ':') )

There's only a single (unrelated) use of ; as a separator right
now (afaics), but adding it here would seem quite desirable to
me.

Also, speaking of (the lack of) tokenization of the command line
in the caller, wouldn't it make sense to accept white space as
separators here too?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel