F2I-Consulting / fesapi

DevKit for ENERGISTICS™ data standards (mainly RESQML™), multi-languages (C++, Java, C#, Python)
Apache License 2.0
32 stars 24 forks source link

32-bit overflow for large times #336

Closed cgodkin closed 6 months ago

cgodkin commented 6 months ago

What are the steps to reproduce this issue?

We found an overflow in TimeTools.cpp timegm (and to_sys_time) for very large times.

What does happen?

Exception thrown by library: The timestamp has not been found in the allowed range.

from

AbstractProperty::getSingleTimestamp()

What were you expecting to happen?

Legal time_t (since it's 64-bit) should work fine.

Any logs, error output, etc?

(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)

Any other comments?

User is doing a very long simulation with time steps to the year 6996!

What versions of fesapi are you using?

2.8.0.0

Suggested fix

Change last two functions of TimeTools.cpp to something like this:

date::sys_seconds
to_sys_time(std::tm const& t)
{
        using namespace date;
        using namespace std::chrono;
        date::sys_seconds tmp = date::sys_days{ date::year{t.tm_year + 1900} / (t.tm_mon + 1) / t.tm_mday };
        tmp += hours{ t.tm_hour } +minutes{ t.tm_min } +seconds{ t.tm_sec };
        return tmp;
}

time_t timeTools::timegm(std::tm const& t)
{
        date::sys_seconds tmp = to_sys_time (t);
        return std::chrono::system_clock::to_time_t(tmp);
}

Note that to_sys_time() previously built tmp in one step. But the large year caused an overflow in these 32-bit ints when it was part of the intermediate value. Now make tmp in two steps, saving first part into 64-bit value and then adding seconds later.

Change timegm similarly or perhaps change it to useto_sys_time`

We have patched locally and this works for us.

cgodkin commented 6 months ago

More explanation: 6996-09-05 as time_t is 158626684800 but if you divide that by 60 to get minutes since the epoch, it's 2643778080, which is not too far past 2^31. So as a 32-bit quantity, the overflow forces it negative but doesn't yet (if it were unsigned) require more than 32 bits. This overflow for minutes since the epoch first happens in January 6053.

philippeVerney commented 6 months ago

Thanks Carl for this detailed issue which indeed looks to have to be fixed!