From dda4620997fa6cd7ced0291ee9236fa6b04e63a3 Mon Sep 17 00:00:00 2001 From: Shawn Zhong Date: Fri, 13 Jan 2023 02:43:51 -0600 Subject: [PATCH 1/6] Implement glibc ext for sec, min, and hour --- include/fmt/chrono.h | 139 ++++++++++++++++++++++++++++++------------- test/chrono-test.cc | 31 ++++++++++ 2 files changed, 130 insertions(+), 40 deletions(-) diff --git a/include/fmt/chrono.h b/include/fmt/chrono.h index b48b0d98feb8..041aa926ed0a 100644 --- a/include/fmt/chrono.h +++ b/include/fmt/chrono.h @@ -664,6 +664,13 @@ enum class numeric_system { alternative }; +enum class pad_type { + unspecified, + none, + zero, + space, +}; + // Parses a put_time-like format string and invokes handler actions. template FMT_CONSTEXPR const Char* parse_chrono_format(const Char* begin, @@ -672,6 +679,7 @@ FMT_CONSTEXPR const Char* parse_chrono_format(const Char* begin, if (begin == end || *begin == '}') return begin; if (*begin != '%') FMT_THROW(format_error("invalid format")); auto ptr = begin; + pad_type pad = pad_type::unspecified; while (ptr != end) { auto c = *ptr; if (c == '}') break; @@ -680,7 +688,21 @@ FMT_CONSTEXPR const Char* parse_chrono_format(const Char* begin, continue; } if (begin != ptr) handler.on_text(begin, ptr); - ++ptr; // consume '%' + c = *++ptr; // consume '%' + switch (c) { + case '_': + pad = pad_type::space; + ++ptr; + break; + case '-': + pad = pad_type::none; + ++ptr; + break; + case '0': + pad = pad_type::zero; + ++ptr; + break; + } if (ptr == end) FMT_THROW(format_error("invalid format")); c = *ptr++; switch (c) { @@ -758,16 +780,16 @@ FMT_CONSTEXPR const Char* parse_chrono_format(const Char* begin, break; // Hour, minute, second: case 'H': - handler.on_24_hour(numeric_system::standard); + handler.on_24_hour(numeric_system::standard, pad); break; case 'I': - handler.on_12_hour(numeric_system::standard); + handler.on_12_hour(numeric_system::standard, pad); break; case 'M': - handler.on_minute(numeric_system::standard); + handler.on_minute(numeric_system::standard, pad); break; case 'S': - handler.on_second(numeric_system::standard); + handler.on_second(numeric_system::standard, pad); break; // Other: case 'c': @@ -872,16 +894,16 @@ FMT_CONSTEXPR const Char* parse_chrono_format(const Char* begin, handler.on_dec1_weekday(numeric_system::alternative); break; case 'H': - handler.on_24_hour(numeric_system::alternative); + handler.on_24_hour(numeric_system::alternative, pad); break; case 'I': - handler.on_12_hour(numeric_system::alternative); + handler.on_12_hour(numeric_system::alternative, pad); break; case 'M': - handler.on_minute(numeric_system::alternative); + handler.on_minute(numeric_system::alternative, pad); break; case 'S': - handler.on_second(numeric_system::alternative); + handler.on_second(numeric_system::alternative, pad); break; case 'z': handler.on_utc_offset(numeric_system::alternative); @@ -965,10 +987,10 @@ struct tm_format_checker : null_chrono_spec_handler { FMT_CONSTEXPR void on_day_of_year() {} FMT_CONSTEXPR void on_day_of_month(numeric_system) {} FMT_CONSTEXPR void on_day_of_month_space(numeric_system) {} - FMT_CONSTEXPR void on_24_hour(numeric_system) {} - FMT_CONSTEXPR void on_12_hour(numeric_system) {} - FMT_CONSTEXPR void on_minute(numeric_system) {} - FMT_CONSTEXPR void on_second(numeric_system) {} + FMT_CONSTEXPR void on_24_hour(numeric_system, pad_type) {} + FMT_CONSTEXPR void on_12_hour(numeric_system, pad_type) {} + FMT_CONSTEXPR void on_minute(numeric_system, pad_type) {} + FMT_CONSTEXPR void on_second(numeric_system, pad_type) {} FMT_CONSTEXPR void on_datetime(numeric_system) {} FMT_CONSTEXPR void on_loc_date(numeric_system) {} FMT_CONSTEXPR void on_loc_time(numeric_system) {} @@ -1238,6 +1260,29 @@ class tm_writer { *out_++ = *d++; *out_++ = *d; } + void write2(int value, pad_type pad) { + switch (pad) { + case pad_type::unspecified: + case pad_type::zero: + write2(value); + break; + case pad_type::space: + if (value < 10) { + *out_++ = ' '; + write1(value); + } else { + write2(value); + } + break; + case pad_type::none: + if (value < 10) { + write1(value); + } else { + write2(value); + } + break; + } + } void write_year_extended(long long year) { // At least 4 characters. @@ -1514,23 +1559,25 @@ class tm_writer { } } - void on_24_hour(numeric_system ns) { - if (is_classic_ || ns == numeric_system::standard) return write2(tm_hour()); + void on_24_hour(numeric_system ns, pad_type pad) { + if (is_classic_ || ns == numeric_system::standard) + return write2(tm_hour(), pad); format_localized('H', 'O'); } - void on_12_hour(numeric_system ns) { + void on_12_hour(numeric_system ns, pad_type pad) { if (is_classic_ || ns == numeric_system::standard) - return write2(tm_hour12()); + return write2(tm_hour12(), pad); format_localized('I', 'O'); } - void on_minute(numeric_system ns) { - if (is_classic_ || ns == numeric_system::standard) return write2(tm_min()); + void on_minute(numeric_system ns, pad_type pad) { + if (is_classic_ || ns == numeric_system::standard) + return write2(tm_min(), pad); format_localized('M', 'O'); } - void on_second(numeric_system ns) { + void on_second(numeric_system ns, pad_type pad) { if (is_classic_ || ns == numeric_system::standard) { - write2(tm_sec()); + write2(tm_sec(), pad); if (subsecs_) { if (std::is_floating_point::value) { auto buf = memory_buffer(); @@ -1594,10 +1641,10 @@ struct chrono_format_checker : null_chrono_spec_handler { template FMT_CONSTEXPR void on_text(const Char*, const Char*) {} - FMT_CONSTEXPR void on_24_hour(numeric_system) {} - FMT_CONSTEXPR void on_12_hour(numeric_system) {} - FMT_CONSTEXPR void on_minute(numeric_system) {} - FMT_CONSTEXPR void on_second(numeric_system) {} + FMT_CONSTEXPR void on_24_hour(numeric_system, pad_type) {} + FMT_CONSTEXPR void on_12_hour(numeric_system, pad_type) {} + FMT_CONSTEXPR void on_minute(numeric_system, pad_type) {} + FMT_CONSTEXPR void on_second(numeric_system, pad_type) {} FMT_CONSTEXPR void on_12_hour_time() {} FMT_CONSTEXPR void on_24_hour_time() {} FMT_CONSTEXPR void on_iso_time() {} @@ -1819,13 +1866,25 @@ struct chrono_formatter { } } - void write(Rep value, int width) { + void write(Rep value, int width, pad_type pad = pad_type::unspecified) { write_sign(); if (isnan(value)) return write_nan(); uint32_or_64_or_128_t n = to_unsigned(to_nonnegative_int(value, max_value())); int num_digits = detail::count_digits(n); - if (width > num_digits) out = std::fill_n(out, width - num_digits, '0'); + if (width > num_digits) { + switch (pad) { + case pad_type::zero: + case pad_type::unspecified: + out = std::fill_n(out, width - num_digits, '0'); + break; + case pad_type::space: + out = std::fill_n(out, width - num_digits, ' '); + break; + case pad_type::none: + break; + } + } out = format_decimal(out, n, num_digits).end; } @@ -1874,34 +1933,34 @@ struct chrono_formatter { void on_day_of_month(numeric_system) {} void on_day_of_month_space(numeric_system) {} - void on_24_hour(numeric_system ns) { + void on_24_hour(numeric_system ns, pad_type pad) { if (handle_nan_inf()) return; - if (ns == numeric_system::standard) return write(hour(), 2); + if (ns == numeric_system::standard) return write(hour(), 2, pad); auto time = tm(); time.tm_hour = to_nonnegative_int(hour(), 24); - format_tm(time, &tm_writer_type::on_24_hour, ns); + format_tm(time, &tm_writer_type::on_24_hour, ns, pad); } - void on_12_hour(numeric_system ns) { + void on_12_hour(numeric_system ns, pad_type pad) { if (handle_nan_inf()) return; - if (ns == numeric_system::standard) return write(hour12(), 2); + if (ns == numeric_system::standard) return write(hour12(), 2, pad); auto time = tm(); time.tm_hour = to_nonnegative_int(hour12(), 12); - format_tm(time, &tm_writer_type::on_12_hour, ns); + format_tm(time, &tm_writer_type::on_12_hour, ns, pad); } - void on_minute(numeric_system ns) { + void on_minute(numeric_system ns, pad_type pad) { if (handle_nan_inf()) return; - if (ns == numeric_system::standard) return write(minute(), 2); + if (ns == numeric_system::standard) return write(minute(), 2, pad); auto time = tm(); time.tm_min = to_nonnegative_int(minute(), 60); - format_tm(time, &tm_writer_type::on_minute, ns); + format_tm(time, &tm_writer_type::on_minute, ns, pad); } - void on_second(numeric_system ns) { + void on_second(numeric_system ns, pad_type pad) { if (handle_nan_inf()) return; if (ns == numeric_system::standard) { @@ -1913,7 +1972,7 @@ struct chrono_formatter { if (buf.size() < 2 || buf[1] == '.') *out++ = '0'; out = std::copy(buf.begin(), buf.end(), out); } else { - write(second(), 2); + write(second(), 2, pad); write_fractional_seconds( out, std::chrono::duration(val), precision); } @@ -1921,7 +1980,7 @@ struct chrono_formatter { } auto time = tm(); time.tm_sec = to_nonnegative_int(second(), 60); - format_tm(time, &tm_writer_type::on_second, ns); + format_tm(time, &tm_writer_type::on_second, ns, pad); } void on_12_hour_time() { @@ -1945,7 +2004,7 @@ struct chrono_formatter { on_24_hour_time(); *out++ = ':'; if (handle_nan_inf()) return; - on_second(numeric_system::standard); + on_second(numeric_system::standard, pad_type::unspecified); } void on_am_pm() { diff --git a/test/chrono-test.cc b/test/chrono-test.cc index 2d20013f43ba..839c1726ef48 100644 --- a/test/chrono-test.cc +++ b/test/chrono-test.cc @@ -919,3 +919,34 @@ TEST(chrono_test, timestamps_sub_seconds) { EXPECT_EQ("00.250", fmt::format("{:%S}", epoch + d)); } } + +TEST(chrono_test, glibc_extensions) { + { + const auto d = std::chrono::hours(1) + std::chrono::minutes(2) + + std::chrono::seconds(3); + + EXPECT_EQ(fmt::format("{:%I,%H,%M,%S}", d), "01,01,02,03"); + EXPECT_EQ(fmt::format("{:%0I,%0H,%0M,%0S}", d), "01,01,02,03"); + EXPECT_EQ(fmt::format("{:%_I,%_H,%_M,%_S}", d), " 1, 1, 2, 3"); + EXPECT_EQ(fmt::format("{:%-I,%-H,%-M,%-S}", d), "1,1,2,3"); + + EXPECT_EQ(fmt::format("{:%OI,%OH,%OM,%OS}", d), "01,01,02,03"); + EXPECT_EQ(fmt::format("{:%0OI,%0OH,%0OM,%0OS}", d), "01,01,02,03"); + EXPECT_EQ(fmt::format("{:%_OI,%_OH,%_OM,%_OS}", d), " 1, 1, 2, 3"); + EXPECT_EQ(fmt::format("{:%-OI,%-OH,%-OM,%-OS}", d), "1,1,2,3"); + } + + { + const auto tm = make_tm(1970, 1, 1, 1, 2, 3); + EXPECT_EQ(fmt::format("{:%I,%H,%M,%S}", tm), "01,01,02,03"); + EXPECT_EQ(fmt::format("{:%0I,%0H,%0M,%0S}", tm), "01,01,02,03"); + EXPECT_EQ(fmt::format("{:%_I,%_H,%_M,%_S}", tm), " 1, 1, 2, 3"); + EXPECT_EQ(fmt::format("{:%-I,%-H,%-M,%-S}", tm), "1,1,2,3"); + + EXPECT_EQ(fmt::format("{:%OI,%OH,%OM,%OS}", tm), "01,01,02,03"); + EXPECT_EQ(fmt::format("{:%0OI,%0OH,%0OM,%0OS}", tm), "01,01,02,03"); + EXPECT_EQ(fmt::format("{:%_OI,%_OH,%_OM,%_OS}", tm), " 1, 1, 2, 3"); + EXPECT_EQ(fmt::format("{:%-OI,%-OH,%-OM,%-OS}", tm), "1,1,2,3"); + } + +} From f6efeba365988f76f797961f12c57c8328faaa53 Mon Sep 17 00:00:00 2001 From: Shawn Zhong Date: Fri, 13 Jan 2023 03:31:09 -0600 Subject: [PATCH 2/6] Add comments --- include/fmt/chrono.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/fmt/chrono.h b/include/fmt/chrono.h index 041aa926ed0a..21ddeca9e1dd 100644 --- a/include/fmt/chrono.h +++ b/include/fmt/chrono.h @@ -664,10 +664,15 @@ enum class numeric_system { alternative }; +// Glibc extensions for formatting numeric values. enum class pad_type { unspecified, + // Do not pad a numeric result string. none, + // Pad a numeric result string with zeros even if the conversion specifier + // character uses space-padding by default. zero, + // Pad a numeric result string with spaces. space, }; From 43c2eae78d072c64a135907ee0c7bdcaaf631076 Mon Sep 17 00:00:00 2001 From: Shawn Zhong Date: Fri, 13 Jan 2023 04:17:19 -0600 Subject: [PATCH 3/6] Support floating point sec --- include/fmt/chrono.h | 29 +++++++++++++++++------------ test/chrono-test.cc | 15 +++++++++++++++ 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/include/fmt/chrono.h b/include/fmt/chrono.h index 21ddeca9e1dd..d37ed10aadfd 100644 --- a/include/fmt/chrono.h +++ b/include/fmt/chrono.h @@ -676,6 +676,19 @@ enum class pad_type { space, }; +template +auto write_padding(OutputIt out, int width, pad_type pad) { + switch (pad) { + case pad_type::zero: + case pad_type::unspecified: + return std::fill_n(out, width, '0'); + case pad_type::space: + return std::fill_n(out, width, ' '); + case pad_type::none: + return out; + } +} + // Parses a put_time-like format string and invokes handler actions. template FMT_CONSTEXPR const Char* parse_chrono_format(const Char* begin, @@ -1878,17 +1891,7 @@ struct chrono_formatter { to_unsigned(to_nonnegative_int(value, max_value())); int num_digits = detail::count_digits(n); if (width > num_digits) { - switch (pad) { - case pad_type::zero: - case pad_type::unspecified: - out = std::fill_n(out, width - num_digits, '0'); - break; - case pad_type::space: - out = std::fill_n(out, width - num_digits, ' '); - break; - case pad_type::none: - break; - } + out = detail::write_padding(out, width - num_digits, pad); } out = format_decimal(out, n, num_digits).end; } @@ -1974,7 +1977,9 @@ struct chrono_formatter { write_floating_seconds(buf, std::chrono::duration(val), precision); if (negative) *out++ = '-'; - if (buf.size() < 2 || buf[1] == '.') *out++ = '0'; + if (buf.size() < 2 || buf[1] == '.') { + out = detail::write_padding(out, 1, pad); + } out = std::copy(buf.begin(), buf.end(), out); } else { write(second(), 2, pad); diff --git a/test/chrono-test.cc b/test/chrono-test.cc index 839c1726ef48..17e57bf5f15c 100644 --- a/test/chrono-test.cc +++ b/test/chrono-test.cc @@ -949,4 +949,19 @@ TEST(chrono_test, glibc_extensions) { EXPECT_EQ(fmt::format("{:%-OI,%-OH,%-OM,%-OS}", tm), "1,1,2,3"); } + { + const auto d = std::chrono::seconds(3) + std::chrono::milliseconds(140); + EXPECT_EQ(fmt::format("{:%S}", d), "03.140"); + EXPECT_EQ(fmt::format("{:%0S}", d), "03.140"); + EXPECT_EQ(fmt::format("{:%_S}", d), " 3.140"); + EXPECT_EQ(fmt::format("{:%-S}", d), "3.140"); + } + + { + const auto d = std::chrono::duration(3.14); + EXPECT_EQ(fmt::format("{:%S}", d), "03.140000"); + EXPECT_EQ(fmt::format("{:%0S}", d), "03.140000"); + EXPECT_EQ(fmt::format("{:%_S}", d), " 3.140000"); + EXPECT_EQ(fmt::format("{:%-S}", d), "3.140000"); + } } From 6a0f6ff708bcb6d5896ba69b8e502811c356e159 Mon Sep 17 00:00:00 2001 From: Shawn Zhong Date: Fri, 13 Jan 2023 04:24:45 -0600 Subject: [PATCH 4/6] Fix build --- include/fmt/chrono.h | 51 +++++++++++++++++--------------------------- 1 file changed, 19 insertions(+), 32 deletions(-) diff --git a/include/fmt/chrono.h b/include/fmt/chrono.h index d37ed10aadfd..06a311b354e5 100644 --- a/include/fmt/chrono.h +++ b/include/fmt/chrono.h @@ -677,16 +677,15 @@ enum class pad_type { }; template -auto write_padding(OutputIt out, int width, pad_type pad) { - switch (pad) { - case pad_type::zero: - case pad_type::unspecified: - return std::fill_n(out, width, '0'); - case pad_type::space: - return std::fill_n(out, width, ' '); - case pad_type::none: - return out; - } +auto write_padding(OutputIt out, pad_type pad, int width) -> OutputIt { + if (pad == pad_type::none) return out; + return std::fill_n(out, width, pad == pad_type::space ? ' ' : '0'); +} + +template +auto write_padding(OutputIt out, pad_type pad) -> OutputIt { + if (pad != pad_type::none) *out++ = pad == pad_type::space ? ' ' : '0'; + return out; } // Parses a put_time-like format string and invokes handler actions. @@ -1279,26 +1278,14 @@ class tm_writer { *out_++ = *d; } void write2(int value, pad_type pad) { - switch (pad) { - case pad_type::unspecified: - case pad_type::zero: - write2(value); - break; - case pad_type::space: - if (value < 10) { - *out_++ = ' '; - write1(value); - } else { - write2(value); - } - break; - case pad_type::none: - if (value < 10) { - write1(value); - } else { - write2(value); - } - break; + unsigned int v = to_unsigned(value) % 100; + if (v >= 10) { + const char* d = digits2(v); + *out_++ = *d++; + *out_++ = *d; + } else { + out_ = detail::write_padding(out_, pad); + *out_++ = static_cast('0' + v); } } @@ -1891,7 +1878,7 @@ struct chrono_formatter { to_unsigned(to_nonnegative_int(value, max_value())); int num_digits = detail::count_digits(n); if (width > num_digits) { - out = detail::write_padding(out, width - num_digits, pad); + out = detail::write_padding(out, pad, width - num_digits); } out = format_decimal(out, n, num_digits).end; } @@ -1978,7 +1965,7 @@ struct chrono_formatter { precision); if (negative) *out++ = '-'; if (buf.size() < 2 || buf[1] == '.') { - out = detail::write_padding(out, 1, pad); + out = detail::write_padding(out, pad); } out = std::copy(buf.begin(), buf.end(), out); } else { From 5dc570cc0e170dea1c8cd68a8c6ccc1bf92452b0 Mon Sep 17 00:00:00 2001 From: Shawn Zhong Date: Fri, 13 Jan 2023 05:47:34 -0600 Subject: [PATCH 5/6] Fix bug found by fuzzing --- include/fmt/chrono.h | 1 + test/chrono-test.cc | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/include/fmt/chrono.h b/include/fmt/chrono.h index 06a311b354e5..d3a9ec59c2c9 100644 --- a/include/fmt/chrono.h +++ b/include/fmt/chrono.h @@ -706,6 +706,7 @@ FMT_CONSTEXPR const Char* parse_chrono_format(const Char* begin, } if (begin != ptr) handler.on_text(begin, ptr); c = *++ptr; // consume '%' + if (ptr == end) FMT_THROW(format_error("invalid format")); switch (c) { case '_': pad = pad_type::space; diff --git a/test/chrono-test.cc b/test/chrono-test.cc index 17e57bf5f15c..8f02a100ad9b 100644 --- a/test/chrono-test.cc +++ b/test/chrono-test.cc @@ -921,6 +921,13 @@ TEST(chrono_test, timestamps_sub_seconds) { } TEST(chrono_test, glibc_extensions) { + EXPECT_THROW_MSG((void)fmt::format(runtime("{:%0}"), std::chrono::seconds()), + fmt::format_error, "invalid format"); + EXPECT_THROW_MSG((void)fmt::format(runtime("{:%_}"), std::chrono::seconds()), + fmt::format_error, "invalid format"); + EXPECT_THROW_MSG((void)fmt::format(runtime("{:%-}"), std::chrono::seconds()), + fmt::format_error, "invalid format"); + { const auto d = std::chrono::hours(1) + std::chrono::minutes(2) + std::chrono::seconds(3); From 64951f1295a4e9b630592025f8bef9eb59792b9f Mon Sep 17 00:00:00 2001 From: Shawn Zhong Date: Sat, 14 Jan 2023 01:23:44 -0600 Subject: [PATCH 6/6] Fix buffer overflow --- include/fmt/chrono.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/fmt/chrono.h b/include/fmt/chrono.h index d3a9ec59c2c9..f06ac2bca06c 100644 --- a/include/fmt/chrono.h +++ b/include/fmt/chrono.h @@ -705,8 +705,9 @@ FMT_CONSTEXPR const Char* parse_chrono_format(const Char* begin, continue; } if (begin != ptr) handler.on_text(begin, ptr); - c = *++ptr; // consume '%' + ++ptr; // consume '%' if (ptr == end) FMT_THROW(format_error("invalid format")); + c = *ptr; switch (c) { case '_': pad = pad_type::space;