[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen/cmdline: Fix buggy strncmp(s, LITERAL, ss - s) construct
>>> On 31.12.18 at 18:35, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |