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

Two problems with time conversion on Linux #338

Closed cgodkin closed 6 months ago

cgodkin commented 6 months ago

What are the steps to reproduce this issue?

We have a data set with a very large time span - 5000 years. The times far in the future are not preserved as they are converted to and from time_t on Linux with GCC 10.4 (at least).

What does happen?

There are overflow problems with both from_time_t() and to_time_t() on Linux.

What were you expecting to happen?

Successful round trip with time_t and std::tm.

Any other comments?

We have patched locally and everything is working.

What versions of fesapi are you using?

2.8.

Suggested changes

  1. The main problem on Linux is that this call will overflow:
    td::chrono::system_clock::from_time_t(t)

    when t is a timepoint after 2262, because the precision of Linux's system_clock is nanoseconds, and that is the point at which the number of nanoseconds since the epoch reaches 2^63.

So we suggest that you replace all usages of from_time_t like this:

std::tm tmConversion = timeTools::to_calendar_time(std::chrono::system_clock::from_time_t(timestamp));

with

std::tm tmConversion = timeTools::to_calendar_time(date::sys_seconds(std::chrono::seconds(timestamp)));

(Note that date::sys_seconds is used here since it appears you're building for C++17; std::chrono::sys_seconds would work for C++20.)

Occurrences:

src/eml2/TimeSeries.cpp:30
src/common/AbstractObject.cpp:335
src/common/AbstractObject.cpp:422
src/tools/TimeTools.cpp:50
src/resqml2/AbstractProperty.cpp:161
src/prodml2_2/TimeSeriesData.cpp:80
src/prodml2_2/TimeSeriesData.cpp:104
src/witsml2_1/WellboreCompletion.cpp:277
src/witsml2_1/WellboreCompletion.cpp:293
src/witsml2_1/WellboreCompletion.cpp:652
src/witsml2_1/WellboreCompletion.cpp:672
src/witsml2_1/WellboreCompletion.cpp:728
src/witsml2_1/WellboreCompletion.cpp:748
src/MacroDefinitions.h:524
src/MacroDefinitions.h:541
src/MacroDefinitions.h:556
  1. After we made those changes, a debug test program still produced the wrong results on our roundtrip tests.

The function system_clock::to_time_t() is not a template, but instead it takes a system_clock::time_point as its argument. Recall that this has precision of nanoseconds. So: passing a sys_seconds object into to_time_t immediately upcasts it to nanoseconds (which then overflows if it's a timepoint after Y2262). Then the implementation of to_time_t immediately downcasts it back to seconds -- but by this time, if there has been an overflow, the downcast now gives the wrong result.

So the fix for this one is, don't use to_time_t either. Instead of timeTools::timegm() returning to_time_t(tmp), we suggest that you return tmp.time_since_epoch().count() -- which we know will give the right thing since its precision is seconds and its epoch is the Posix epoch.

Why does it work optimized? Well, when building optimized GCC assumes that undefined behavior cannot be reached, so it assumes no overflow can ever happen, so it just treats the upcast to nanoseconds followed by downcast back to seconds as a no-op.

There are two occurrences of to_time_t() in FESAPI. The important one is in tools/TimeTools.cpp which must be changed.

The occurence in common/AbstractObject.cpp deals with "now" so it will never be 250 years in the future!

philippeVerney commented 6 months ago

Thanks a lot @cgodkin This is an amazing report with a perfect proposal for solution. Really happy to read it and learn from it. And I am more than happy to easily fix it thanks to your advices. Thanks also to your team who helped to achieve such a nice analysis!