<iomanip>: std::get_time asserts with "istreambuf_iterator is not dereferenceable" when format is longer than the stream

[fsb4000]

Describe the bug
std::get_time doesn't stop parsing the input stream as soon as a mismatch is found or stream is eof, but continues until the format string is exhausted instead. This provokes a dereference of the input stream iterator even when it is at the eof. In debug builds it raises an assert. In release builds it is silently ignored.

Command-line test case

#include <sstream>
#include <iomanip>
#include <string>
int main()
{
    std::string s("123");
    std::tm t = {};
    std::istringstream ss(s);
    ss >> std::get_time(&t, "%Y-%m-%d %H:%M:%S");
    if (ss.fail()) {
        return 1;
    }
}

Expected behavior
Should setstate failbit in the stream instead of the assertion.

STL version
Microsoft Visual Studio Community 2019
Version 16.6.4

Additional context
See also:

• DevCom-18311, DevCom-246363, DevCom-362423, and Microsoft-internal VSO-713785 / AB#713785

[StephanTLavavej]

Yep, that's a duplicate! I'll link them up, thanks. My analysis from VSO-713785:

This is a bug. The problem is in <xloctime>:

_InIt __CLR_OR_THIS_CALL _Getfmt(_InIt _First, _InIt _Last, ios_base& _Iosbase, ios_base::iostate& _State, tm* _Pt,
    const char* _Fmtfirst) const { // get formatted time for format NTBS
    const _Ctype& _Ctype_fac = _STD use_facet<_Ctype>(_Iosbase.getloc());

    for (; *_Fmtfirst != '\0'; ++_Fmtfirst) {
        if (*_Fmtfirst == '%') {
            _First = do_get(_First, _Last, _Iosbase, _State, _Pt,
                *++_Fmtfirst); // convert a single field
        } else if (*_Fmtfirst == ' ') {
            while (_First != _Last && _Ctype_fac.is(_Ctype::space, *_First)) {
                ++_First;
            }
        } else if (_Ctype_fac.narrow(*_First) != *_Fmtfirst) { // bad literal match
            _State |= ios_base::failbit;
            break;

Here, the "bad literal match" codepath assumes that it can dereference _First, but we've reached the end of the input (it's looking for hours:minutes:seconds but there are only hours:minutes).

I'm not sure if we should also set eofbit when we encounter the end of the input here.

[Arzaghi]

@StephanTLavavej
I investigated this issue. It seems it has been resolved automatically with one of my previous PRs (#1168). The following lines prevent reproducing the issue.

STL/stl/inc/xloctime

Lines 136 to 139 in 530bdc5

	if (_State != ios_base::goodbit) {
	// Failed to convert the field. Do not proceed to the next fields. Return with failed _State.
	break;
	}

May I just add some test cases to cover this issue?

[CaseyCarter]

May I just add some test cases to cover this issue?

Yes, absolutely. If intervening changes have indirectly fixed this bug, the appropriate action is to add a regression test and close it as fixed.

[Arzaghi]

I'm not sure if we should also set eofbit when we encounter the end of the input here.

It seems Clang and GCC set eofbit in this case.

[Amaroker]

There are three DevCom issues related to this issue (see "Additional context" above). Shouldn't they be marked fixed as well?

[CaseyCarter]

There are three DevCom issues related to this issue (see "Additional context" above). Shouldn't they be marked fixed as well?

Yes they should - good catch. We don't have smart enough automation to close internal bugs when the GH bug is closed, we have to remember to either (1) mark the PR as fixing the internal bug when we port the PR, or (2) mark the internal bug as fixed when we close the GH bug. That little non-automated step is a bit of a stumbling block.

I've closed VSO-713785, which flows automatically to the DevCom bugs - they are "Fixed Pending Release".

Thanks, @Amaroker!

[fsb4000]

@CaseyCarter Maybe related:
the bug is fixed #1309
but https://developercommunity.visualstudio.com/content/problem/1190997/meow.html is still "Under Consideration"

[CaseyCarter]

@CaseyCarter Maybe related:
the bug is fixed #1309
but https://developercommunity.visualstudio.com/content/problem/1190997/meow.html is still "Under Consideration"

It's "Related" in the sense that apparently neither @StephanTLavavej nor I can properly follow the documented procedure at https://github.com/microsoft/STL/wiki/Checklist-For-Merging-A-Pull-Request. I am now highly suspicious that there are more such instances and that I should audit all the closed issues.

[CaseyCarter]

I am now highly suspicious that there are more such instances and that I should audit all the closed issues.

I was right to suspect: I found 3-4 more occurrences. Thanks again, @fsb4000!

[fsb4000]

Great! You are welcome! @CaseyCarter