[libvirt] [PATCHv2] util: make it more robust to calculate timeout value

[Peter Krempa]

On Fri, May 22, 2015 at 15:52:25 +0800, Wang Yufei wrote:
> From: Zhang Bo <oscar.zhangbo at huawei.com>
> 
> When we change system clock to years ago, a certain CPU may use up 100% cputime.
> The reason is that in function virEventPollCalculateTimeout(), we assign the
> unsigned long long result to an INT variable,
>         *timeout = then - now; // timeout is INT, and then/now are long long
>         if (*timeout < 0)
>             *timeout = 0;
> there's a chance that variable @then minus variable @now may be a very large number
> that overflows INT value expression, then *timeout will be negative and be assigned to 0.
> Next the 'poll' in function virEventPollRunOnce() will get into an 'endless' while loop there.
> thus, the cpu that virEventPollRunOnce() thread runs on will go up to 100%.
> 
> Although as we discussed before in https://www.redhat.com/archives/libvir-list/2015-May/msg00400.html
> it should be prohibited to set-time while other applications are running, but it does
> seems to have no harm to make the codes more robust.
> 
> Signed-off-by: Wang Yufei <james.wangyufei at huawei.com>
> Signed-off-by: Zhang Bo <oscar.zhangbo at huawei.com>
> ---
>  src/util/vireventpoll.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
> index ffda206..9c9e7af 100644
> --- a/src/util/vireventpoll.c
> +++ b/src/util/vireventpoll.c
> @@ -357,9 +357,12 @@ static int virEventPollCalculateTimeout(int *timeout)
>              return -1;
>  
>          EVENT_DEBUG("Schedule timeout then=%llu now=%llu", then, now);
> -        *timeout = then - now;
> -        if (*timeout < 0)
> +        if (then <= now) {
>              *timeout = 0;
> +        } else {
> +            *timeout = (int) (then - now);

This still won't fix the overflow issue since the same implicit typecast
would be done without the explicit one. You probably should clamp the
value to INT_MAX if you want to be safe.

> +            *timeout = (*timeout > 0) ? (*timeout) : (*timeout)*(-1);

This would denote that timeout overflowed, hence you did not fix it at
first.

> +        }
>      } else {
>          *timeout = -1;
>      }

I'm not discussing the previous comments done by DanPB though.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150522/54544555/attachment-0001.sig>