Revert change to split_seconds() from #354 - #376
Conversation
In split_seconds(), decrementing `sec` before evaluating `tp - sec` causes `sec` to fall outside the representable range of `time_point<D>` when `tp` is near `time_point<D>::min()`. For example, with nanosecond resolution, `time_point<D>::min()` is approximately -9,223,372,036.85 seconds. Truncating towards zero with `time_point_cast<seconds>(tp)` yields -9,223,372,036 seconds. Decrementing it to floor the value yields -9,223,372,037 seconds. Evaluating `tp - sec` converts `sec` back to `time_point<D>`, computing: -9,223,372,037 * 1,000,000,000 = -9,223,372,037,000,000,000 ns which overflows int64_t::min() (-9,223,372,036,854,775,808 ns) and triggers a UBSan signed-integer-overflow failure in cctz::format(). Note that even in C++17, calling `std::chrono::floor<seconds>(tp)` and then evaluating `tp - floor` causes this exact same overflow because the floored second cannot be converted to `time_point<D>`. Fix this by computing `sub = tp - sec` against the truncated `sec` (which is guaranteed to be in range because it is closer to zero than `tp`), and then adjusting `sec` and `sub` independently if `sub.count() < 0`.
| auto sub = tp - sec; | ||
| if (sub.count() < 0) { |
There was a problem hiding this comment.
Fix this by computing
sub = tp - secagainst the truncatedsec(which is guaranteed to be in range because it is closer to zero thantp), and then adjustingsecandsubindependently ifsub.count() < 0.
OK, thanks. I should not so easily second guess how I'd crafted things before. Sigh.
That said, I think we should still change the conditional to sub < D::zero() so as to stay in the chrono domain.
Aside: I (think I) had run all the CCTZ tests with overflow detection on and not hit this. Does that mean you only noticed it downstream, and that perhaps we need an additional test expectation here?
There was a problem hiding this comment.
I keep forgetting about sub < D::zero(). Done.
The problem is reproducible with the tests we have. This change includes a change to CI that triggers the failure.
Locally the command I use is USE_BAZEL_VERSION=8.6.0 bazelisk test --repo_env=CC=clang --copt=-fsanitize=undefined --linkopt=-fsanitize=undefined --copt=-fno-sanitize-recover=undefined :time_zone_format_test
The gotcha is -fno-sanitize-recover=undefined. By default UBSAN only logs errors. The process still returns 0. You need -fno-sanitize-recover=[checks] to abort the process.
There was a problem hiding this comment.
The gotcha is
-fno-sanitize-recover=undefined.
Indeed, that is what I was missing. Thanks.
| --build_tag_filters=-fuzztest | ||
| --copt=-DGTEST_REMOVE_LEGACY_TEST_CASEAPI_=1 | ||
| --copt=-fsanitize=address,undefined | ||
| --copt=-fno-sanitize-recover=undefined |
There was a problem hiding this comment.
PS: Is all a better choice here?
| if (sec > tp) sec -= seconds{1}; // TODO(C++17): use std::chrono::floor | ||
| return {sec, std::chrono::duration_cast<D>(tp - sec)}; | ||
| auto sub = tp - sec; | ||
| if (sub < D::zero()) { |
There was a problem hiding this comment.
Can you add a comment that explains this case (negative subseconds) with an example?
| return {sec, std::chrono::duration_cast<D>(tp - sec)}; | ||
| auto sub = tp - sec; | ||
| // time_point_cast truncates towards zero, so for negative tp with fractional | ||
| // seconds (e.g., -1.5s), sec is truncated to -1s and sub is negative (-0.5s). |
There was a problem hiding this comment.
It looks like the "to -1s" should be parenthetical.
In split_seconds(), decrementing
secbefore evaluatingtp - seccausessecto fall outside the representable range oftime_point<D>whentpis neartime_point<D>::min().For example, with nanosecond resolution,
time_point<D>::min()is approximately -9,223,372,036.85 seconds. Truncating towards zero withtime_point_cast<seconds>(tp)yields -9,223,372,036 seconds. Decrementing it to floor the value yields -9,223,372,037 seconds. Evaluatingtp - secconvertssecback totime_point<D>, computing: -9,223,372,037 * 1,000,000,000 = -9,223,372,037,000,000,000 ns which overflows int64_t::min() (-9,223,372,036,854,775,808 ns) and triggers a UBSan signed-integer-overflow failure in cctz::format().Note that even in C++17, calling
std::chrono::floor<seconds>(tp)and then evaluatingtp - floorcauses this exact same overflow because the floored second cannot be converted totime_point<D>.Fix this by computing
sub = tp - secagainst the truncatedsec(which is guaranteed to be in range because it is closer to zero thantp), and then adjustingsecandsubindependently ifsub.count() < 0.Also enable UBSAN in one of the builds to catch this.