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

[Martin Kletzander]

On Mon, May 25, 2015 at 02:22:42PM +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 | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
>index ffda206..4e4f407 100644
>--- a/src/util/vireventpoll.c
>+++ b/src/util/vireventpoll.c
>@@ -357,9 +357,10 @@ 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 = ((then-now) > INT_MAX) ? INT_MAX : (then-now);

This will work, although the following would be nicer to read since
there were such problems with the calculation in earlier versions:

  if (then <= now) {
      *timeout = 0;
  } else {
      long long tmp = then - now;
      if (tmp > INT_MAX)
          *timeout = INT_MAX
      else
          *timeout = tmp;
  }

But that, of course, has the same meaning as your version, which is
fine.  ACK to that, I'll push it in a while.

>     } else {
>         *timeout = -1;
>     }
>--
>1.7.12.4
>
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150527/9c124954/attachment-0001.sig>