[PATCH v2 0/2] rpc: avoid crash when system time jump back
From: BiaoXiang Ye <yebiaoxiang@huawei.com> Using monotonic timer instead of detecting a jump backwards as suggested by Jano and Daniel, thanks for suggestions. BiaoXiang Ye (2): rpc: avoid crash when system time jump back rpc: introduce macro MILLISECONDS_PER_SECOND src/rpc/virkeepalive.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) -- 2.23.0
From: BiaoXiang Ye <yebiaoxiang@huawei.com> Setting the system time backward would lead to a multiplication overflow in function virKeepAliveStart. The function virKeepAliveTimerInternal got the same bug too. Backtrace below: #0 0x0000ffffae898470 in raise () from /usr/lib64/libc.so.6 #1 0x0000ffffae89981c in abort () from /usr/lib64/libc.so.6 #2 0x0000ffffaf9a36a8 in __mulvsi3 () from /usr/lib64/libvirt.so.0 #3 0x0000ffffaf8fd9e8 in virKeepAliveStart (ka=0xaaaaf954ce10, interval=interval entry=0, count=count entry=0) at ../../src/rpc/virkeepalive.c:283 #4 0x0000ffffaf908560 in virNetServerClientStartKeepAlive (client=0xaaaaf954cbe0) at ../../src/rpc/virnetserverclient.c:1628 #5 0x0000aaaac57eb6dc in remoteDispatchConnectSupportsFeature (server=0xaaaaf95309d0, msg=0xaaaaf9549d90, ret=0xffff8c007fc0, args=0xffff8c002e70, rerr=0xffff9ea054a0, client=0xaaaaf954cbe0) at ../../src/remote/remote_daemon_dispatch.c:5063 #6 remoteDispatchConnectSupportsFeatureHelper (server=0xaaaaf95309d0, client=0xaaaaf954cbe0, msg=0xaaaaf9549d90, rerr=0xffff9ea054a0, args=0xffff8c002e70, ret=0xffff8c007fc0) at ./remote/remote_daemon_dispatch_stubs.h:3503 #7 0x0000ffffaf9053a4 in virNetServerProgramDispatchCall(msg=0xaaaaf9549d90, client=0xaaaaf954cbe0, server=0x0, prog=0xaaaaf953a170) at ../../src/rpc/virnetserverprogram.c:451 #8 virNetServerProgramDispatch (prog=0xaaaaf953a170, server=0x0, server entry=0xaaaaf95309d0, client=0xaaaaf954cbe0, msg=0xaaaaf9549d90) at ../../src/rpc/virnetserverprogram.c:306 #9 0x0000ffffaf90a6bc in virNetServerProcessMsg (msg=<optimized out>, prog=<optimized out>, client=<optimized out>, srv=0xaaaaf95309d0) at ../../src/rpc/virnetserver.c:137 #10 virNetServerHandleJob (jobOpaque=0xaaaaf950df80, opaque=0xaaaaf95309d0) at ../../src/rpc/virnetserver.c:154 #11 0x0000ffffaf812e14 in virThreadPoolWorker (opaque=<optimized out>) at ../../src/util/virthreadpool.c:163 #12 0x0000ffffaf81237c in virThreadHelper (data=<optimized out>) at ../../src/util/virthread.c:246 #13 0x0000ffffaea327ac in ?? () from /usr/lib64/libpthread.so.0 #14 0x0000ffffae93747c in ?? () from /usr/lib64/libc.so.6 (gdb) frame 3 #3 0x0000ffffaf8fd9e8 in virKeepAliveStart (ka=0xaaaaf954ce10, interval=interval entry=0, count=count entry=0) at ../../src/rpc/virkeepalive.c:283 283 timeout = ka->interval - delay; (gdb) list 278 now = time(NULL); 279 delay = now - ka->lastPacketReceived; <='delay' got a negative value 280 if (delay > ka->interval) 281 timeout = 0; 282 else 283 timeout = ka->interval - delay; 284 ka->intervalStart = now - (ka->interval - timeout); 285 ka->timer = virEventAddTimeout(timeout * 1000, virKeepAliveTimer, <= multiplication overflow 286 ka, virObjectFreeCallback); 287 if (ka->timer < 0) (gdb) p now $2 = 18288001 (gdb) p ka->lastPacketReceived $3 = 1609430405 Signed-off-by: BiaoXiang Ye <yebiaoxiang@huawei.com> --- src/rpc/virkeepalive.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/rpc/virkeepalive.c b/src/rpc/virkeepalive.c index 860b91b6b1..786f9038ef 100644 --- a/src/rpc/virkeepalive.c +++ b/src/rpc/virkeepalive.c @@ -31,7 +31,7 @@ #include "virprobe.h" #define VIR_FROM_THIS VIR_FROM_RPC - +#define MICROSEC_PER_SEC (1000 * 1000) VIR_LOG_INIT("rpc.keepalive"); struct _virKeepAlive { @@ -40,8 +40,8 @@ struct _virKeepAlive { int interval; unsigned int count; unsigned int countToDeath; - time_t lastPacketReceived; - time_t intervalStart; + gint64 lastPacketReceived; + gint64 intervalStart; int timer; virKeepAliveSendFunc sendCB; @@ -113,7 +113,7 @@ static bool virKeepAliveTimerInternal(virKeepAlivePtr ka, virNetMessagePtr *msg) { - time_t now = time(NULL); + gint64 now = g_get_monotonic_time() / MICROSEC_PER_SEC; int timeval; if (ka->interval <= 0 || ka->intervalStart == 0) @@ -231,9 +231,9 @@ virKeepAliveStart(virKeepAlivePtr ka, unsigned int count) { int ret = -1; - time_t delay; + gint64 delay; int timeout; - time_t now; + gint64 now; virObjectLock(ka); @@ -270,7 +270,7 @@ virKeepAliveStart(virKeepAlivePtr ka, "ka=%p client=%p interval=%d count=%u", ka, ka->client, interval, count); - now = time(NULL); + now = g_get_monotonic_time() / MICROSEC_PER_SEC; delay = now - ka->lastPacketReceived; if (delay > ka->interval) timeout = 0; @@ -375,7 +375,8 @@ virKeepAliveCheckMessage(virKeepAlivePtr ka, virObjectLock(ka); ka->countToDeath = ka->count; - ka->lastPacketReceived = ka->intervalStart = time(NULL); + ka->intervalStart = g_get_monotonic_time() / MICROSEC_PER_SEC; + ka->lastPacketReceived = ka->intervalStart; if (msg->header.prog == KEEPALIVE_PROGRAM && msg->header.vers == KEEPALIVE_PROTOCOL_VERSION && -- 2.23.0
On Tue, 9 Feb 2021 09:19:47 +0000 BiaoxiangYe <yebiaoxiang@huawei.com> wrote:
From: BiaoXiang Ye <yebiaoxiang@huawei.com>
Setting the system time backward would lead to a multiplication overflow in function virKeepAliveStart. The function virKeepAliveTimerInternal got the same bug too.
Backtrace below: #0 0x0000ffffae898470 in raise () from /usr/lib64/libc.so.6 #1 0x0000ffffae89981c in abort () from /usr/lib64/libc.so.6 #2 0x0000ffffaf9a36a8 in __mulvsi3 () from /usr/lib64/libvirt.so.0 #3 0x0000ffffaf8fd9e8 in virKeepAliveStart (ka=0xaaaaf954ce10, interval=interval entry=0, count=count entry=0) at ../../src/rpc/virkeepalive.c:283 #4 0x0000ffffaf908560 in virNetServerClientStartKeepAlive (client=0xaaaaf954cbe0) at ../../src/rpc/virnetserverclient.c:1628 #5 0x0000aaaac57eb6dc in remoteDispatchConnectSupportsFeature (server=0xaaaaf95309d0, msg=0xaaaaf9549d90, ret=0xffff8c007fc0, args=0xffff8c002e70, rerr=0xffff9ea054a0, client=0xaaaaf954cbe0) at ../../src/remote/remote_daemon_dispatch.c:5063 #6 remoteDispatchConnectSupportsFeatureHelper (server=0xaaaaf95309d0, client=0xaaaaf954cbe0, msg=0xaaaaf9549d90, rerr=0xffff9ea054a0, args=0xffff8c002e70, ret=0xffff8c007fc0) at ./remote/remote_daemon_dispatch_stubs.h:3503 #7 0x0000ffffaf9053a4 in virNetServerProgramDispatchCall(msg=0xaaaaf9549d90, client=0xaaaaf954cbe0, server=0x0, prog=0xaaaaf953a170) at ../../src/rpc/virnetserverprogram.c:451 #8 virNetServerProgramDispatch (prog=0xaaaaf953a170, server=0x0, server entry=0xaaaaf95309d0, client=0xaaaaf954cbe0, msg=0xaaaaf9549d90) at ../../src/rpc/virnetserverprogram.c:306 #9 0x0000ffffaf90a6bc in virNetServerProcessMsg (msg=<optimized out>, prog=<optimized out>, client=<optimized out>, srv=0xaaaaf95309d0) at ../../src/rpc/virnetserver.c:137 #10 virNetServerHandleJob (jobOpaque=0xaaaaf950df80, opaque=0xaaaaf95309d0) at ../../src/rpc/virnetserver.c:154 #11 0x0000ffffaf812e14 in virThreadPoolWorker (opaque=<optimized out>) at ../../src/util/virthreadpool.c:163 #12 0x0000ffffaf81237c in virThreadHelper (data=<optimized out>) at ../../src/util/virthread.c:246 #13 0x0000ffffaea327ac in ?? () from /usr/lib64/libpthread.so.0 #14 0x0000ffffae93747c in ?? () from /usr/lib64/libc.so.6 (gdb) frame 3 #3 0x0000ffffaf8fd9e8 in virKeepAliveStart (ka=0xaaaaf954ce10, interval=interval entry=0, count=count entry=0) at ../../src/rpc/virkeepalive.c:283 283 timeout = ka->interval - delay; (gdb) list 278 now = time(NULL); 279 delay = now - ka->lastPacketReceived; <='delay' got a negative value 280 if (delay > ka->interval) 281 timeout = 0; 282 else 283 timeout = ka->interval - delay; 284 ka->intervalStart = now - (ka->interval - timeout); 285 ka->timer = virEventAddTimeout(timeout * 1000, virKeepAliveTimer, <= multiplication overflow 286 ka, virObjectFreeCallback); 287 if (ka->timer < 0) (gdb) p now $2 = 18288001 (gdb) p ka->lastPacketReceived $3 = 1609430405
Signed-off-by: BiaoXiang Ye <yebiaoxiang@huawei.com> --- src/rpc/virkeepalive.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/src/rpc/virkeepalive.c b/src/rpc/virkeepalive.c index 860b91b6b1..786f9038ef 100644 --- a/src/rpc/virkeepalive.c +++ b/src/rpc/virkeepalive.c @@ -31,7 +31,7 @@ #include "virprobe.h"
#define VIR_FROM_THIS VIR_FROM_RPC - +#define MICROSEC_PER_SEC (1000 * 1000)
FYI, glib provides G_USEC_PER_SEC which you could use here instead of defining your own. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
VIR_LOG_INIT("rpc.keepalive");
struct _virKeepAlive { @@ -40,8 +40,8 @@ struct _virKeepAlive { int interval; unsigned int count; unsigned int countToDeath; - time_t lastPacketReceived; - time_t intervalStart; + gint64 lastPacketReceived; + gint64 intervalStart; int timer;
virKeepAliveSendFunc sendCB; @@ -113,7 +113,7 @@ static bool virKeepAliveTimerInternal(virKeepAlivePtr ka, virNetMessagePtr *msg) { - time_t now = time(NULL); + gint64 now = g_get_monotonic_time() / MICROSEC_PER_SEC; int timeval;
if (ka->interval <= 0 || ka->intervalStart == 0) @@ -231,9 +231,9 @@ virKeepAliveStart(virKeepAlivePtr ka, unsigned int count) { int ret = -1; - time_t delay; + gint64 delay; int timeout; - time_t now; + gint64 now;
virObjectLock(ka);
@@ -270,7 +270,7 @@ virKeepAliveStart(virKeepAlivePtr ka, "ka=%p client=%p interval=%d count=%u", ka, ka->client, interval, count);
- now = time(NULL); + now = g_get_monotonic_time() / MICROSEC_PER_SEC; delay = now - ka->lastPacketReceived; if (delay > ka->interval) timeout = 0; @@ -375,7 +375,8 @@ virKeepAliveCheckMessage(virKeepAlivePtr ka, virObjectLock(ka);
ka->countToDeath = ka->count; - ka->lastPacketReceived = ka->intervalStart = time(NULL); + ka->intervalStart = g_get_monotonic_time() / MICROSEC_PER_SEC; + ka->lastPacketReceived = ka->intervalStart;
if (msg->header.prog == KEEPALIVE_PROGRAM && msg->header.vers == KEEPALIVE_PROTOCOL_VERSION &&
From: BiaoXiang Ye <yebiaoxiang@huawei.com> Use macro instead of magic number, keep the style consistent with MICROSEC_PER_SEC. Signed-off-by: BiaoXiang Ye <yebiaoxiang@huawei.com> --- src/rpc/virkeepalive.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/rpc/virkeepalive.c b/src/rpc/virkeepalive.c index 786f9038ef..baea2e6478 100644 --- a/src/rpc/virkeepalive.c +++ b/src/rpc/virkeepalive.c @@ -32,6 +32,7 @@ #define VIR_FROM_THIS VIR_FROM_RPC #define MICROSEC_PER_SEC (1000 * 1000) +#define MILLISECONDS_PER_SECOND (1000) VIR_LOG_INIT("rpc.keepalive"); struct _virKeepAlive { @@ -121,7 +122,7 @@ virKeepAliveTimerInternal(virKeepAlivePtr ka, if (now - ka->intervalStart < ka->interval) { timeval = ka->interval - (now - ka->intervalStart); - virEventUpdateTimeout(ka->timer, timeval * 1000); + virEventUpdateTimeout(ka->timer, timeval * MILLISECONDS_PER_SECOND); return false; } @@ -141,7 +142,7 @@ virKeepAliveTimerInternal(virKeepAlivePtr ka, ka->countToDeath--; ka->intervalStart = now; *msg = virKeepAliveMessage(ka, KEEPALIVE_PROC_PING); - virEventUpdateTimeout(ka->timer, ka->interval * 1000); + virEventUpdateTimeout(ka->timer, ka->interval * MILLISECONDS_PER_SECOND); return false; } } @@ -250,7 +251,7 @@ virKeepAliveStart(virKeepAlivePtr ka, goto cleanup; } /* Guard against overflow */ - if (interval > INT_MAX / 1000) { + if (interval > INT_MAX / MILLISECONDS_PER_SECOND) { virReportError(VIR_ERR_INTERNAL_ERROR, _("keepalive interval %d too large"), interval); goto cleanup; @@ -277,7 +278,8 @@ virKeepAliveStart(virKeepAlivePtr ka, else timeout = ka->interval - delay; ka->intervalStart = now - (ka->interval - timeout); - ka->timer = virEventAddTimeout(timeout * 1000, virKeepAliveTimer, + ka->timer = virEventAddTimeout(timeout * MILLISECONDS_PER_SECOND, + virKeepAliveTimer, ka, virObjectFreeCallback); if (ka->timer < 0) goto cleanup; @@ -327,8 +329,8 @@ virKeepAliveTimeout(virKeepAlivePtr ka) if (timeout < 0) timeout = 0; /* Guard against overflow */ - if (timeout > INT_MAX / 1000) - timeout = INT_MAX / 1000; + if (timeout > INT_MAX / MILLISECONDS_PER_SECOND) + timeout = INT_MAX / MILLISECONDS_PER_SECOND; } virObjectUnlock(ka); @@ -336,7 +338,7 @@ virKeepAliveTimeout(virKeepAlivePtr ka) if (timeout < 0) return -1; else - return timeout * 1000; + return timeout * MILLISECONDS_PER_SECOND; } @@ -403,7 +405,7 @@ virKeepAliveCheckMessage(virKeepAlivePtr ka, } if (ka->timer >= 0) - virEventUpdateTimeout(ka->timer, ka->interval * 1000); + virEventUpdateTimeout(ka->timer, ka->interval * MILLISECONDS_PER_SECOND); virObjectUnlock(ka); -- 2.23.0
On Tue, Feb 09, 2021 at 09:19:48 +0000, BiaoxiangYe wrote:
From: BiaoXiang Ye <yebiaoxiang@huawei.com>
Use macro instead of magic number, keep the style consistent with MICROSEC_PER_SEC.
I'm not sure where's the magic in converting seconds to milliseconds or microseconds to seconds. However, I can see it's a bit easier to spot we're converting seconds and milliseconds rather than milliseconds and microseconds, for example.
Signed-off-by: BiaoXiang Ye <yebiaoxiang@huawei.com> --- src/rpc/virkeepalive.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/src/rpc/virkeepalive.c b/src/rpc/virkeepalive.c index 786f9038ef..baea2e6478 100644 --- a/src/rpc/virkeepalive.c +++ b/src/rpc/virkeepalive.c @@ -32,6 +32,7 @@
#define VIR_FROM_THIS VIR_FROM_RPC #define MICROSEC_PER_SEC (1000 * 1000) +#define MILLISECONDS_PER_SECOND (1000) VIR_LOG_INIT("rpc.keepalive");
Please, keep the empty line above VIR_LOG_INIT. MICROSEC_PER_SEC vs MILLISECONDS_PER_SECOND definitely don't use consistent style. Ideally you could use G_USEC_PER_SEC as Jonathon suggested, but I'm afraid there's no corresponding G_MSEC_PER_SEC to be used in this patch. I guess G_USEC_PER_SEC and VIR_MSEC_PER_SEC could be usable as a workaround... Or just use numbers instead of both macros, they are quite clear anyway. Jirka
participants (3)
-
BiaoxiangYe -
Jiri Denemark -
Jonathon Jongsma