Re: [Xen-devel] [PATCH v2 01/13] Export hypervisor symbols

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 09/25/2013 09:15 AM, Jan Beulich wrote:
> > > > On 20.09.13 at 11:42, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> > +    case XENPF_get_symbols:
> > +    {
> > +        ret = xensyms_read(&op->u.symdata);
> > +        if ( ret >= 0 && __copy_field_to_guest(u_xenpf_op, op, u.symdata) )
> > +            ret = -EFAULT;
> > +    }
> > +    break;
> This yields a positive return value if a symbol was found, 0 if none
> was found, and negative on error. Can we avoid this non-standard
> first aspect?
We need to know on the caller side when EOF is reached. There is no good
error code that I can see that would be appropriate here. ERANGE or ENFILE
are the closest I can imagine but EOF is not really an error so I am not 
sure
this would be the right thing.

We could look at first byte of the string and see where it's a 0 but 
that's also
somewhat non-standard. Encoding type or address as an invalid token also
doesn't look nice.

> > +        return 0;
> > +
> > +    spin_lock(&symbols_mutex);
> > +
> > +    if ( symdata->xen_symnum == 0 )
> > +        next_offset = next_symbol = 0;
> > +    else if ( next_symbol != symdata->xen_symnum )
> > +        /* Non-sequential access */
> > +        next_offset = get_symbol_offset(symdata->xen_symnum);
> > +
> > +    symdata->type = symbols_get_symbol_type(next_offset);
> > +    next_offset = symbols_expand_symbol(next_offset, symdata->name,
> > +        sizeof(symdata->name));
> > +    symdata->address = symbols_offsets[symdata->xen_symnum] + SYMBOLS_ORIGIN;
> > +
> > +    next_symbol = symdata->xen_symnum + 1;
> > +
> > +    spin_unlock(&symbols_mutex);
> > +
> > +    return strlen(symdata->name);
> Altogether the changes you do appear to allow the nul terminator
> to be written outside of the passed in array. Hence (a) you need
> to avoid corrupting memory and (b) whether using strlen() here is
> appropriate depends on how you deal with (a).
Yes, I should have passed 'sizeof(symdata->name)-1' to 
symbols_expand_symbol()
(I was thinking of strlen instead of sizeof)

> > +
> > +struct xenpf_symdata {
> > +    /* IN variables */
> > +    uint64_t xen_symnum;
> > +
> > +    /* OUT variables */
> > +    uint64_t address;
> > +    uint64_t type;
> "type" and "xen_symnum" could easily be less than 64 bits wide.
I am trying to avoid 32-bit compatibility issues here. You will see 
sometimes
unnecessary uint64_t in other structures well for the same reason.


-boris

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