Bug in UTC -> localtime conversion after 2038

Bug in UTC -> localtime conversion after 2038

[Udo de Boer]

Hi,

UTC time to local timezone conversion did not work for years after 2038. 
Daylight saving in the timezone was not recognized. Tested on 32bit 
xtensa architecture with GCC 8.4 (esp32) and with 64bit time_t enabled.

The struct tm is filled correct. So year, day etc are all filled with 
the correct values. But the tm.tm_isdst is 0 and no daylight saving is 
applied.

It was caused by the following calculation in time/tzcalc_limits.c at 
line 68

        /* store the change-over time in GMT form by adding offset */
        tz->__tzrule[i].change = days * SECSPERDAY +
        tz->__tzrule[i].s + tz->__tzrule[i].offset;

Here tz->__tzrule[i].change is a time_t (64bit). For some reason the 
compiler does the calculation in 32 bit. All variables used in the 
calculation are 32bit.

I solved this locally by changing the lines to.

        /* store the change-over time in GMT form by adding offset */
        tz->__tzrule[i].change = (time_t)days * SECSPERDAY +
        tz->__tzrule[i].s + tz->__tzrule[i].offset;

Adding the cast time_t should not cause a problem when time_t is still 
32bit.

I don't know if this is correct solution. I also only needed this 
function. So more bugs like this could be hidden away in the time code.

Re: Bug in UTC -> localtime conversion after 2038

[Richard Damon]

On 11/4/20 6:36 AM, Udo de Boer wrote:
> Hi,
>
> UTC time to local timezone conversion did not work for years after
> 2038. Daylight saving in the timezone was not recognized. Tested on
> 32bit xtensa architecture with GCC 8.4 (esp32) and with 64bit time_t
> enabled.
>
> The struct tm is filled correct. So year, day etc are all filled with
> the correct values. But the tm.tm_isdst is 0 and no daylight saving is
> applied.
>
> It was caused by the following calculation in time/tzcalc_limits.c at
> line 68
>
>        /* store the change-over time in GMT form by adding offset */
>        tz->__tzrule[i].change = days * SECSPERDAY +
>        tz->__tzrule[i].s + tz->__tzrule[i].offset;
>
> Here tz->__tzrule[i].change is a time_t (64bit). For some reason the
> compiler does the calculation in 32 bit. All variables used in the
> calculation are 32bit.
>
> I solved this locally by changing the lines to.
>
>        /* store the change-over time in GMT form by adding offset */
>        tz->__tzrule[i].change = (time_t)days * SECSPERDAY +
>        tz->__tzrule[i].s + tz->__tzrule[i].offset;
>
> Adding the cast time_t should not cause a problem when time_t is still
> 32bit.
>
> I don't know if this is correct solution. I also only needed this
> function. So more bugs like this could be hidden away in the time code.
>
C arithmetic is defined based on the size of the operands, not the
destination, so math on 32 bit quantities is done at 32 bits (unless int
is bigger, as int is the smallest size used).

The casting of one of the operands to the bigger size is the right solution.

-- 
Richard Damon

Re: Bug in UTC -> localtime conversion after 2038

[Ivan Grokhotkov]

> On 4 Nov 2020, at 12:36, Udo de Boer <udo.de.boer@ubero.nl> wrote:
> 
> [External: This email originated outside Espressif]
> 
> Hi,
> 
> UTC time to local timezone conversion did not work for years after 2038.
> Daylight saving in the timezone was not recognized. Tested on 32bit
> xtensa architecture with GCC 8.4 (esp32) and with 64bit time_t enabled.
> 
> The struct tm is filled correct. So year, day etc are all filled with
> the correct values. But the tm.tm_isdst is 0 and no daylight saving is
> applied.
> 
> It was caused by the following calculation in time/tzcalc_limits.c at
> line 68
> 
>       /* store the change-over time in GMT form by adding offset */
>       tz->__tzrule[i].change = days * SECSPERDAY +
>       tz->__tzrule[i].s + tz->__tzrule[i].offset;
> 
> Here tz->__tzrule[i].change is a time_t (64bit). For some reason the
> compiler does the calculation in 32 bit. All variables used in the
> calculation are 32bit.
> 
> I solved this locally by changing the lines to.
> 
>       /* store the change-over time in GMT form by adding offset */
>       tz->__tzrule[i].change = (time_t)days * SECSPERDAY +
>       tz->__tzrule[i].s + tz->__tzrule[i].offset;
> 
> Adding the cast time_t should not cause a problem when time_t is still
> 32bit.
> 
> I don't know if this is correct solution. I also only needed this
> function. So more bugs like this could be hidden away in the time code.
> 
> 
> 
> 
> 
> 
> 


