[PATCH 1/2] util: Guard against integer overflow in nexttimer
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 <410664530@qq.com> --- 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; data->interval = interval; data->cb = cb; data->opaque = opaque; -- 2.55.0.windows.3
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 <410664530@qq.com> --- 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
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. 410664530@qq.com 410664530@qq.com 原始邮件 发件人：Ján Tomko <jtomko@redhat.com> 发件时间：2026年7月24日 19:28 收件人：Yong Chen <410664530@qq.com> 抄送：devel <devel@lists.libvirt.org> 主题：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 <410664530@qq.com> >--- > 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 >
On Tue, Jul 28, 2026 at 12:21:11PM +0800, 410664530 via Devel wrote:
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.
No, you cannot change public API in this way. A variable type size change is an ABI breakage. With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
On Thu, Jul 23, 2026 at 07:47:56PM +0800, 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.
That's a surprisingly high usage of timers. IIUC that works out at the daemon using 7,000,000 per day, or 5000 per minute. What workload are you imposing on the daemon to hit that kind of incredibly high rate of timer ID increments ?
Fix by resetting nexttimer to 1 when it overflows below 1, ensuring timer IDs are always positive.
Signed-off-by: Yong Chen <410664530@qq.com> --- 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;
If we need to guard against this, then we need to stop using a simple integer counter, and instead track a list of ranges of unused timer IDs. A fair bit more complex, but it would allow us to have the full set of 2^32 concurrent timers active. Initially we would have a free range start=0 end=4294967296 After some time allocating timers, this ends up with a free range start=10 end=4294967296 if we then free 4 timers in the middle, we might get two ranges start=4 end=7 -- start=10 end=4294967296 and so on, as we free timers, we either decrement the 'start' of an existing range, or have to add a new range to the list. Always allocating from the lowest range would stop us getting too many new ranges over time.
data->interval = interval; data->cb = cb; data->opaque = opaque; -- 2.55.0.windows.3
With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
participants (4)
-
410664530 -
Daniel P. Berrangé -
Ján Tomko -
Yong Chen