Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout event callback race

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Jim Fehlig writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix stale timeout event 
callback race"):
> Ian Jackson wrote:
> > This is a forwards-compatible change for applications using the libxl
> > API, and will hopefully eliminate these races in callback-supplying
> > applications (such as libvirt) without the need for corresponding
> > changes to the application.

When I wrote this of course I forgot to mention that previously libxl
would never call libxl_osevent_hooks->timeout_modify and now it never
calls ->timeout_deregister.

So this change can expose bugs in the application's implementation of
->timeout_modify.

> I'm not sure how to avoid changing the application (libvirt). Currently,
> the libvirt libxl driver removes the timeout from the libvirt event loop
> in the timeout_deregister() function. This function is never called now
> and hence the timeout is never removed. I had to make the following
> change in the libxl driver to use your patches

I think this is because of a bug of the kind I describe above.

> - gettimeofday(&now, NULL);
> - timeout = (abs_t.tv_usec - now.tv_usec) / 1000;
> - timeout += (abs_t.tv_sec - now.tv_sec) * 1000;

Specifically, this code has an integer arithmetic overflow.

If libxl calls this function with a value of abs_t which is more than
about INT_MAX/1000 seconds (24 days on a 32-bit machine) in the past,
the calculation (abs_t.tv_sec - now.tv_sec) * 1000; overflows.

And the patch makes libxl always pass abs_t={0,0} which is more than
24 days ago unless the code is running during January 1970 :-).

> - virEventUpdateTimeout(timer_info->id, timeout);

Also, what does this do if timeout is negative ?

Thanks,
Ian.

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