On Mon, Feb 15, 2016 at 7:08 AM, Jan Beulich <jbeul...@suse.com> wrote:
> >>> On 15.02.16 at 14:29, <cz...@bitdefender.com> wrote:
> > On 2/15/2016 2:44 PM, Jan Beulich wrote:
> >>
> >>> switch ( mop->op )
> >>> {
> >>> case XEN_DOMCTL_MONITOR_OP_ENABLE:
> >>> case XEN_DOMCTL_MONITOR_OP_DISABLE:
> >>> /* Check if event type is available. */
> >>> if ( unlikely(!(arch_monitor_get_capabilities(d) & (1 <<
> mop->event))) )
> >>> return -EOPNOTSUPP;
> >>> /* Arch-side handles enable/disable ops. */
> >>> return arch_monitor_domctl_event(d, mop);
> >> Ah, I see now that I've mis-read the default: code further below,
> >> which actually calls arch_monitor_domctl_op(), not ..._event().
> >> However, there's an "undefined behavior" issue with the code
> >> above then when mop->event >= 31 - I think you want to left
> >> shift 1U instead of plain 1, and you need to range check
> >> mop->event first.
> >>
> > Never looked @ that part before, used it the way it was.
> > I suppose that's because "according to the C specification, the result
> > of a bit shift
> > operation on a signed argument gives implementation-defined results, so
> > in/theory/|1U << i|is
> > more portable than|1 << i|" (taken from a stackoverflow post).
>
> Yes.
>
> > After changing 1 to 1U though, I don't understand why we should also
> > range-check mop->event.
> > I'm imagining when (mop->event > 31):
> > * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?)
>
> No, it's plain undefined.
>
> > * in both cases arch_monitor_get_capabilities(d) & (1U << mop->event)
> > would be = 0
> > * in which case we would return -EOPNOTSUPP
> > , no?
>
> And even that would be true only today, and would break once
> bit 31 gets a meaning. Whenever possible we should avoid
> introducing such latent issues.
>
Ah yes, good catch, +1 for doing this extra check.
Thanks,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
