From c316691ab9def3570c8214ea41f7013ae16e90b6 Mon Sep 17 00:00:00 2001 From: Nikolay Baklicharov Date: Sun, 14 Jul 2024 19:17:42 +0300 Subject: [PATCH 01/11] Fix std::put_time() crash on invalid format specifier --- stl/inc/xloctime | 53 +++++++++++++++++-- .../std/tests/Dev11_0836436_get_time/test.cpp | 28 +++++++++- 2 files changed, 76 insertions(+), 5 deletions(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index 52da0f5bae..8b713b63ea 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -651,6 +651,17 @@ protected: __CLR_OR_THIS_CALL ~time_get_byname() noexcept override {} }; +constexpr bool _Is_valid_fmt_specifier(const char _Specifier) { + constexpr char _Valid_specifiers[] = "aAbBcCdDeFgGhHIjmMnprRStTuUVwWxXyYzZ"; + for (const char _Valid_specifier : _Valid_specifiers) { + if (_Valid_specifier == _Specifier) { + return true; + } + } + + return false; +} + _EXPORT_STD extern "C++" template >> class time_put : public locale::facet { // facet for converting encoded times to text public: @@ -687,7 +698,19 @@ public: _Specifier = _Ctype_fac.narrow(*_Fmtfirst); } - _Dest = do_put(_Dest, _Iosbase, _Fill, _Pt, _Specifier, _Modifier); // convert a single field + if (_Specifier == '%' && _Modifier == '\0') { + // if the specifier is percent and no modifier is set, just append it + *_Dest++ = _Percent; + } else if (!_Is_valid_fmt_specifier(_Specifier)) { + // no valid specifier, directly copy as literal elements + *_Dest++ = _Percent; + if (_Modifier != '\0') { + *_Dest++ = _Modifier; + } + *_Dest++ = _Specifier; + } else { + _Dest = do_put(_Dest, _Iosbase, _Fill, _Pt, _Specifier, _Modifier); // convert a single field + } } } @@ -811,7 +834,19 @@ public: _Specifier = _Ctype_fac.narrow(*_Fmtfirst); } - _Dest = do_put(_Dest, _Iosbase, _Fill, _Pt, _Specifier, _Modifier); // convert a single field + if (_Specifier == '%' && _Modifier == '\0') { + // if the specifier is percent and no modifier is set, just append it + *_Dest++ = _Percent; + } else if (!_Is_valid_fmt_specifier(_Specifier)) { + // no valid specifier, directly copy as literal elements + *_Dest++ = _Percent; + *_Dest++ = _Raw; + if (_Modifier != '\0') { + *_Dest++ = *_Fmtfirst; + } + } else { + _Dest = do_put(_Dest, _Iosbase, _Fill, _Pt, _Specifier, _Modifier); // convert a single field + } } } @@ -943,7 +978,19 @@ public: _Specifier = _Ctype_fac.narrow(*_Fmtfirst); } - _Dest = do_put(_Dest, _Iosbase, _Fill, _Pt, _Specifier, _Modifier); // convert a single field + if (_Specifier == '%' && _Modifier == '\0') { + // if the specifier is percent and no modifier is set, just append it + *_Dest++ = _Percent; + } else if (!_Is_valid_fmt_specifier(_Specifier)) { + // no valid specifier, directly copy as literal elements + *_Dest++ = _Percent; + if (_Modifier != '\0') { + *_Dest++ = _Modifier; + } + *_Dest++ = _Specifier; + } else { + _Dest = do_put(_Dest, _Iosbase, _Fill, _Pt, _Specifier, _Modifier); // convert a single field + } } } diff --git a/tests/std/tests/Dev11_0836436_get_time/test.cpp b/tests/std/tests/Dev11_0836436_get_time/test.cpp index 7d2409b522..c1463775ba 100644 --- a/tests/std/tests/Dev11_0836436_get_time/test.cpp +++ b/tests/std/tests/Dev11_0836436_get_time/test.cpp @@ -110,6 +110,7 @@ void test_invalid_argument(); void test_buffer_resizing(); void test_gh_2618(); void test_gh_2848(); +void test_gh_4820(); int main() { assert(read_hour("12 AM") == 0); @@ -157,6 +158,7 @@ int main() { test_buffer_resizing(); test_gh_2618(); test_gh_2848(); + test_gh_4820(); } typedef istreambuf_iterator Iter; @@ -792,16 +794,17 @@ void test_invalid_argument() { time_t t = time(nullptr); tm currentTime; localtime_s(¤tTime, &t); + currentTime.tm_hour = 25; // set invalid hour { wstringstream wss; - wss << put_time(¤tTime, L"%Y-%m-%d-%H-%M-%s"); + wss << put_time(¤tTime, L"%Y-%m-%d-%H-%M"); assert(wss.rdstate() == ios_base::badbit); } { stringstream ss; - ss << put_time(¤tTime, "%Y-%m-%d-%H-%M-%s"); + ss << put_time(¤tTime, "%Y-%m-%d-%H-%M"); assert(ss.rdstate() == ios_base::badbit); } #endif // _M_CEE_PURE @@ -905,3 +908,24 @@ void test_gh_2848() { assert(err == (ios_base::eofbit | ios_base::failbit)); } } + +void test_gh_4820() { + // GH-4820 : std::put_time should copy unknown conversion specifiers instead of crash + time_t t = time(nullptr); + tm currentTime; + localtime_s(¤tTime, &t); + + { + wstringstream wss; + wss << put_time(¤tTime, L"%Ei%!%E%J%P"); + assert(wss.rdstate() == ios_base::goodbit); + assert(wss.str() == L"%Ei%!%E%J%P"); + } + + { + stringstream ss; + ss << put_time(¤tTime, "%Ei%!%E%J%P"); + assert(ss.rdstate() == ios_base::goodbit); + assert(ss.str() == "%Ei%!%E%J%P"); + } +} From a8211a8d4876e117a82a15be7b52c1fe3de4b923 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 5 Aug 2024 01:09:46 -0700 Subject: [PATCH 02/11] Perf: Lift out array as `_INLINE_VAR constexpr`. --- stl/inc/xloctime | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index 8b713b63ea..a7ba36ecf0 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -651,8 +651,9 @@ protected: __CLR_OR_THIS_CALL ~time_get_byname() noexcept override {} }; +_INLINE_VAR constexpr char _Valid_specifiers[] = "aAbBcCdDeFgGhHIjmMnprRStTuUVwWxXyYzZ"; + constexpr bool _Is_valid_fmt_specifier(const char _Specifier) { - constexpr char _Valid_specifiers[] = "aAbBcCdDeFgGhHIjmMnprRStTuUVwWxXyYzZ"; for (const char _Valid_specifier : _Valid_specifiers) { if (_Valid_specifier == _Specifier) { return true; From c05887dae15fa3f240751e3dfd26c62ee8e5fa02 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 5 Aug 2024 01:24:08 -0700 Subject: [PATCH 03/11] Avoid string literal in initializer, which stores a null character. --- stl/inc/xloctime | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index a7ba36ecf0..0670a098ac 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -651,7 +651,8 @@ protected: __CLR_OR_THIS_CALL ~time_get_byname() noexcept override {} }; -_INLINE_VAR constexpr char _Valid_specifiers[] = "aAbBcCdDeFgGhHIjmMnprRStTuUVwWxXyYzZ"; +_INLINE_VAR constexpr char _Valid_specifiers[] = {'a', 'A', 'b', 'B', 'c', 'C', 'd', 'D', 'e', 'F', 'g', 'G', 'h', 'H', + 'I', 'j', 'm', 'M', 'n', 'p', 'r', 'R', 'S', 't', 'T', 'u', 'U', 'V', 'w', 'W', 'x', 'X', 'y', 'Y', 'z', 'Z'}; constexpr bool _Is_valid_fmt_specifier(const char _Specifier) { for (const char _Valid_specifier : _Valid_specifiers) { From b36f81999d0dcfd52fb393bf8ad04df624fdfb51 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 5 Aug 2024 01:29:59 -0700 Subject: [PATCH 04/11] Clarity: Mention "strftime specifiers" in names. --- stl/inc/xloctime | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index 0670a098ac..6f55ad26f9 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -651,11 +651,12 @@ protected: __CLR_OR_THIS_CALL ~time_get_byname() noexcept override {} }; -_INLINE_VAR constexpr char _Valid_specifiers[] = {'a', 'A', 'b', 'B', 'c', 'C', 'd', 'D', 'e', 'F', 'g', 'G', 'h', 'H', - 'I', 'j', 'm', 'M', 'n', 'p', 'r', 'R', 'S', 't', 'T', 'u', 'U', 'V', 'w', 'W', 'x', 'X', 'y', 'Y', 'z', 'Z'}; +_INLINE_VAR constexpr char _Valid_strftime_specifiers[] = {'a', 'A', 'b', 'B', 'c', 'C', 'd', 'D', 'e', 'F', 'g', 'G', + 'h', 'H', 'I', 'j', 'm', 'M', 'n', 'p', 'r', 'R', 'S', 't', 'T', 'u', 'U', 'V', 'w', 'W', 'x', 'X', 'y', 'Y', 'z', + 'Z'}; -constexpr bool _Is_valid_fmt_specifier(const char _Specifier) { - for (const char _Valid_specifier : _Valid_specifiers) { +constexpr bool _Is_valid_strftime_specifier(const char _Specifier) { + for (const char _Valid_specifier : _Valid_strftime_specifiers) { if (_Valid_specifier == _Specifier) { return true; } @@ -703,7 +704,7 @@ public: if (_Specifier == '%' && _Modifier == '\0') { // if the specifier is percent and no modifier is set, just append it *_Dest++ = _Percent; - } else if (!_Is_valid_fmt_specifier(_Specifier)) { + } else if (!_Is_valid_strftime_specifier(_Specifier)) { // no valid specifier, directly copy as literal elements *_Dest++ = _Percent; if (_Modifier != '\0') { @@ -839,7 +840,7 @@ public: if (_Specifier == '%' && _Modifier == '\0') { // if the specifier is percent and no modifier is set, just append it *_Dest++ = _Percent; - } else if (!_Is_valid_fmt_specifier(_Specifier)) { + } else if (!_Is_valid_strftime_specifier(_Specifier)) { // no valid specifier, directly copy as literal elements *_Dest++ = _Percent; *_Dest++ = _Raw; @@ -983,7 +984,7 @@ public: if (_Specifier == '%' && _Modifier == '\0') { // if the specifier is percent and no modifier is set, just append it *_Dest++ = _Percent; - } else if (!_Is_valid_fmt_specifier(_Specifier)) { + } else if (!_Is_valid_strftime_specifier(_Specifier)) { // no valid specifier, directly copy as literal elements *_Dest++ = _Percent; if (_Modifier != '\0') { From dbb8aa743bec30053637311ce49da29d43f2e1e6 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 5 Aug 2024 01:40:11 -0700 Subject: [PATCH 05/11] Cite the C Standard. C++ currently refers to C17, but C23 lists the same specifiers and fixes a damaged mention of %b. --- stl/inc/xloctime | 1 + 1 file changed, 1 insertion(+) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index 6f55ad26f9..233726f347 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -651,6 +651,7 @@ protected: __CLR_OR_THIS_CALL ~time_get_byname() noexcept override {} }; +// C23 7.29.3.5 "The strftime function"/3 _INLINE_VAR constexpr char _Valid_strftime_specifiers[] = {'a', 'A', 'b', 'B', 'c', 'C', 'd', 'D', 'e', 'F', 'g', 'G', 'h', 'H', 'I', 'j', 'm', 'M', 'n', 'p', 'r', 'R', 'S', 't', 'T', 'u', 'U', 'V', 'w', 'W', 'x', 'X', 'y', 'Y', 'z', 'Z'}; From a313fcfa5cdc7f42c549b8eb7065f55e6195bfd8 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 5 Aug 2024 01:46:30 -0700 Subject: [PATCH 06/11] Add `_NODISCARD`. --- stl/inc/xloctime | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index 233726f347..3f35c7c26e 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -656,7 +656,7 @@ _INLINE_VAR constexpr char _Valid_strftime_specifiers[] = {'a', 'A', 'b', 'B', ' 'h', 'H', 'I', 'j', 'm', 'M', 'n', 'p', 'r', 'R', 'S', 't', 'T', 'u', 'U', 'V', 'w', 'W', 'x', 'X', 'y', 'Y', 'z', 'Z'}; -constexpr bool _Is_valid_strftime_specifier(const char _Specifier) { +_NODISCARD constexpr bool _Is_valid_strftime_specifier(const char _Specifier) { for (const char _Valid_specifier : _Valid_strftime_specifiers) { if (_Valid_specifier == _Specifier) { return true; From 33636c61c5da6edd0e7dc94210cfc21f6ef0ed15 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 5 Aug 2024 01:50:47 -0700 Subject: [PATCH 07/11] Style: Avoid Yoda conditions, we will. --- stl/inc/xloctime | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index 3f35c7c26e..c78b3eab88 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -658,7 +658,7 @@ _INLINE_VAR constexpr char _Valid_strftime_specifiers[] = {'a', 'A', 'b', 'B', ' _NODISCARD constexpr bool _Is_valid_strftime_specifier(const char _Specifier) { for (const char _Valid_specifier : _Valid_strftime_specifiers) { - if (_Valid_specifier == _Specifier) { + if (_Specifier == _Valid_specifier) { return true; } } From 5a4c6e8b75b57c09c86d70333a1cc799d1e2f762 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 5 Aug 2024 03:14:37 -0700 Subject: [PATCH 08/11] Style: Use `const auto&` in range-for. --- stl/inc/xloctime | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index c78b3eab88..4921663641 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -657,7 +657,7 @@ _INLINE_VAR constexpr char _Valid_strftime_specifiers[] = {'a', 'A', 'b', 'B', ' 'Z'}; _NODISCARD constexpr bool _Is_valid_strftime_specifier(const char _Specifier) { - for (const char _Valid_specifier : _Valid_strftime_specifiers) { + for (const auto& _Valid_specifier : _Valid_strftime_specifiers) { if (_Specifier == _Valid_specifier) { return true; } From da31025e28b87390560ef6f3e895a4322427bc9f Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 5 Aug 2024 04:41:30 -0700 Subject: [PATCH 09/11] Clarity: `time_put` should resemble `time_put`. --- stl/inc/xloctime | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index 4921663641..2085af41c3 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -844,10 +844,10 @@ public: } else if (!_Is_valid_strftime_specifier(_Specifier)) { // no valid specifier, directly copy as literal elements *_Dest++ = _Percent; - *_Dest++ = _Raw; if (_Modifier != '\0') { - *_Dest++ = *_Fmtfirst; + *_Dest++ = _Raw; } + *_Dest++ = *_Fmtfirst; } else { _Dest = do_put(_Dest, _Iosbase, _Fill, _Pt, _Specifier, _Modifier); // convert a single field } From feec8029802e628f90159e36e627b0aab7e6e8df Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 5 Aug 2024 04:42:36 -0700 Subject: [PATCH 10/11] Bugfix: Avoid truncation in `time_put`. --- stl/inc/xloctime | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index 2085af41c3..6926937743 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -966,14 +966,15 @@ public: *_Dest++ = _Fmtfirst[-1]; break; } else { // get specifier after % - char _Specifier = _Ctype_fac.narrow(*_Fmtfirst); + _Elem _Raw = *_Fmtfirst; + char _Specifier = _Ctype_fac.narrow(_Raw); char _Modifier = '\0'; _Elem _Percent = _Fmtfirst[-1]; if (_Specifier == 'E' || _Specifier == 'O' || _Specifier == 'Q' || _Specifier == '#') { if (++_Fmtfirst == _Fmtlast) { // no specifier, copy %[E0Q#] as literal elements *_Dest++ = _Percent; - *_Dest++ = _Specifier; + *_Dest++ = _Raw; break; } @@ -989,9 +990,9 @@ public: // no valid specifier, directly copy as literal elements *_Dest++ = _Percent; if (_Modifier != '\0') { - *_Dest++ = _Modifier; + *_Dest++ = _Raw; } - *_Dest++ = _Specifier; + *_Dest++ = *_Fmtfirst; } else { _Dest = do_put(_Dest, _Iosbase, _Fill, _Pt, _Specifier, _Modifier); // convert a single field } From 1548a59ed735fd191c1574990d044da2a7cd1a65 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 5 Aug 2024 05:46:19 -0700 Subject: [PATCH 11/11] Add more test coverage with comments. --- .../std/tests/Dev11_0836436_get_time/test.cpp | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/tests/std/tests/Dev11_0836436_get_time/test.cpp b/tests/std/tests/Dev11_0836436_get_time/test.cpp index c1463775ba..ce5152ca1c 100644 --- a/tests/std/tests/Dev11_0836436_get_time/test.cpp +++ b/tests/std/tests/Dev11_0836436_get_time/test.cpp @@ -915,17 +915,32 @@ void test_gh_4820() { tm currentTime; localtime_s(¤tTime, &t); + // Case 1: Test various unknown conversion specifiers. + // Case 2: "%%" is a known escape sequence with a dedicated fast path. + // Case 3: "% " is percent followed by space, which is an unknown conversion specifier. + // Case 4: "%E%Z" is parsed as "%E%" followed by "Z", so it should be copied unchanged, + // even though "%Z" by itself would be a known conversion specifier (time zone name). + // (In case 1, "%E%J" is parsed the same way; the difference is that "%J" would be unknown.) { wstringstream wss; - wss << put_time(¤tTime, L"%Ei%!%E%J%P"); + wss << put_time(¤tTime, L"1:%Ei%!%E%J%P 2:%% 3:% 4:%E%Z"); assert(wss.rdstate() == ios_base::goodbit); - assert(wss.str() == L"%Ei%!%E%J%P"); + assert(wss.str() == L"1:%Ei%!%E%J%P 2:% 3:% 4:%E%Z"); } { stringstream ss; - ss << put_time(¤tTime, "%Ei%!%E%J%P"); + ss << put_time(¤tTime, "1:%Ei%!%E%J%P 2:%% 3:% 4:%E%Z"); assert(ss.rdstate() == ios_base::goodbit); - assert(ss.str() == "%Ei%!%E%J%P"); + assert(ss.str() == "1:%Ei%!%E%J%P 2:% 3:% 4:%E%Z"); + } + + // Also verify that wide characters aren't truncated. + // This tests a character appearing by itself, two as specifiers, and two as modified specifiers. + { + wstringstream wss; + wss << put_time(¤tTime, L"\x043a%\x043e%\x0448%E\x043a%O\x0430"); + assert(wss.rdstate() == ios_base::goodbit); + assert(wss.str() == L"\x043a%\x043e%\x0448%E\x043a%O\x0430"); } }