From 472c8cd017adf44038f967e0350fcd0c82fffddc Mon Sep 17 00:00:00 2001 From: Ed Catmur Date: Mon, 7 Feb 2022 00:45:31 +0000 Subject: [PATCH 1/5] Fail parsing if input is insufficient to supply all fields (and literals) of the format spec, even if it is sufficient to fully specify the parsable argument. For example "23:59" is not a valid input for "%H:%M:%S" (or for "%T"). https://eel.is/c++draft/time.parse#17.sentence-1 > If the from_stream overload fails to parse everything specified by > the format string, or if insufficient information is parsed to specify a > complete duration, time point, or calendrical data structure, > setstate(ios_base::failbit) is called on the basic_istream. --- stl/inc/chrono | 8 +++++--- .../test.cpp | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/stl/inc/chrono b/stl/inc/chrono index 09712f9bde..aff28725b4 100644 --- a/stl/inc/chrono +++ b/stl/inc/chrono @@ -4613,8 +4613,10 @@ namespace chrono { const _Ctype& _Ctype_fac{_STD use_facet<_Ctype>(_Iosbase.getloc())}; constexpr _InIt _Last{}; - while (*_Fmt != '\0' && _First != _Last && _State == ios_base::goodbit) { - if (*_Fmt == '%') { + while (*_Fmt != '\0' && (_State & ~ios_base::eofbit) == ios_base::goodbit) { + if (_First == _Last) { + _State |= ios_base::failbit; + } else if (*_Fmt == '%') { _First = _Parse_time_field(_First, _Iosbase, _State, *++_Fmt, '\0', 0, _Subsecond_precision); } else if (_Ctype_fac.narrow(*_First++) != *_Fmt) { _State |= ios_base::failbit; @@ -4847,7 +4849,7 @@ namespace chrono { if (_Ok) { _TRY_IO_BEGIN - for (; _FmtFirst != _FmtLast && _State == ios_base::goodbit; ++_FmtFirst) { + for (; _FmtFirst != _FmtLast && (_State & ~ios_base::eofbit) == ios_base::goodbit; ++_FmtFirst) { if (_First == _Last) { // EOF is not an error if the remaining flags can match zero characters. for (; _FmtFirst != _FmtLast; ++_FmtFirst) { diff --git a/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp b/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp index 1a1c20b3c8..9d245c5147 100644 --- a/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp +++ b/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp @@ -878,6 +878,24 @@ void parse_other_week_date() { assert(ymd == 2022y / January / 1d); } +void parse_incomplete() { + // Parsing should fail if the input is insufficient to supply all fields of the format string, even if the input is + // sufficient to supply all fields of the parsable. + // Check both explicit and shorthand format strings, since the code path is different. + year_month ym; + fail_parse("2021-01", "%Y-%m-%d", ym); + fail_parse("2022-02", "%F", ym); + + seconds time; + fail_parse("01:59", "%H:%M:%S", time); + fail_parse("03:23", "%T", time); + fail_parse("04", "%R", time); + + // However, it is OK to omit seconds from the format when parsing a duration to seconds precision. + test_parse("05:24", "%H:%M", time); + test_parse("06:25", "%R", time); +} + void parse_whitespace() { seconds time; fail_parse("ab", "a%nb", time); @@ -1213,6 +1231,7 @@ void test_parse() { parse_calendar_types_basic(); parse_iso_week_date(); parse_other_week_date(); + parse_incomplete(); parse_whitespace(); parse_timepoints(); test_io_manipulator(); From c49f820724e9cdf66e04b3c5148ffeaafb14cd0e Mon Sep 17 00:00:00 2001 From: Ed Catmur Date: Sun, 13 Feb 2022 23:38:07 +0000 Subject: [PATCH 2/5] Update stl/inc/chrono Co-authored-by: Matt Stephanson <68978048+MattStephanson@users.noreply.github.com> --- stl/inc/chrono | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/chrono b/stl/inc/chrono index aff28725b4..eac2bc278e 100644 --- a/stl/inc/chrono +++ b/stl/inc/chrono @@ -4615,7 +4615,7 @@ namespace chrono { while (*_Fmt != '\0' && (_State & ~ios_base::eofbit) == ios_base::goodbit) { if (_First == _Last) { - _State |= ios_base::failbit; + _State |= ios_base::failbit | ios_base::eofbit; } else if (*_Fmt == '%') { _First = _Parse_time_field(_First, _Iosbase, _State, *++_Fmt, '\0', 0, _Subsecond_precision); } else if (_Ctype_fac.narrow(*_First++) != *_Fmt) { From adb644ed1ed1b14aa13ae07678510ea792e45ed5 Mon Sep 17 00:00:00 2001 From: Ed Catmur Date: Sun, 13 Feb 2022 23:42:33 +0000 Subject: [PATCH 3/5] Add tests as suggested in PR. Test for whitespace after other fields (explicit and shorthand) to ensure that we don't return early on EOF. Check exact rdstate flags to ensure that we set eofbit in addition to failbit. --- .../test.cpp | 52 +++++++------------ 1 file changed, 19 insertions(+), 33 deletions(-) diff --git a/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp b/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp index 9d245c5147..159f3fe1d0 100644 --- a/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp +++ b/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp @@ -127,7 +127,7 @@ void test_duration_output() { template -void test_parse(const CharT* str, const CStringOrStdString& fmt, Parsable& p, +std::ios_base::iostate parse_state(const CharT* str, const CStringOrStdString& fmt, Parsable& p, type_identity_t*> abbrev = nullptr, minutes* offset = nullptr) { p = Parsable{}; if (abbrev) { @@ -157,41 +157,19 @@ void test_parse(const CharT* str, const CStringOrStdString& fmt, Parsable& p, } } - assert(sstr); + return sstr.rdstate(); } template -void fail_parse(const CharT* str, const CStringOrStdString& fmt, Parsable& p, +void test_parse(const CharT* str, const CStringOrStdString& fmt, Parsable& p, type_identity_t*> abbrev = nullptr, minutes* offset = nullptr) { - p = Parsable{}; - if (abbrev) { - if constexpr (is_same_v) { - *abbrev = "!"; - } else { - *abbrev = L"!"; - } - } - - if (offset) { - *offset = minutes::min(); - } - - basic_stringstream sstr{str}; - if (abbrev) { - if (offset) { - sstr >> parse(fmt, p, *abbrev, *offset); - } else { - sstr >> parse(fmt, p, *abbrev); - } - } else { - if (offset) { - sstr >> parse(fmt, p, *offset); - } else { - sstr >> parse(fmt, p); - } - } + assert((parse_state(str, fmt, p, abbrev, offset) & ~std::ios_base::eofbit) == 0); +} - assert(!sstr); +template +void fail_parse(const CharT* str, const CStringOrStdString& fmt, Parsable& p, + type_identity_t*> abbrev = nullptr, minutes* offset = nullptr) { + assert((parse_state(str, fmt, p, abbrev, offset) & ~std::ios_base::eofbit) != 0); } template @@ -883,14 +861,22 @@ void parse_incomplete() { // sufficient to supply all fields of the parsable. // Check both explicit and shorthand format strings, since the code path is different. year_month ym; - fail_parse("2021-01", "%Y-%m-%d", ym); - fail_parse("2022-02", "%F", ym); + assert(parse_state("2021-01", "%Y-%m-%d", ym) == (std::ios_base::eofbit | std::ios_base::failbit)); + assert(parse_state("2022-02", "%F", ym) == (std::ios_base::eofbit | std::ios_base::failbit)); + assert(parse_state("2021-", "%Y-%m-%d", ym) == (std::ios_base::eofbit | std::ios_base::failbit)); + assert(parse_state("2022-", "%F", ym) == (std::ios_base::eofbit | std::ios_base::failbit)); seconds time; fail_parse("01:59", "%H:%M:%S", time); fail_parse("03:23", "%T", time); fail_parse("04", "%R", time); + // Check for parsing of whitespace fields after other fields. More whitespace tests below. + test_parse("15:19", "%H:%M%t", time); + test_parse("15:19", "%R%t", time); + fail_parse("15:19", "%H:%M%n", time); + fail_parse("15:19", "%R%n", time); + // However, it is OK to omit seconds from the format when parsing a duration to seconds precision. test_parse("05:24", "%H:%M", time); test_parse("06:25", "%R", time); From e7f64875c7db562cddd35cacc351e9b6f709373e Mon Sep 17 00:00:00 2001 From: Ed Catmur Date: Fri, 18 Feb 2022 01:21:02 +0000 Subject: [PATCH 4/5] style fix --- .../P0355R7_calendars_and_time_zones_io/test.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp b/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp index 159f3fe1d0..e471f8bdf1 100644 --- a/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp +++ b/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp @@ -127,7 +127,7 @@ void test_duration_output() { template -std::ios_base::iostate parse_state(const CharT* str, const CStringOrStdString& fmt, Parsable& p, +ios_base::iostate parse_state(const CharT* str, const CStringOrStdString& fmt, Parsable& p, type_identity_t*> abbrev = nullptr, minutes* offset = nullptr) { p = Parsable{}; if (abbrev) { @@ -163,13 +163,13 @@ std::ios_base::iostate parse_state(const CharT* str, const CStringOrStdString& f template void test_parse(const CharT* str, const CStringOrStdString& fmt, Parsable& p, type_identity_t*> abbrev = nullptr, minutes* offset = nullptr) { - assert((parse_state(str, fmt, p, abbrev, offset) & ~std::ios_base::eofbit) == 0); + assert((parse_state(str, fmt, p, abbrev, offset) & ~ios_base::eofbit) == 0); } template void fail_parse(const CharT* str, const CStringOrStdString& fmt, Parsable& p, type_identity_t*> abbrev = nullptr, minutes* offset = nullptr) { - assert((parse_state(str, fmt, p, abbrev, offset) & ~std::ios_base::eofbit) != 0); + assert((parse_state(str, fmt, p, abbrev, offset) & ~ios_base::eofbit) != 0); } template @@ -861,10 +861,10 @@ void parse_incomplete() { // sufficient to supply all fields of the parsable. // Check both explicit and shorthand format strings, since the code path is different. year_month ym; - assert(parse_state("2021-01", "%Y-%m-%d", ym) == (std::ios_base::eofbit | std::ios_base::failbit)); - assert(parse_state("2022-02", "%F", ym) == (std::ios_base::eofbit | std::ios_base::failbit)); - assert(parse_state("2021-", "%Y-%m-%d", ym) == (std::ios_base::eofbit | std::ios_base::failbit)); - assert(parse_state("2022-", "%F", ym) == (std::ios_base::eofbit | std::ios_base::failbit)); + assert(parse_state("2021-01", "%Y-%m-%d", ym) == (ios_base::eofbit | ios_base::failbit)); + assert(parse_state("2022-02", "%F", ym) == (ios_base::eofbit | ios_base::failbit)); + assert(parse_state("2021-", "%Y-%m-%d", ym) == (ios_base::eofbit | ios_base::failbit)); + assert(parse_state("2022-", "%F", ym) == (ios_base::eofbit | ios_base::failbit)); seconds time; fail_parse("01:59", "%H:%M:%S", time); From 85447fdd38475081d0951925363e8271e60b4162 Mon Sep 17 00:00:00 2001 From: Ed Catmur Date: Sat, 19 Feb 2022 22:16:41 +0100 Subject: [PATCH 5/5] Code review Break out of loop after setting failbit | eofbit Test against goodbit instead of 0 --- stl/inc/chrono | 1 + tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/stl/inc/chrono b/stl/inc/chrono index eac2bc278e..7a676dc235 100644 --- a/stl/inc/chrono +++ b/stl/inc/chrono @@ -4616,6 +4616,7 @@ namespace chrono { while (*_Fmt != '\0' && (_State & ~ios_base::eofbit) == ios_base::goodbit) { if (_First == _Last) { _State |= ios_base::failbit | ios_base::eofbit; + break; } else if (*_Fmt == '%') { _First = _Parse_time_field(_First, _Iosbase, _State, *++_Fmt, '\0', 0, _Subsecond_precision); } else if (_Ctype_fac.narrow(*_First++) != *_Fmt) { diff --git a/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp b/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp index e471f8bdf1..fd83418233 100644 --- a/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp +++ b/tests/std/tests/P0355R7_calendars_and_time_zones_io/test.cpp @@ -163,13 +163,13 @@ ios_base::iostate parse_state(const CharT* str, const CStringOrStdString& fmt, P template void test_parse(const CharT* str, const CStringOrStdString& fmt, Parsable& p, type_identity_t*> abbrev = nullptr, minutes* offset = nullptr) { - assert((parse_state(str, fmt, p, abbrev, offset) & ~ios_base::eofbit) == 0); + assert((parse_state(str, fmt, p, abbrev, offset) & ~ios_base::eofbit) == ios_base::goodbit); } template void fail_parse(const CharT* str, const CStringOrStdString& fmt, Parsable& p, type_identity_t*> abbrev = nullptr, minutes* offset = nullptr) { - assert((parse_state(str, fmt, p, abbrev, offset) & ~ios_base::eofbit) != 0); + assert((parse_state(str, fmt, p, abbrev, offset) & ~ios_base::eofbit) != ios_base::goodbit); } template