From 33cc28b22e3fc60fc1f7ec84c0ab10acb06287ab Mon Sep 17 00:00:00 2001 From: Matt Stephanson Date: Wed, 3 Feb 2021 17:28:14 -0800 Subject: [PATCH 1/4] Fixes #1606: get_time handles leading zeros incorrectly Also: - failbit should be set if the stream is EOF while not parsing a specifier. - A '%' at the end of a format string is an error, not a literal. - Reads leading spaces, in addition to leading zeros. --- stl/inc/xloctime | 46 ++++++++++--------- .../std/tests/Dev11_0836436_get_time/test.cpp | 42 +++++++++++++++++ 2 files changed, 67 insertions(+), 21 deletions(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index d812c1b5743..535cbe2841a 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -93,7 +93,15 @@ public: _State = ios_base::goodbit; for (; _Fmtfirst != _Fmtlast; ++_Fmtfirst) { - if (_Ctype_fac.narrow(*_Fmtfirst) != '%') { // match literal element + if (_State != ios_base::goodbit) { + // [locale.time.get.members] p8.2 + // _State is fail, eof, or bad. Do not proceed to the next fields. Return with current _State. + break; + } else if (_First == _Last) { + // p8.3 + _State = ios_base::eofbit | ios_base::failbit; + break; + } else if (_Ctype_fac.narrow(*_Fmtfirst) != '%') { // match literal element if (_Ctype_fac.is(_Ctype::space, *_Fmtfirst)) { while (_First != _Last && _Ctype_fac.is(_Ctype::space, *_First)) { ++_First; @@ -104,12 +112,11 @@ public: } else { ++_First; } - } else if (++_Fmtfirst == _Fmtlast) { // treat trailing % as literal match - if (*_First != _Fmtfirst[-1]) { - _State |= ios_base::failbit; - } else { - ++_First; - } + } else if (++_Fmtfirst == _Fmtlast) { + // [locale.time.get.members]/8.4: "If the number of elements in the range [fmt, fmtend) is not + // sufficient to unambiguously determine whether the conversion specification is complete and valid, the + // function evaluates err = ios_base::failbit." + _State = ios_base::failbit; break; } else { // get specifier after % char _Specifier = _Ctype_fac.narrow(*_Fmtfirst); @@ -132,18 +139,9 @@ public: } _First = do_get(_First, _Last, _Iosbase, _State, _Pt, _Specifier, _Modifier); // convert a single field - - if (_State != ios_base::goodbit) { - // _State is fail, eof, or bad. Do not proceed to the next fields. Return with current _State. - break; - } } } - if (_First == _Last) { - _State |= ios_base::eofbit; - } - return _First; } @@ -223,7 +221,7 @@ protected: if (_State != ios_base::goodbit || _Ctype_fac.narrow(*_First) != ':') { _State |= ios_base::failbit; // min field is bad } else { - _State |= _Getint(++_First, _Last, 0, 59, _Pt->tm_sec, _Ctype_fac); + _State |= _Getint(++_First, _Last, 0, 60, _Pt->tm_sec, _Ctype_fac); } return _First; @@ -546,7 +544,14 @@ private: char* _Ptr = _Ac; char _Ch; - if (_First != _Last) { + int _Digits_seen = 0; + + while (_First != _Last && _Digits_seen < _Hi_digits && _Ctype_fac.is(ctype_base::space, *_First)) { + ++_First; + ++_Digits_seen; + } + + if (_First != _Last && _Digits_seen < _Hi_digits) { if ((_Ch = _Ctype_fac.narrow(*_First)) == '+') { // copy plus sign *_Ptr++ = '+'; ++_First; @@ -556,9 +561,8 @@ private: } } - int _Digits_seen = 0; - - for (; _First != _Last && _Ctype_fac.narrow(*_First) == '0'; ++_First) { // strip leading zeros + for (; _First != _Last && _Digits_seen < _Hi_digits && _Ctype_fac.narrow(*_First) == '0'; + ++_First) { // strip leading zeros ++_Digits_seen; } diff --git a/tests/std/tests/Dev11_0836436_get_time/test.cpp b/tests/std/tests/Dev11_0836436_get_time/test.cpp index 2d704e204dd..2ee74f2dfd2 100644 --- a/tests/std/tests/Dev11_0836436_get_time/test.cpp +++ b/tests/std/tests/Dev11_0836436_get_time/test.cpp @@ -494,6 +494,48 @@ void test_990695() { assert(t.tm_min == 8); assert(t.tm_sec == 0); } + + { + // Should fail if EOF while not parsing specifier ([locale.time.get.members]/8.4). + tm t{}; + stringstream ss{"4"}; + ss >> get_time(&t, "42"); + assert(ss.rdstate() == (ios_base::eofbit | ios_base::failbit)); + } + + { + // Trailing % should not be treated as a literal (p8.4 again). + tm t{}; + stringstream ss{"%"}; + ss >> get_time(&t, "%"); + assert(ss.fail()); + } + + { + // GH-1606: reads too many leading zeros + istringstream iss("19700405T000006"); + tm t{}; + iss >> get_time(&t, "%Y%m%dT%H%M%S"); + assert(iss); + + printf("Expected hour 0, min 0, sec 6\n"); + printf(" Got hour %d, min %d, sec %d\n", t.tm_hour, t.tm_min, t.tm_sec); + + assert(t.tm_year == 70); + assert(t.tm_mon == 3); + assert(t.tm_mday == 5); + assert(t.tm_hour == 0); + assert(t.tm_min == 0); + assert(t.tm_sec == 6); + } + + { + // strptime specification: "leading zeros are permitted but not required" + tm t{}; + stringstream{" 7 4"} >> get_time(&t, "%m%d"); + assert(t.tm_mon == 6); + assert(t.tm_mday == 4); + } } } From 187904d38f441f87fbdee734d4d74b106fbc1faf Mon Sep 17 00:00:00 2001 From: Matt Stephanson Date: Fri, 5 Feb 2021 19:45:13 -0800 Subject: [PATCH 2/4] more comformance improvements more consistency with existing test code style --- stl/inc/xloctime | 18 ++++------- .../std/tests/Dev11_0836436_get_time/test.cpp | 30 ++++++++++++++----- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index 535cbe2841a..f6c4b1ad213 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -106,31 +106,25 @@ public: while (_First != _Last && _Ctype_fac.is(_Ctype::space, *_First)) { ++_First; } - } else if (*_First != *_Fmtfirst) { // bad literal match + } else if (_Ctype_fac.toupper(*_First) != _Ctype_fac.toupper(*_Fmtfirst)) { // bad literal match _State |= ios_base::failbit; break; } else { ++_First; } } else if (++_Fmtfirst == _Fmtlast) { - // [locale.time.get.members]/8.4: "If the number of elements in the range [fmt, fmtend) is not - // sufficient to unambiguously determine whether the conversion specification is complete and valid, the - // function evaluates err = ios_base::failbit." + // p8.4: "If the number of elements in the range [fmt, fmtend) is not sufficient to unambiguously + // determine whether the conversion specification is complete and valid, the function evaluates err = + // ios_base::failbit." _State = ios_base::failbit; break; } else { // get specifier after % char _Specifier = _Ctype_fac.narrow(*_Fmtfirst); char _Modifier = '\0'; - _Elem _Percent = _Fmtfirst[-1]; if (_Specifier == 'E' || _Specifier == 'O' || _Specifier == 'Q' || _Specifier == '#') { - if (++_Fmtfirst == _Fmtlast) { // no specifier, treat %[E0Q#] as literal match - if (*_First != _Percent || ++_First == _Last || _Ctype_fac.narrow(*_First) != _Specifier) { - _State |= ios_base::failbit; - } else { - ++_First; - } - + if (++_Fmtfirst == _Fmtlast) { // no specifier + _State = ios_base::failbit; break; } else { // save both qualifier and specifier _Modifier = _Specifier; diff --git a/tests/std/tests/Dev11_0836436_get_time/test.cpp b/tests/std/tests/Dev11_0836436_get_time/test.cpp index 2ee74f2dfd2..07f2abd6f15 100644 --- a/tests/std/tests/Dev11_0836436_get_time/test.cpp +++ b/tests/std/tests/Dev11_0836436_get_time/test.cpp @@ -498,17 +498,33 @@ void test_990695() { { // Should fail if EOF while not parsing specifier ([locale.time.get.members]/8.4). tm t{}; - stringstream ss{"4"}; - ss >> get_time(&t, "42"); - assert(ss.rdstate() == (ios_base::eofbit | ios_base::failbit)); + istringstream iss("4"); + iss >> get_time(&t, "42"); + assert(iss.rdstate() == (ios_base::eofbit | ios_base::failbit)); } { // Trailing % should not be treated as a literal (p8.4 again). tm t{}; - stringstream ss{"%"}; - ss >> get_time(&t, "%"); - assert(ss.fail()); + istringstream iss("%"); + iss >> get_time(&t, "%"); + assert(iss.fail()); + } + + { + // % with modifier but no specifier is also incomplete. + tm t{}; + istringstream iss("%E"); + iss >> get_time(&t, "%E"); + assert(iss.fail()); + } + + { + // Literal match is case-insensitive. + tm t{}; + istringstream iss("aBc"); + iss >> get_time(&t, "AbC"); + assert(iss); } { @@ -532,7 +548,7 @@ void test_990695() { { // strptime specification: "leading zeros are permitted but not required" tm t{}; - stringstream{" 7 4"} >> get_time(&t, "%m%d"); + istringstream{" 7 4"} >> get_time(&t, "%m%d"); assert(t.tm_mon == 6); assert(t.tm_mday == 4); } From cc674438e60802a8e71be1d683999f0632772a40 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Tue, 2 Mar 2021 01:28:29 -0800 Subject: [PATCH 3/4] Use tolower for case-insensitive comparisons. --- stl/inc/xloctime | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index f6c4b1ad213..1ae9659e05e 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -106,7 +106,7 @@ public: while (_First != _Last && _Ctype_fac.is(_Ctype::space, *_First)) { ++_First; } - } else if (_Ctype_fac.toupper(*_First) != _Ctype_fac.toupper(*_Fmtfirst)) { // bad literal match + } else if (_Ctype_fac.tolower(*_First) != _Ctype_fac.tolower(*_Fmtfirst)) { // bad literal match _State |= ios_base::failbit; break; } else { From 7f065ba960e50857e499fae8b9fae99f84bdddf1 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Tue, 2 Mar 2021 01:33:21 -0800 Subject: [PATCH 4/4] Cite N4878 consistently. --- stl/inc/xloctime | 10 +++++----- tests/std/tests/Dev11_0836436_get_time/test.cpp | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/stl/inc/xloctime b/stl/inc/xloctime index 1ae9659e05e..b29d17822bc 100644 --- a/stl/inc/xloctime +++ b/stl/inc/xloctime @@ -94,11 +94,11 @@ public: for (; _Fmtfirst != _Fmtlast; ++_Fmtfirst) { if (_State != ios_base::goodbit) { - // [locale.time.get.members] p8.2 + // N4878 [locale.time.get.members]/8.2 // _State is fail, eof, or bad. Do not proceed to the next fields. Return with current _State. break; } else if (_First == _Last) { - // p8.3 + // N4878 [locale.time.get.members]/8.3 _State = ios_base::eofbit | ios_base::failbit; break; } else if (_Ctype_fac.narrow(*_Fmtfirst) != '%') { // match literal element @@ -113,9 +113,9 @@ public: ++_First; } } else if (++_Fmtfirst == _Fmtlast) { - // p8.4: "If the number of elements in the range [fmt, fmtend) is not sufficient to unambiguously - // determine whether the conversion specification is complete and valid, the function evaluates err = - // ios_base::failbit." + // N4878 [locale.time.get.members]/8.4: "If the number of elements in the range [fmt, fmtend) is not + // sufficient to unambiguously determine whether the conversion specification is complete and valid, + // the function evaluates err = ios_base::failbit." _State = ios_base::failbit; break; } else { // get specifier after % diff --git a/tests/std/tests/Dev11_0836436_get_time/test.cpp b/tests/std/tests/Dev11_0836436_get_time/test.cpp index 07f2abd6f15..d802f7db9ec 100644 --- a/tests/std/tests/Dev11_0836436_get_time/test.cpp +++ b/tests/std/tests/Dev11_0836436_get_time/test.cpp @@ -496,7 +496,7 @@ void test_990695() { } { - // Should fail if EOF while not parsing specifier ([locale.time.get.members]/8.4). + // Should fail if EOF while not parsing specifier (N4878 [locale.time.get.members]/8.4). tm t{}; istringstream iss("4"); iss >> get_time(&t, "42"); @@ -504,7 +504,7 @@ void test_990695() { } { - // Trailing % should not be treated as a literal (p8.4 again). + // Trailing % should not be treated as a literal (N4878 [locale.time.get.members]/8.4 again). tm t{}; istringstream iss("%"); iss >> get_time(&t, "%");