<xloctime>: time_put::do_put does not handle EINVAL from _Wcsftime

[futuarmo]

This PR fixes issue #1461

Error comes from _Strftime and _Wcsftime, _Count becomes 0 and we should stop execution and set ios_base::badbit if _Count == 0.

virtual _OutIt __CLR_OR_THIS_CALL do_put(_OutIt _Dest, ios_base&, _Elem, const tm* _Pt, char _Specifier,
        char _Modifier = '\0') const { // put formatted time from _Pt to _Dest for [_Fmtfirst, _Fmtlast)
        char _Fmt[5] = "!%x\0"; // '!' for nonzero count, null for modifier
        size_t _Count;
        size_t _Num;
        string _Str;
        if (_Modifier == '\0') {
            _Fmt[2] = _Specifier;
        } else { // add both modifier and specifier
            _Fmt[2] = _Modifier;
            _Fmt[3] = _Specifier;
        }

        for (_Num = 16;; _Num *= 2) { // convert into ever larger string buffer until success
            _Str.append(_Num, '\0');
            if (0 < (_Count = _Strftime(&_Str[0], _Str.size(), _Fmt, _Pt, _Tnames._Getptr()))) {
                break;
            }
        }

        return _STD copy(&_Str[1], &_Str[_Count], _Dest);
    }

do_put is called after operator<<

template <class _Elem, class _Traits, class _Elem2>
basic_ostream<_Elem, _Traits>& operator<<(basic_ostream<_Elem, _Traits>& _Ostr,
    const _Timeobj<_Elem2, const tm*>& _Manip) { // put time information to output stream
    using _Myos   = basic_ostream<_Elem, _Traits>;
    using _Iter   = ostreambuf_iterator<_Elem, _Traits>;
    using _Mytput = time_put<_Elem2, _Iter>;

    static_assert(is_same_v<_Elem, _Elem2>, "wrong character type for put_time");

    ios_base::iostate _State = ios_base::goodbit;
    const typename _Myos::sentry _Ok(_Ostr);

    if (_Ok) { // state okay, insert monetary amount
        const _Mytput& _Tput_fac = _STD use_facet<_Mytput>(_Ostr.getloc());
        _TRY_IO_BEGIN
        if (_Tput_fac.put(_Iter(_Ostr.rdbuf()), _Ostr, _Ostr.fill(), _Manip._Tptr, _Manip._Fmtfirst, _Manip._Fmtlast)
                .failed()) {
            _State |= ios_base::badbit;
        }
        _CATCH_IO_(ios_base, _Ostr)
    }

    _Ostr.setstate(_State);
    return _Ostr;
}

So, we can change _Ostr state only here according to standard function definition
I tried to pass _State as a parameter to put method and it works perfectly, but it contrary to standard and, as I understand, it will be ABI break.
Also I tried to invalidate iterator in do_put method, it works with wstringstream, but doesn't works with sstringstream:
do_put method part:

for (_Num = 16;; _Num *= 2) { // convert into ever larger string buffer until success
            _Str.append(_Num, '\0');
            _Count = _Strftime(&_Str[0], _Str.size(), _Fmt, _Pt, _Tnames._Getptr());
            if (0 < _Count) {
                break;
            } else if (0 == _Count) {
                *_Dest = _Traits::to_char_type(_Traits::eof());
                return _Dest;
            }
        }

I realize that it's wrong decision.

So, in my opinion the only way to fix it is to do something like that (it works too):
operator<< in iomanip header

_Ostr.setstate(ios_base::goodbit);
    if (_Ok) { // state okay, insert monetary amount
        const _Mytput& _Tput_fac = _STD use_facet<_Mytput>(_Ostr.getloc());
        _TRY_IO_BEGIN
        if (_Tput_fac.put(_Iter(_Ostr.rdbuf()), _Ostr, _Ostr.fill(), _Manip._Tptr, _Manip._Fmtfirst, _Manip._Fmtlast)
                .failed()) {
            _Ostr.setstate(ios_base::badbit);
        }
        _CATCH_IO_(ios_base, _Ostr)
    }

    //_Ostr.setstate(_State);
    return _Ostr;

And then set state to badbit in case if ucrt returns error (0):
Modified part of do_put:

for (_Num = 16;; _Num *= 2) { // convert into ever larger string buffer until success
            _Str.append(_Num, '\0');
            _Count = _Strftime(&_Str[0], _Str.size(), _Fmt, _Pt, _Tnames._Getptr());
            if (0 < _Count) {
                break;
            } else if (0 == _Count) {
                _Iosbase.setstate(ios_base::badbit);
                return _Dest;
            }
        }

But this decision is not so elegant. Please review it and give some another opinion on this
Also, I added test for this case, but ucrt returns and error, tests become failed and error message appears. I tried to set _set_invalid_parameter_handler to NULL and empty function but it doesn't help. Please, help to avoid it.

[MattStephanson]

You need to set the invalid parameter handler to an actual function, not just NULL, like in @amyw-msft's original submission (although in her function the %d format should be %u, to avoid a signed/unsigned mismatch error).

[futuarmo]

You need to set the invalid parameter handler to an actual function, not just NULL, like in @amyw-msft's original submission (although in her function the %d format should be %u, to avoid a signed/unsigned mismatch error).

Thank you for advice , I tried it, but it also makes tests failed

[MattStephanson]

I added the following handler, and tests/Dev11_0836436_get_time passed. Can you push the exact code you tried?

void test_invalid_parameter_handler(const wchar_t* const expression, const wchar_t* const function,
    const wchar_t* const file, const unsigned int line, const uintptr_t reserved) {
    (void) expression;
    (void) reserved;

    // Stop test early. Without this,
    static int num_called = 0;
    if (num_called++ > 10) {
        wprintf(L"Test Failed: Invalid parameter handler was called over 10 times by %s in %s:%u\n", function, file,
            line); // These arguments are only populated in debug mode.
        exit(1);
    }
}

[futuarmo]

I added the following handler, and tests/Dev11_0836436_get_time passed. Can you push the exact code you tried?

void test_invalid_parameter_handler(const wchar_t* const expression, const wchar_t* const function,
    const wchar_t* const file, const unsigned int line, const uintptr_t reserved) {
    (void) expression;
    (void) reserved;

    // Stop test early. Without this,
    static int num_called = 0;
    if (num_called++ > 10) {
        wprintf(L"Test Failed: Invalid parameter handler was called over 10 times by %s in %s:%u\n", function, file,
            line); // These arguments are only populated in debug mode.
        exit(1);
    }
}

Great! I'll try it later and if everything will be ok, there won't be any need to push my variant

[CaseyCarter]

Thanks for this bugfix that we took forever to review @futuarmo. Please let us know if you won't have time available to make the requested changes, and we'll polish up the PR and get it across the finish line.

[StephanTLavavej]

FYI @amyw-msft @CaseyCarter, I pushed minor changes after you approved.

[futuarmo]

@StephanTLavavej, thanks for fixes! I haven't time to do it in this several days

[StephanTLavavej]

I had to push an additional change to fix the internal test pass: /clr:pure doesn't have _set_invalid_parameter_handler so we need to skip that part of the test.

[StephanTLavavej]

Thanks for fixing this issue! 😸 🎉 🗓️