`<xloctime>`: `time_put::do_put` does not handle `EINVAL` from `_Wcsftime` by futuarmo · Pull Request #1563 · microsoft/STL

futuarmo · 2021-01-09T18:52:13Z

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 · 2021-01-11T02:48:23Z

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 · 2021-01-11T03:18:06Z

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 · 2021-01-11T04:59:55Z

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 · 2021-01-11T05:23:39Z

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

stl/inc/iomanip

stl/inc/xloctime

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.

tests/std/tests/Dev11_0836436_get_time/test.cpp

…sue_#1461

tests/std/tests/Dev11_0836436_get_time/test.cpp

stl/inc/xloctime

StephanTLavavej · 2021-04-29T08:36:06Z

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

futuarmo · 2021-04-29T08:39:55Z

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

StephanTLavavej · 2021-04-30T07:45:58Z

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 · 2021-04-30T23:13:33Z

Thanks for fixing this issue! 😸 🎉 🗓️

futuarmo added 4 commits January 7, 2021 23:37

set failbit on invalid format specifier

c15988f

fixed wrong format specifier handling

cc3973f

fixed typos

77063dc

fixed formatting

ea858d9

MattStephanson reviewed Jan 11, 2021

View reviewed changes

stl/inc/iomanip Outdated Show resolved Hide resolved

stl/inc/xloctime Outdated Show resolved Hide resolved

StephanTLavavej added the bug Something isn't working label Jan 12, 2021

added new tests for buffer resizing way and check for EINVAL

8ddf2cd

futuarmo marked this pull request as ready for review January 13, 2021 10:11

futuarmo requested a review from a team as a code owner January 13, 2021 10:11

StephanTLavavej added the high priority Important! label Jan 13, 2021

amyw-msft self-assigned this Jan 13, 2021

Base automatically changed from master to main January 28, 2021 00:35

StephanTLavavej removed the high priority Important! label Feb 3, 2021

Merge branch 'main' into gh1563

ff71f95

CaseyCarter self-assigned this Mar 18, 2021

CaseyCarter changed the title ~~<xloctime>: time_put::do_put does not handle EINVAL from _Wcsftime~~ <xloctime>: time_put::do_put does not handle EINVAL from _Wcsftime` Mar 31, 2021

CaseyCarter changed the title ~~<xloctime>: time_put::do_put does not handle EINVAL from _Wcsftime`~~ <xloctime>: time_put::do_put does not handle EINVAL from _Wcsftime Mar 31, 2021

CaseyCarter linked an issue Mar 31, 2021 that may be closed by this pull request

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

Closed

CaseyCarter suggested changes Mar 31, 2021

View reviewed changes

futuarmo added 5 commits April 4, 2021 17:02

added fixes according to review

6d7350e

Merge branch 'main' of https://github.com/microsoft/STL into issue_#1459

9c35ee0

Merge branch 'issue_#1460' of https://github.com/futuarmo/STL into is…

d57ca2e

…sue_#1461

fixed formatting

644f2df

fixed formatting

3eef56e

futuarmo requested a review from CaseyCarter April 6, 2021 10:10

CaseyCarter approved these changes Apr 7, 2021

View reviewed changes

CaseyCarter removed their assignment Apr 7, 2021

amyw-msft approved these changes Apr 8, 2021

View reviewed changes

CaseyCarter unassigned amyw-msft Apr 8, 2021

StephanTLavavej requested changes Apr 28, 2021

View reviewed changes

tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved

tests/std/tests/Dev11_0836436_get_time/test.cpp Outdated Show resolved Hide resolved

stl/inc/xloctime Outdated Show resolved Hide resolved

StephanTLavavej self-assigned this Apr 28, 2021

Code review feedback.

e54382a

StephanTLavavej approved these changes Apr 29, 2021

View reviewed changes

StephanTLavavej removed their assignment Apr 29, 2021

CaseyCarter approved these changes Apr 29, 2021

View reviewed changes

StephanTLavavej self-assigned this Apr 30, 2021

Fix /clr:pure.

a0cbf5a

StephanTLavavej approved these changes Apr 30, 2021

View reviewed changes

StephanTLavavej merged commit b29a701 into microsoft:main Apr 30, 2021

Conversation

futuarmo commented Jan 9, 2021

Uh oh!

MattStephanson commented Jan 11, 2021 • edited by StephanTLavavej Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

futuarmo commented Jan 11, 2021 • edited by StephanTLavavej Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattStephanson commented Jan 11, 2021

Uh oh!

futuarmo commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CaseyCarter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StephanTLavavej commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

futuarmo commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StephanTLavavej commented Apr 30, 2021

Uh oh!

StephanTLavavej commented Apr 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MattStephanson commented Jan 11, 2021 •

edited by StephanTLavavej

Loading

futuarmo commented Jan 11, 2021 •

edited by StephanTLavavej

Loading

futuarmo commented Jan 11, 2021 •

edited

Loading

StephanTLavavej commented Apr 29, 2021 •

edited

Loading

futuarmo commented Apr 29, 2021 •

edited

Loading