microsoft / STL

MSVC's implementation of the C++ Standard Library.
Other
10.15k stars 1.5k forks source link

`<chrono>`: `format("%c", time_point)` can lead to crash #3764

Closed achabense closed 1 year ago

achabense commented 1 year ago

This is a follow-up of #924. The same crash can happen even with format, as it will result in a call to put_time(which will make call to functions like _Strftime):

#include<format>
#include<iostream>
#include<chrono>
int main() {
    using namespace std::chrono;
    try {
        sys_time<seconds> t{seconds(335303598060)};
        std::cout << std::format("{:%c}", t);
    }
    catch (std::exception& e) {
        std::cout << e.what();// unreacheable.
    }
    return 0;
}

https://github.com/microsoft/STL/blob/c8d1efb6d504f6392acf8f6d01fd703f7c8826c0/stl/inc/chrono#L5470 Instead of an exception, the program will give a crashing error:

Unhandled exception at 0x00007FF80ACA1208 (ucrtbase.dll) in test.exe:
An invalid parameter was passed to a function that considers invalid parameters fatal.

Please consider doing some investigations for strftime series. The crash-on-failed-check behavior interacts really poorly with C++.

achabense commented 1 year ago

Here is a rough test for strftime. The lines that are not commented-out are exceptions that will not crash the program.

Code to test int test() { tm t{ -1,-1,-1,-1,-1,-1,-1,-1,-1 }; char buffer[100]; // Year strftime(buffer, 100, "%Y", &t); strftime(buffer, 100, "%EY", &t); strftime(buffer, 100, "%y", &t); //strftime(buffer, 100, "%0y", &t);//Shows "Expression: false" in debug mode. strftime(buffer, 100, "%Ey", &t); strftime(buffer, 100, "%C", &t); strftime(buffer, 100, "%EC", &t); strftime(buffer, 100, "%G", &t); strftime(buffer, 100, "%g", &t); // Month //strftime(buffer, 100, "%b", &t); //strftime(buffer, 100, "%h", &t); //strftime(buffer, 100, "%B", &t); //strftime(buffer, 100, "%m", &t); //strftime(buffer, 100, "%0m", &t); // Week //strftime(buffer, 100, "%U", &t); //strftime(buffer, 100, "%0U", &t); //strftime(buffer, 100, "%W", &t); //strftime(buffer, 100, "%0W", &t); strftime(buffer, 100, "%V", &t); //strftime(buffer, 100, "%0V", &t); // Day of the year/month //strftime(buffer, 100, "%j", &t); //strftime(buffer, 100, "%d", &t); //strftime(buffer, 100, "%0d", &t); //strftime(buffer, 100, "%e", &t); //strftime(buffer, 100, "%0e", &t); // Day of the week //strftime(buffer, 100, "%a", &t); //strftime(buffer, 100, "%A", &t); //strftime(buffer, 100, "%w", &t); //strftime(buffer, 100, "%0w", &t); //strftime(buffer, 100, "%u", &t); //strftime(buffer, 100, "%0u", &t); // Hour, minute, second //strftime(buffer, 100, "%H", &t); //strftime(buffer, 100, "%0H", &t); //strftime(buffer, 100, "%I", &t); //strftime(buffer, 100, "%0I", &t); //strftime(buffer, 100, "%M", &t); //strftime(buffer, 100, "%0M", &t); //strftime(buffer, 100, "%S", &t); //strftime(buffer, 100, "%0S", &t); // Other //strftime(buffer, 100, "%c", &t); //strftime(buffer, 100, "%Ec", &t); //strftime(buffer, 100, "%x", &t); //strftime(buffer, 100, "%Ex", &t); //strftime(buffer, 100, "%X", &t); //strftime(buffer, 100, "%EX", &t); //strftime(buffer, 100, "%D", &t); //strftime(buffer, 100, "%F", &t); //strftime(buffer, 100, "%r", &t); //strftime(buffer, 100, "%R", &t); //strftime(buffer, 100, "%T", &t); //strftime(buffer, 100, "%p", &t); strftime(buffer, 100, "%z", &t); strftime(buffer, 100, "%Z", &t); return 0; }
StephanTLavavej commented 1 year ago

Thanks for the test cases. The first thing we need to understand is whether this is a bug in the UCRT strftime implementation according to its documented behavior, in which case we need to report it to the UCRT maintainers.

MattStephanson commented 1 year ago
sys_time<seconds> t{seconds(335303598060)};
std::cout << std::format("{:%c}", t);

This is a time in the year 12595. The UCRT only considers the years [0, 9999] valid, see Windows Kits\10\Source\10.0.22000.0\ucrt\time\wcsftime.cpp, line 963. POSIX.1-2017 adds "The %Y conversion specification to strftime() was frequently assumed to be a four-digit year, but the ISO C standard does not specify that %Y is restricted to any subset of allowed values from the tm_year field". So, this part appears to be a UCRT bug.

tm t{ -1,-1,-1,-1,-1,-1,-1,-1,-1 };
...

All of these tm fields are invalid except for tm_year, so I'm not sure what you're demonstrating here. "%0y", however, doesn't work because the UCRT only claims to conform to C99 and a subset of POSIX.1-1996, neither of which recognize the 0 flag. Edit: I see your point is that you think these should throw instead of crashing, although I don't see anything in locale.time.put that specifies what the error handling should be.

achabense commented 1 year ago

The main problem is that in format, there has been a lot of effort to get rid of the crash behavior posed by put_time/strftime. I think we hope to completely avoid a direct crash for format. As to the test for strftime, it's a copy of the list in ref. I admit that it is of little value except for the strftime(buffer, 100, "%V", &t); case(I wasn't aware that year can be -1 and "%0*"s are not supported here). Yes, I think for strftime, a crash is too much for a failed time assumption, especially as there are a lot of other functions relying on it. @MattStephanson Thanks for pointing out where I can find the source file 👀.

