-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix overflow in time_point formatting with large dates #3727
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
include/fmt/chrono.h
Outdated
template <typename Duration, | ||
FMT_ENABLE_IF(std::is_integral<typename Duration::rep>::value)> | ||
std::time_t to_time_t( | ||
std::chrono::time_point<std::chrono::system_clock, Duration> time_point) { | ||
#if FMT_SAFE_DURATION_CAST | ||
return fmt_safe_duration_cast<std::chrono::duration<std::time_t>>( | ||
time_point.time_since_epoch()) | ||
.count(); | ||
#else | ||
return std::chrono::duration_cast<std::chrono::duration<std::time_t>>( | ||
time_point.time_since_epoch()) | ||
.count(); | ||
#endif | ||
} | ||
|
||
template <typename Duration, | ||
FMT_ENABLE_IF(std::is_floating_point<typename Duration::rep>::value)> | ||
std::time_t to_time_t( | ||
std::chrono::time_point<std::chrono::system_clock, Duration> time_point) { | ||
return std::chrono::duration_cast<std::chrono::duration<std::time_t>>( | ||
time_point.time_since_epoch()) | ||
.count(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that the two overloads of to_time_t
are identical modulo fmt_safe_duration_cast
I suggest merging them into one and moving the difference into fmt_safe_duration_cast
which can just forward to duration_cast
for floating point and when FMT_SAFE_DURATION_CAST
is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? I feel like fmt_safe_duration_cast
should always offer the same safety guarantees, there being "safe" in the name. If we did this, perhaps this should be renamed fmt_duration_cast
instead.
Regardless, I'd still need two overloads of that, since I can't use if constexpr
to select a different path for floats. Would that be OK with you?
It also leaves open the actual issue, being that the safe casts don't seem to be available for mixed float<->integer casts. I don't immediately see a reason why, other than it just being more work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overflow in float to int conversions are also undefined behaviour, ideally we should be able to catch these in safe mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming to fmt_duration_cast
sounds reasonable. We should handle the "safe" mode in it rather than having the callers deal with it. I understand that there will be two overloads, but the overall result should be simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and pushed
The function is now more generic and will handle all casts. It also takes care of toggling safe vs unsafe casts using FMT_SAFE_DURATION_CAST.
include/fmt/chrono.h
Outdated
@@ -430,6 +430,45 @@ auto write(OutputIt out, const std::tm& time, const std::locale& loc, | |||
return write_encoded_tm_str(out, string_view(buf.data(), buf.size()), loc); | |||
} | |||
|
|||
#if FMT_SAFE_DURATION_CAST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be moved into fmt_duration_cast
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, if I understood what you meant correctly!
include/fmt/chrono.h
Outdated
FMT_ENABLE_IF((std::is_integral<FromRep>::value && | ||
std::is_integral<typename To::rep>::value) || | ||
(std::is_floating_point<FromRep>::value && | ||
std::is_floating_point<typename To::rep>::value))> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a trait for this and reuse here and below (negated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I have pushed the requested changes and hopefully fixed the Windows CI. Let me know if you'd like me to rebase this into a single commit. |
Thank you! |
* Fix fmtlib#3725 and rename fmt_safe_duration_cast to fmt_duration_cast The function is now more generic and will handle all casts. It also takes care of toggling safe vs unsafe casts using FMT_SAFE_DURATION_CAST. * Refactor fmt_duration_cast to put #ifdef inside the function * Fix compilation error with FMT_USE_LOCAL_TIME
This fixes #3725. I am not 100% sure the new implementation is what people would want; in particular, with
FMT_SAFE_DURATION_CAST
enabled, trying to format a date that is too large to fit intime_t
will throw an exception. WithoutFMT_SAFE_DURATION_CAST
I believe this will just invoke an undefined behavior (overflow oftime_t
, which is likely a signed integer type).