Thank you for reporting and proposing the fix!
Espressif will include this into the next esp32 toolchain release.

For the convenience of the maintainers, I’ve attached the patch below.


Best regards,
Ivan Grokhotkov



From 782c19cd861479cf09a4e2ee27bb7fec1872c626 Mon Sep 17 00:00:00 2001
From: Ivan Grokhotkov <ivan@espressif.com>
Date: Wed, 4 Nov 2020 16:49:51 +0100
Subject: [PATCH] newlib: libc: fix 32-bit integer overflow when calculating TZ
 rules

---
 newlib/libc/time/tzcalc_limits.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/newlib/libc/time/tzcalc_limits.c b/newlib/libc/time/tzcalc_limits.c
index e0ea6549c..00044a5d3 100644
--- a/newlib/libc/time/tzcalc_limits.c
+++ b/newlib/libc/time/tzcalc_limits.c
@@ -66,7 +66,7 @@ __tzcalc_limits (int year)
 	}
 
       /* store the change-over time in GMT form by adding offset */
-      tz->__tzrule[i].change = days * SECSPERDAY +
+      tz->__tzrule[i].change = (time_t) days * SECSPERDAY +
       tz->__tzrule[i].s + tz->__tzrule[i].offset;
     }
 
-- 
2.21.0 (Apple Git-122)

Re: Bug in UTC -> localtime conversion after 2038

[Jeff Johnston]

Thanks Ivan.  I added the patch manually as you didn't attach it, but made
you the author.  Patch applied.

-- Jeff J.

On Wed, Nov 4, 2020 at 11:21 AM Ivan Grokhotkov via Newlib <
newlib@sourceware.org> wrote:

>
>
> > On 4 Nov 2020, at 12:36, Udo de Boer <udo.de.boer@ubero.nl> wrote:
> >
> > [External: This email originated outside Espressif]
> >
> > Hi,
> >
> > UTC time to local timezone conversion did not work for years after 2038.
> > Daylight saving in the timezone was not recognized. Tested on 32bit
> > xtensa architecture with GCC 8.4 (esp32) and with 64bit time_t enabled.
> >
> > The struct tm is filled correct. So year, day etc are all filled with
> > the correct values. But the tm.tm_isdst is 0 and no daylight saving is
> > applied.
> >
> > It was caused by the following calculation in time/tzcalc_limits.c at
> > line 68
> >
> >       /* store the change-over time in GMT form by adding offset */
> >       tz->__tzrule[i].change = days * SECSPERDAY +
> >       tz->__tzrule[i].s + tz->__tzrule[i].offset;
> >
> > Here tz->__tzrule[i].change is a time_t (64bit). For some reason the
> > compiler does the calculation in 32 bit. All variables used in the
> > calculation are 32bit.
> >
> > I solved this locally by changing the lines to.
> >
> >       /* store the change-over time in GMT form by adding offset */
> >       tz->__tzrule[i].change = (time_t)days * SECSPERDAY +
> >       tz->__tzrule[i].s + tz->__tzrule[i].offset;
> >
> > Adding the cast time_t should not cause a problem when time_t is still
> > 32bit.
> >
> > I don't know if this is correct solution. I also only needed this
> > function. So more bugs like this could be hidden away in the time code.
> >
> >
> >
> >
> >
> >
> >
>
>
> Thank you for reporting and proposing the fix!
> Espressif will include this into the next esp32 toolchain release.
>
> For the convenience of the maintainers, I’ve attached the patch below.
>
>
> Best regards,
> Ivan Grokhotkov
>
>
>
> From 782c19cd861479cf09a4e2ee27bb7fec1872c626 Mon Sep 17 00:00:00 2001
> From: Ivan Grokhotkov <ivan@espressif.com>
> Date: Wed, 4 Nov 2020 16:49:51 +0100
> Subject: [PATCH] newlib: libc: fix 32-bit integer overflow when
> calculating TZ
>  rules
>
> ---
>  newlib/libc/time/tzcalc_limits.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/newlib/libc/time/tzcalc_limits.c
> b/newlib/libc/time/tzcalc_limits.c
> index e0ea6549c..00044a5d3 100644
> --- a/newlib/libc/time/tzcalc_limits.c
> +++ b/newlib/libc/time/tzcalc_limits.c
> @@ -66,7 +66,7 @@ __tzcalc_limits (int year)
>         }
>
>        /* store the change-over time in GMT form by adding offset */
> -      tz->__tzrule[i].change = days * SECSPERDAY +
> +      tz->__tzrule[i].change = (time_t) days * SECSPERDAY +
>        tz->__tzrule[i].s + tz->__tzrule[i].offset;
>      }
>
> --
> 2.21.0 (Apple Git-122)
>
>
>
>
>
>