On Wed, Aug 24, 2016 at 03:08:01AM -0600, Jan Beulich wrote: > >>> On 24.08.16 at 04:22, <konrad.w...@oracle.com> wrote: > > --- a/xen/common/livepatch.c > > +++ b/xen/common/livepatch.c > > @@ -237,13 +237,34 @@ static const char *livepatch_symbols_lookup(unsigned > > long addr, > > static int resolve_old_address(struct livepatch_func *f, > > const struct livepatch_elf *elf) > > { > > + const char *s; > > + char *plus = NULL; > > Pointless initializer.
We need that otherwise this part (which is at the bottom of this function): if ( plus ) { *plus = '+'; f->old_addr += offset; } May be invoked for symbols that that don't have the '+' in them. > > > + unsigned long offset = 0; > > + > > if ( f->old_addr ) > > return 0; > > > > - f->old_addr = (void *)symbols_lookup_by_name(f->name); > > + s = f->name; > > Otoh this could become s'es initializer. <nods> > > > + /* +<offset> */ > > + plus = strchr(f->name, '+'); > > And I think you should prefer using the local variable here. <nods> > > Furthermore you're losing const here - does f->name really point > to memory that doesn't get mapped r/o? Yes. The 'struct livepatch_func' contains the the ->opaque array of 31 bytes (from which we use 5 bytes) which the hypervisor uses to stash the original instructions. > > > + if ( plus ) > > + { > > + const char *endp = NULL; > > Pointless initializer again (or else ... > > > + offset = simple_strtoul(plus + 1, &endp, 16); > > + > > + if ( *endp != '\0' ) > > ... the deref here couldn't be unconditional). > > > + return -EINVAL; > > + > > + /* So that symbol lookup works. */ > > + *plus = '\0'; > > + s = f->name; > > Why? f->name didn't change afaict. Ugh. Stale code. > > Overall - are you sure you want to disallow symbol names containing > + characters? I.e. you don't want to add support for some form of > quoting? Can you actually have + in a function or object? Or are you asking in terms of the '\0' overwritting the '+' ? We put it back later on so that the symbol still has + in it: 274 if ( plus ) 275 { 276 *plus = '+'; 277 f->old_addr += offset; 278 } I will change the comment from 'So that symbol lookup works.' to 'Temporary mutilation of name so that symbol lookup works.' _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel