Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix put_time() crash on invalid format specifier #4840

Merged
62 changes: 57 additions & 5 deletions stl/inc/xloctime
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,21 @@ 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'};

_NODISCARD constexpr bool _Is_valid_strftime_specifier(const char _Specifier) {
for (const auto& _Valid_specifier : _Valid_strftime_specifiers) {
if (_Specifier == _Valid_specifier) {
return true;
}
}

return false;
}

_EXPORT_STD extern "C++" template <class _Elem, class _OutIt = ostreambuf_iterator<_Elem, char_traits<_Elem>>>
class time_put : public locale::facet { // facet for converting encoded times to text
public:
Expand Down Expand Up @@ -687,7 +702,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_strftime_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
}
}
}

Expand Down Expand Up @@ -811,7 +838,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_strftime_specifier(_Specifier)) {
// no valid specifier, directly copy as literal elements
*_Dest++ = _Percent;
if (_Modifier != '\0') {
*_Dest++ = _Raw;
}
*_Dest++ = *_Fmtfirst;
} else {
_Dest = do_put(_Dest, _Iosbase, _Fill, _Pt, _Specifier, _Modifier); // convert a single field
}
}
}

Expand Down Expand Up @@ -927,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;
}

Expand All @@ -943,7 +983,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_strftime_specifier(_Specifier)) {
// no valid specifier, directly copy as literal elements
*_Dest++ = _Percent;
if (_Modifier != '\0') {
*_Dest++ = _Raw;
}
*_Dest++ = *_Fmtfirst;
} else {
_Dest = do_put(_Dest, _Iosbase, _Fill, _Pt, _Specifier, _Modifier); // convert a single field
}
}
}

Expand Down
43 changes: 41 additions & 2 deletions tests/std/tests/Dev11_0836436_get_time/test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -157,6 +158,7 @@ int main() {
test_buffer_resizing();
test_gh_2618();
test_gh_2848();
test_gh_4820();
}

typedef istreambuf_iterator<char> Iter;
Expand Down Expand Up @@ -792,16 +794,17 @@ void test_invalid_argument() {
time_t t = time(nullptr);
tm currentTime;
localtime_s(&currentTime, &t);
currentTime.tm_hour = 25; // set invalid hour
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved

{
wstringstream wss;
wss << put_time(&currentTime, L"%Y-%m-%d-%H-%M-%s");
wss << put_time(&currentTime, L"%Y-%m-%d-%H-%M");
assert(wss.rdstate() == ios_base::badbit);
}

{
stringstream ss;
ss << put_time(&currentTime, "%Y-%m-%d-%H-%M-%s");
ss << put_time(&currentTime, "%Y-%m-%d-%H-%M");
assert(ss.rdstate() == ios_base::badbit);
}
#endif // _M_CEE_PURE
Expand Down Expand Up @@ -905,3 +908,39 @@ void test_gh_2848() {
assert(err == (ios_base::eofbit | ios_base::failbit));
}
}

void test_gh_4820() {
// GH-4820 <iomanip>: std::put_time should copy unknown conversion specifiers instead of crash
time_t t = time(nullptr);
tm currentTime;
localtime_s(&currentTime, &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(&currentTime, L"1:%Ei%!%E%J%P 2:%% 3:% 4:%E%Z");
assert(wss.rdstate() == ios_base::goodbit);
assert(wss.str() == L"1:%Ei%!%E%J%P 2:% 3:% 4:%E%Z");
}

{
stringstream ss;
ss << put_time(&currentTime, "1:%Ei%!%E%J%P 2:%% 3:% 4:%E%Z");
assert(ss.rdstate() == ios_base::goodbit);
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(&currentTime, 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");
}
}