Re: [PATCH] system/rtc: Fix a possible year-2038 integer overflow problem
Thomas Huth <[email protected]> Mon, 18 May 2026 18:30:11 +0200
| Newsgroups | org.nongnu.qemu-trivial,org.nongnu.qemu-devel |
|---|---|
| Message-ID | <[email protected]> |
On 18/05/2026 18.18, Daniel P. Berrangé wrote: > On Mon, May 18, 2026 at 05:10:36PM +0100, Daniel P. Berrangé wrote: >> On Mon, May 18, 2026 at 06:02:55PM +0200, Thomas Huth wrote: >>> From: Thomas Huth <[email protected]> >>> >>> rtc_realtime_clock_offset is initialized with: >>> >>> rtc_realtime_clock_offset = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000; >>> >>> And QEMU_CLOCK_REALTIME might be based on gettimeofday() in certain >>> cases (see get_clock_realtime() in include/qemu/timer.h). So this >>> counter will exceed 32 bits in the year 2038, thus we should not >>> store this value in a normal integer variable. Change it to an int64_t >>> to fix the problem. >>> And while we're at it, also adjust the nearby rtc_host_datetime_offset >>> variable to be on the safe side in the related code. >>> >>> Signed-off-by: Thomas Huth <[email protected]> >>> --- >>> system/rtc.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> Reviewed-by: Daniel P. Berrangé <[email protected]> > > Actually on second reading I think this patch is not quite right. > The code which uses the value is: > > time_t value = qemu_clock_get_ms(clock) / 1000; > switch (clock) { > case QEMU_CLOCK_REALTIME: > value -= rtc_realtime_clock_offset; > > > On 64-bit platforms 'int' was 64-bit already and so was time_t so we > have no bug to fix. Uh, did you mix this up with "long" ? > On 32-bit platforms the patch fixes 'int' to 'int64' which is fine > but that int64 value is then subtracted from time_t which is usually > going to be 32-bit again, unless the OS was built with 64-bit time_t > on 32-bit which almost no one does. > > IOW we only platform this fixes is 32-bit OS with 64-bit time_t. > > We can't do any better than that as the caller code paths need a > time_t to pass into gmtime/localtime. > > So IMHO we should just declare it as time_t. Fine for me, too. I'll send a v2... Thomas