Re: [Xen-devel] [PATCH v4 6/9] livepatch: Add parsing for the symbol+0x<offset>

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.09.16 at 21:56, <konrad.wilk@xxxxxxxxxx> wrote:
> On Wed, Aug 24, 2016 at 03:08:01AM -0600, Jan Beulich wrote:
>> >>> On 24.08.16 at 04:22, <konrad.wilk@xxxxxxxxxx> 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.

I don't see how it would. This

    plus = strchr(f->name, '+');

comes ahead of any paths leading to the code you quote.

>> > +    /* +<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.

How does the patch name end up in (5 bytes of) the opaque field?
In any event the correctness of deliberately stripping const should
be explained in a comment (if, of course, it can't be avoided in the
first place).

>> 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?

Why not? The ELF spec, iirc, doesn't put any restrictions on what
characters (other than nul of course) can be used in symbol names.
gas actually has received full quoting support a year or two ago,
to no longer needlessly restrict the character set available here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel