From 090dcfd70f91030f54998cabef0dfb10d122d9fa Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 21 Aug 2024 10:55:09 +0200 Subject: [PATCH 1/6] Add tests for Utility::FormatDateTime() --- test/CMakeLists.txt | 1 + test/base-utility.cpp | 49 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 28b528463..e3772886a 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -128,6 +128,7 @@ add_boost_test(base base_utility/validateutf8 base_utility/EscapeCreateProcessArg base_utility/TruncateUsingHash + base_utility/FormatDateTime base_value/scalar base_value/convert base_value/format diff --git a/test/base-utility.cpp b/test/base-utility.cpp index 65222e1fd..77d91f47a 100644 --- a/test/base-utility.cpp +++ b/test/base-utility.cpp @@ -135,4 +135,53 @@ BOOST_AUTO_TEST_CASE(TruncateUsingHash) std::string(37, 'a') + "...86f33652fcffd7fa1443e246dd34fe5d00e25ffd"); } +BOOST_AUTO_TEST_CASE(FormatDateTime) { + using time_t_limit = std::numeric_limits; + + // Helper to repeat a given string a number of times. + auto repeat = [](const std::string& s, size_t n) { + std::ostringstream stream; + for (size_t i = 0; i < n; ++i) { + stream << s; + } + return stream.str(); + }; + + // Valid inputs. + const double ts = 1136214245.0; // 2006-01-02 15:04:05 UTC + BOOST_CHECK_EQUAL("2006-01-02 15:04:05", Utility::FormatDateTime("%F %T", ts)); + BOOST_CHECK_EQUAL("2006", Utility::FormatDateTime("%Y", ts)); + BOOST_CHECK_EQUAL("2006#2006", Utility::FormatDateTime("%Y#%Y", ts)); + BOOST_CHECK_EQUAL("%", Utility::FormatDateTime("%%", ts)); + BOOST_CHECK_EQUAL("%Y", Utility::FormatDateTime("%%Y", ts)); + BOOST_CHECK_EQUAL("", Utility::FormatDateTime("", ts)); + BOOST_CHECK_EQUAL("1970-01-01 00:00:00", Utility::FormatDateTime("%F %T", 0.0)); + BOOST_CHECK_EQUAL("2038-01-19 03:14:07", Utility::FormatDateTime("%F %T", 2147483647.0)); // 2^31 - 1 + if constexpr (sizeof(time_t) > sizeof(int32_t)) { + BOOST_CHECK_EQUAL("2100-03-14 13:37:42", Utility::FormatDateTime("%F %T", 4108714662.0)); // Past year 2038 + } else { + BOOST_WARN_MESSAGE(false, "skipping test with past 2038 input due to 32 bit time_t"); + } + + // Negative (pre-1970) timestamps. +#ifdef _MSC_VER + // localtime_s() on Windows doesn't seem to like them and always errors out. + BOOST_CHECK_THROW(Utility::FormatDateTime("%F %T", -1.0), posix_error); + BOOST_CHECK_THROW(Utility::FormatDateTime("%F %T", -2147483648.0), posix_error); // -2^31 +#else /* _MSC_VER */ + BOOST_CHECK_EQUAL("1969-12-31 23:59:59", Utility::FormatDateTime("%F %T", -1.0)); + BOOST_CHECK_EQUAL("1901-12-13 20:45:52", Utility::FormatDateTime("%F %T", -2147483648.0)); // -2^31 +#endif /* _MSC_VER */ + + // Values right at the limits of time_t. + // + // With 64 bit time_t, there may not be an exact double representation of its min/max value, std::nextafter() is + // used to move the value towards 0 so that it's within the range of doubles that can be represented as time_t. + // + // These are expected to result in an error due to the intermediate struct tm not being able to represent these + // timestamps, so localtime_r() returns EOVERFLOW which makes the implementation throw an exception. + BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", std::nextafter(time_t_limit::min(), 0)), posix_error); + BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", std::nextafter(time_t_limit::max(), 0)), posix_error); +} + BOOST_AUTO_TEST_SUITE_END() From 704acdc6985b558dd148885fc78946072e83b568 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 21 Aug 2024 11:15:34 +0200 Subject: [PATCH 2/6] Utility::FormatDateTime(): use boost::numeric_cast<>() The previous implementation actually had undefined behavior when called with a double that can't be represented as time_t. With boost::numeric_cast, there's a convenient cast available that avoids this and throws an exceptions on overflow. It's undefined behavior ([0], where the implicit conversion rule comes into play because the C-style cast uses static_cast [1] which in turn uses the imlicit conversion as per rule 5 of [2]): > A prvalue of floating-point type can be converted to a prvalue of any integer > type. The fractional part is truncated, that is, the fractional part is > discarded. > > * If the truncated value cannot fit into the destination type, the behavior > is undefined (even when the destination type is unsigned, modulo arithmetic > does not apply). Note that on Linux amd64, the undefined behavior typically manifests itself in the result being the minimal value of time_t which then results in localtime_r failing with EOVERFLOW. [0]: https://en.cppreference.com/w/cpp/language/implicit_conversion#Floating.E2.80.93integral_conversions [1]: https://en.cppreference.com/w/cpp/language/explicit_cast [2]: https://en.cppreference.com/w/cpp/language/static_cast --- lib/base/utility.cpp | 4 +++- test/base-utility.cpp | 7 +++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/base/utility.cpp b/lib/base/utility.cpp index 6ff84ae65..6f272178a 100644 --- a/lib/base/utility.cpp +++ b/lib/base/utility.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -1052,7 +1053,8 @@ String Utility::FormatDuration(double duration) String Utility::FormatDateTime(const char *format, double ts) { char timestamp[128]; - auto tempts = (time_t)ts; /* We don't handle sub-second timestamps here just yet. */ + // Sub-second precision is removed, strftime() has no format specifiers for that anyway. + auto tempts = boost::numeric_cast(ts); tm tmthen; #ifdef _MSC_VER diff --git a/test/base-utility.cpp b/test/base-utility.cpp index 77d91f47a..ced81ae4a 100644 --- a/test/base-utility.cpp +++ b/test/base-utility.cpp @@ -137,6 +137,9 @@ BOOST_AUTO_TEST_CASE(TruncateUsingHash) BOOST_AUTO_TEST_CASE(FormatDateTime) { using time_t_limit = std::numeric_limits; + using double_limit = std::numeric_limits; + using boost::numeric::negative_overflow; + using boost::numeric::positive_overflow; // Helper to repeat a given string a number of times. auto repeat = [](const std::string& s, size_t n) { @@ -182,6 +185,10 @@ BOOST_AUTO_TEST_CASE(FormatDateTime) { // timestamps, so localtime_r() returns EOVERFLOW which makes the implementation throw an exception. BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", std::nextafter(time_t_limit::min(), 0)), posix_error); BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", std::nextafter(time_t_limit::max(), 0)), posix_error); + + // Out of range timestamps. + BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", std::nextafter(time_t_limit::min(), -double_limit::infinity())), negative_overflow); + BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", std::nextafter(time_t_limit::max(), +double_limit::infinity())), positive_overflow); } BOOST_AUTO_TEST_SUITE_END() From c2c66908f61ef3df8e6bffda0d348d447356d974 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 21 Aug 2024 11:43:13 +0200 Subject: [PATCH 3/6] Utility::FormatDateTime(): use localtime_s() on Windows localtime() is not thread-safe as it returns a pointer to a shared tm struct. Everywhere except on Windows, localtime_r() is used already which avoids the problem by using a struct allocated by the caller for the output. Windows actually has a similar function called localtime_s() which has the same properties, just with a different name and order of arguments. --- lib/base/utility.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/base/utility.cpp b/lib/base/utility.cpp index 6f272178a..6985ce539 100644 --- a/lib/base/utility.cpp +++ b/lib/base/utility.cpp @@ -1058,15 +1058,12 @@ String Utility::FormatDateTime(const char *format, double ts) tm tmthen; #ifdef _MSC_VER - tm *temp = localtime(&tempts); - - if (!temp) { + errno_t err = localtime_s(&tmthen, &tempts); + if (err) { BOOST_THROW_EXCEPTION(posix_error() - << boost::errinfo_api_function("localtime") - << boost::errinfo_errno(errno)); + << boost::errinfo_api_function("localtime_s") + << boost::errinfo_errno(err)); } - - tmthen = *temp; #else /* _MSC_VER */ if (!localtime_r(&tempts, &tmthen)) { BOOST_THROW_EXCEPTION(posix_error() From 0285028689f91194f4a8392bef239520b829b8c1 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 21 Aug 2024 11:55:19 +0200 Subject: [PATCH 4/6] Utility::FormatDateTime(): handle errors from strftime() So far, the return value of strftime() was simply ignored and the output buffer passed to the icinga::String constructor. However, there are error conditions where strftime() returns 0 to signal an error, like if the buffer was too small for the output. In that case, there's no guarantee on the buffer contents and reading it can result in undefined behavior. Unfortunately, returning 0 can also indicate success and strftime() doesn't set errno, so there's no reliable way to distinguish both situations. Thus, the implementation now returns the empty string in both cases. I attempted to use std::put_time() at first as that allows for better error handling, however, there were problems with the implementation on Windows (see inline comment), so I put that plan on hold at left strftime() there for the time being. --- lib/base/utility.cpp | 29 +++++++++++++++++++++++++---- test/base-utility.cpp | 5 +++++ 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/lib/base/utility.cpp b/lib/base/utility.cpp index 6985ce539..e1bc42b8d 100644 --- a/lib/base/utility.cpp +++ b/lib/base/utility.cpp @@ -1052,7 +1052,27 @@ String Utility::FormatDuration(double duration) String Utility::FormatDateTime(const char *format, double ts) { - char timestamp[128]; + /* Known limitations of the implementation: Only works if the result is at most 127 bytes, otherwise returns an + * empty string. An empty string is also returned in all other error cases as proper error handling for strftime() + * is impossible. + * + * From strftime(3): + * + * If the output string would exceed max bytes, errno is not set. This makes it impossible to distinguish this + * error case from cases where the format string legitimately produces a zero-length output string. POSIX.1-2001 + * does not specify any errno settings for strftime(). + * + * https://manpages.debian.org/bookworm/manpages-dev/strftime.3.en.html#BUGS + * + * There's also std::put_time() from C++ which works with an ostream and does not have a fixed size output buffer + * and should allow using the error handling of the ostream. However, there seem to be an unfortunate implementation + * of this on some Windows versions where passing an invalid format string results in std::bad_alloc and the process + * allocating more and more memory before throwing the exception. In case someone in the future wants to try + * std::put_time() again: better build packages for Windows and test them across all supported versions. + * Hypothesis: it's implemented using a fixed output buffer and retrying with a larger buffer on error, assuming + * the error was due to the buffer being too small. + */ + // Sub-second precision is removed, strftime() has no format specifiers for that anyway. auto tempts = boost::numeric_cast(ts); tm tmthen; @@ -1072,9 +1092,10 @@ String Utility::FormatDateTime(const char *format, double ts) } #endif /* _MSC_VER */ - strftime(timestamp, sizeof(timestamp), format, &tmthen); - - return timestamp; + char buf[128]; + size_t n = strftime(buf, sizeof(buf), format, &tmthen); + // On error, n == 0 and an empty string is returned. + return std::string(buf, n); } String Utility::FormatErrorNumber(int code) { diff --git a/test/base-utility.cpp b/test/base-utility.cpp index ced81ae4a..5c0d358cd 100644 --- a/test/base-utility.cpp +++ b/test/base-utility.cpp @@ -186,6 +186,11 @@ BOOST_AUTO_TEST_CASE(FormatDateTime) { BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", std::nextafter(time_t_limit::min(), 0)), posix_error); BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", std::nextafter(time_t_limit::max(), 0)), posix_error); + // Excessive format strings can result in something too large for the buffer, errors out to the empty string. + // Note: both returning the proper result or throwing an exception would be fine too, unfortunately, that's + // not really possible due to limitations in strftime() error handling, see comment in the implementation. + BOOST_CHECK_EQUAL("", Utility::FormatDateTime(repeat("%Y", 1000).c_str(), ts)); + // Out of range timestamps. BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", std::nextafter(time_t_limit::min(), -double_limit::infinity())), negative_overflow); BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", std::nextafter(time_t_limit::max(), +double_limit::infinity())), positive_overflow); From d5b3ffaa6dee8cfacbc1b03881f04bbcc943c6d7 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 21 Aug 2024 12:08:04 +0200 Subject: [PATCH 5/6] Utility::FormatDateTime(): handle invalid format strings on Windows On Windows, the strftime() function family invokes an invalid parameter handler when the format string is invalid (see the "Remarks" section in their documentation). std::put_time() shows the same behavior as it uses _wcsftime_l() internally. The default invalid parameter handler may terminate the process, which can be a problem given that the format string can be specified by the user from the Icinga DSL. Thus, temporarily set a thread-local no-op handler to disable the default one allowing the program to continue. This then simply results in the function returning an error which then results in an exception as we ask the stream to throw one. See also: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strftime-wcsftime-strftime-l-wcsftime-l?view=msvc-170 https://learn.microsoft.com/en-us/cpp/c-runtime-library/parameter-validation?view=msvc-170 https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/set-invalid-parameter-handler-set-thread-local-invalid-parameter-handler?view=msvc-170 --- lib/base/utility.cpp | 27 +++++++++++++++++++++++++++ test/base-utility.cpp | 13 +++++++++++++ 2 files changed, 40 insertions(+) diff --git a/lib/base/utility.cpp b/lib/base/utility.cpp index e1bc42b8d..75b905fe1 100644 --- a/lib/base/utility.cpp +++ b/lib/base/utility.cpp @@ -4,6 +4,7 @@ #include "base/utility.hpp" #include "base/convert.hpp" #include "base/application.hpp" +#include "base/defer.hpp" #include "base/logger.hpp" #include "base/exception.hpp" #include "base/socket.hpp" @@ -1092,6 +1093,32 @@ String Utility::FormatDateTime(const char *format, double ts) } #endif /* _MSC_VER */ +#ifdef _MSC_VER + /* On Windows, the strftime() function family invokes an invalid parameter handler when the format string is + * invalid (see the "Remarks" section in their documentation). std::put_time() shows the same behavior as it + * uses _wcsftime_l() internally. The default invalid parameter handler may terminate the process, which can + * be a problem given that the format string can be specified by the user from the Icinga DSL. + * + * Thus, temporarily set a thread-local no-op handler to disable the default one allowing the program to + * continue. This then simply results in the function returning an error which then results in an exception as + * we ask the stream to throw one. + * + * See also: + * https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strftime-wcsftime-strftime-l-wcsftime-l?view=msvc-170 + * https://learn.microsoft.com/en-us/cpp/c-runtime-library/parameter-validation?view=msvc-170 + * https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/set-invalid-parameter-handler-set-thread-local-invalid-parameter-handler?view=msvc-170 + */ + + auto oldHandler = _set_thread_local_invalid_parameter_handler( + [](const wchar_t*, const wchar_t*, const wchar_t*, unsigned int, uintptr_t) { + // Intentionally do nothing to continue executing. + }); + + Defer resetHandler([oldHandler]() { + _set_thread_local_invalid_parameter_handler(oldHandler); + }); +#endif /* _MSC_VER */ + char buf[128]; size_t n = strftime(buf, sizeof(buf), format, &tmthen); // On error, n == 0 and an empty string is returned. diff --git a/test/base-utility.cpp b/test/base-utility.cpp index 5c0d358cd..8fe1814da 100644 --- a/test/base-utility.cpp +++ b/test/base-utility.cpp @@ -191,6 +191,19 @@ BOOST_AUTO_TEST_CASE(FormatDateTime) { // not really possible due to limitations in strftime() error handling, see comment in the implementation. BOOST_CHECK_EQUAL("", Utility::FormatDateTime(repeat("%Y", 1000).c_str(), ts)); + // Invalid format strings. + for (const char* format : {"%", "x % y", "x %! y"}) { + std::string result = Utility::FormatDateTime(format, ts); + + // Implementations of strftime() seem to either keep invalid format specifiers and return them in the output, or + // treat them as an error which our implementation currently maps to the empty string due to strftime() not + // properly reporting errors. If this limitation of our implementation is lifted, other behavior like throwing + // an exception would also be valid. + BOOST_CHECK_MESSAGE(result.empty() || result == format, + "FormatDateTime(" << std::quoted(format) << ", " << ts << ") = " << std::quoted(result) << + " should be one of [\"\", " << std::quoted(format) << "]"); + } + // Out of range timestamps. BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", std::nextafter(time_t_limit::min(), -double_limit::infinity())), negative_overflow); BOOST_CHECK_THROW(Utility::FormatDateTime("%Y", std::nextafter(time_t_limit::max(), +double_limit::infinity())), positive_overflow); From 39ae2e8ca4eca6be1c73a70bdd0c45258a527613 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 21 Aug 2024 12:31:44 +0200 Subject: [PATCH 6/6] Utility::FormatDateTime(): provide an overload for tm* This allows the function to be used both with a double timestamp or a pointer to a tm struct. With this, a similar implementation inside the tests can simply use our regular function. --- lib/base/utility.cpp | 46 +++++++++++++++++--------------- lib/base/utility.hpp | 3 ++- test/icinga-legacytimeperiod.cpp | 11 +------- 3 files changed, 28 insertions(+), 32 deletions(-) diff --git a/lib/base/utility.cpp b/lib/base/utility.cpp index 75b905fe1..338e4f77f 100644 --- a/lib/base/utility.cpp +++ b/lib/base/utility.cpp @@ -1051,8 +1051,31 @@ String Utility::FormatDuration(double duration) return NaturalJoin(tokens); } -String Utility::FormatDateTime(const char *format, double ts) +String Utility::FormatDateTime(const char* format, double ts) { + // Sub-second precision is removed, strftime() has no format specifiers for that anyway. + auto tempts = boost::numeric_cast(ts); + tm tmthen; + +#ifdef _MSC_VER + errno_t err = localtime_s(&tmthen, &tempts); + if (err) { + BOOST_THROW_EXCEPTION(posix_error() + << boost::errinfo_api_function("localtime_s") + << boost::errinfo_errno(err)); + } +#else /* _MSC_VER */ + if (!localtime_r(&tempts, &tmthen)) { + BOOST_THROW_EXCEPTION(posix_error() + << boost::errinfo_api_function("localtime_r") + << boost::errinfo_errno(errno)); + } +#endif /* _MSC_VER */ + + return FormatDateTime(format, &tmthen); +} + +String Utility::FormatDateTime(const char* format, const tm* t) { /* Known limitations of the implementation: Only works if the result is at most 127 bytes, otherwise returns an * empty string. An empty string is also returned in all other error cases as proper error handling for strftime() * is impossible. @@ -1074,25 +1097,6 @@ String Utility::FormatDateTime(const char *format, double ts) * the error was due to the buffer being too small. */ - // Sub-second precision is removed, strftime() has no format specifiers for that anyway. - auto tempts = boost::numeric_cast(ts); - tm tmthen; - -#ifdef _MSC_VER - errno_t err = localtime_s(&tmthen, &tempts); - if (err) { - BOOST_THROW_EXCEPTION(posix_error() - << boost::errinfo_api_function("localtime_s") - << boost::errinfo_errno(err)); - } -#else /* _MSC_VER */ - if (!localtime_r(&tempts, &tmthen)) { - BOOST_THROW_EXCEPTION(posix_error() - << boost::errinfo_api_function("localtime_r") - << boost::errinfo_errno(errno)); - } -#endif /* _MSC_VER */ - #ifdef _MSC_VER /* On Windows, the strftime() function family invokes an invalid parameter handler when the format string is * invalid (see the "Remarks" section in their documentation). std::put_time() shows the same behavior as it @@ -1120,7 +1124,7 @@ String Utility::FormatDateTime(const char *format, double ts) #endif /* _MSC_VER */ char buf[128]; - size_t n = strftime(buf, sizeof(buf), format, &tmthen); + size_t n = strftime(buf, sizeof(buf), format, t); // On error, n == 0 and an empty string is returned. return std::string(buf, n); } diff --git a/lib/base/utility.hpp b/lib/base/utility.hpp index 47b68d251..5132673ca 100644 --- a/lib/base/utility.hpp +++ b/lib/base/utility.hpp @@ -77,7 +77,8 @@ public: static String Join(const Array::Ptr& tokens, char separator, bool escapeSeparator = true); static String FormatDuration(double duration); - static String FormatDateTime(const char *format, double ts); + static String FormatDateTime(const char* format, double ts); + static String FormatDateTime(const char* format, const tm* t); static String FormatErrorNumber(int code); #ifndef _WIN32 diff --git a/test/icinga-legacytimeperiod.cpp b/test/icinga-legacytimeperiod.cpp index 900bb6578..8661b75f7 100644 --- a/test/icinga-legacytimeperiod.cpp +++ b/test/icinga-legacytimeperiod.cpp @@ -527,16 +527,7 @@ struct Segment std::string pretty_time(const tm& t) { -#if defined(__GNUC__) && __GNUC__ < 5 - // GCC did not implement std::put_time() until version 5 - char buf[128]; - size_t n = strftime(buf, sizeof(buf), "%Y-%m-%d %H:%M:%S %Z", &t); - return std::string(buf, n); -#else /* defined(__GNUC__) && __GNUC__ < 5 */ - std::ostringstream stream; - stream << std::put_time(&t, "%Y-%m-%d %H:%M:%S %Z"); - return stream.str(); -#endif /* defined(__GNUC__) && __GNUC__ < 5 */ + return Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %Z", &t); } std::string pretty_time(time_t t)