On Wed, Jul 24, 2026, Ján Tomko wrote: > This can lead to duplicit timer IDs - at least at startup, the daemon > sets some long-lasting timers. > > Can we instead change this to use long long int? > > Using unsigned for this would require rewriting more code. > > Jano
You're right, resetting to 1 could cause duplicate timer IDs since the auto-shutdown timer and CPU/memory collection timer are created at startup and live for the entire lifetime of the daemon. I agree that changing to long long int is the correct approach. This requires changing the timer ID type in: - vireventglib.c: nexttimer, virEventGLibTimeout.timer - virevent.h/c: virEventAddTimeout() return type, virEventRemoveTimeout() and virEventUpdateTimeout() parameters - All callers that store timer IDs This changes the public event API (virEventAddTimeout etc.) which is declared in libvirt-event.h. But since timer IDs are opaque handles, the impact on existing callers is just variable type changes (int -> long long). I'll prepare v2 with this approach. [email protected] [email protected] 原始邮件 发件人：Ján Tomko <[email protected]> 发件时间：2026年7月24日 19:28 收件人：Yong Chen <[email protected]> 抄送：devel <[email protected]> 主题：Re: [PATCH 1/2] util: Guard against integer overflow in nexttimer On a Thursday in 2026, Yong Chen via Devel wrote: >The 'nexttimer' variable is a static int that starts at 1 and is >incremented every time a new timeout is registered via >virEventGLibTimeoutAdd. After running for a long period of time >(many months/years with frequent client connections), nexttimer >can overflow past INT_MAX and become negative. > >A negative timer ID causes multiple problems: >1. virKeepAliveStart: 'if (ka->timer < 0)' returns true, skipping > virObjectRef() which is needed to hold the timer's reference to > the keepalive object. This leads to premature object freeing. >2. virKeepAliveStop: 'if (ka->timer > 0)' returns false, skipping > virEventRemoveTimeout(). The GSource is never destroyed, causing > a use-after-free when the timer fires on the freed object. >3. virNetServerClientNew: 'if (client->sockTimer < 0)' returns true, > causing client creation to fail with 'Connection reset by peer'. > >In a production environment running for 1 year and 8 months, >nexttimer overflowed and accumulated 282 million leaked timer >entries, consuming 19.6GB of memory. > >Fix by resetting nexttimer to 1 when it overflows below 1, ensuring >timer IDs are always positive. > >Signed-off-by: Yong Chen <[email protected]> >--- > src/util/vireventglib.c | 3 +++ > 1 file changed, 3 insertions(+) > >diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c >index 6c54f62123..6fe23b3b7d 100644 >--- a/src/util/vireventglib.c >+++ b/src/util/vireventglib.c >@@ -341,6 +341,9 @@ virEventGLibTimeoutAdd(int interval, > > data = g_new0(struct virEventGLibTimeout, 1); > data->timer = nexttimer++; >+ /* Guard against integer overflow: timer IDs must be positive */ >+ if (nexttimer < 1) >+ nexttimer = 1; This can lead to duplicit timer IDs - at least at startup, the daemon sets some long-lasting timers. Can we instead change this to use long long int? Using unsigned for this would require rewriting more code. Jano > data->interval = interval; > data->cb = cb; > data->opaque = opaque; >-- >2.55.0.windows.3 >