achabense commented 1 year ago

Unlike "%U/W" cases there is no similar check for "%V" case. image

image

achabense commented 1 year ago

Below is a more exhaustive list that shows all specifiers that crash on sys_time<seconds>{seconds(335303598060)} due to usage of put_time. Aside from those commented-out cases, a lot of successful cases are actually tackled by _Custom_write instead of put_time. I'm temporarily closing this issue, as though I think it's highly undesirable behavior for std::format to uncatchably crash on these cases, it seems that this is still conformant to the standard. (I will reopen it if I find something that indicates this violates the standard)

#include<format>
#include<iostream>
#include<chrono>
#include<ranges>

//lists from https://en.cppreference.com/w/cpp/chrono/system_clock/formatter

// invalid by the standard
const auto specs_invalid = {
    "Q","q"
};

const auto specs = {
    "C","y","Y","b","h","B","m","d","e","a","A","u","w",
    //"g", // crash due to put_time
    //"G", // crash due to put_time
    "V","j","U","W","D","F",
    //"x", // crash due to put_time
    "H","I","M","S","p","R","T","r","X","z","Z",
    //"c", // crash due to put_time
};

// O
const auto specs_O = {
    //"Oy", // crash due to put_time
    "Om","Od","Oe","Oe","Ow","OV","OU","OW","OH","OI","OM","OS","Oz"
};

// E
const auto specs_E = {
    //"EC", // crash due to put_time
    //"Ey", // crash due to put_time
    //"EY", // crash due to put_time
    //"Ex", // crash due to put_time
    "EX","Ez",
    // "Ec", // crash due to put_time
};

int main() {
    using namespace std::chrono;
    try {
        sys_time<seconds> t{ seconds(335303598060) };
        auto fmt_arg = make_format_args(t);
        auto all_specs = { specs,specs_O,specs_E };
        for (auto str : std::views::join(all_specs)) {
            char fmt[10];
            sprintf_s(fmt, "{:%%%s}", str);
            printf("trying %s", fmt);
            auto str = std::vformat(fmt, fmt_arg);
            puts(str.c_str());
        }
    }
    catch (std::exception& e) {
        std::cout << e.what();
    }
}
TheStormN commented 2 months ago

@achabense Although you've closed the issue it seems I've addressed it in #4840 and #4883 (the second PR is still in development). Tried the code from your last comment locally and it does not crash anymore.

Update: Fixed!