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
>