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 filesystem::weakly_canonical() on Win11 24H2 #4844

Merged
merged 1 commit into from
Jul 21, 2024

Conversation

StephanTLavavej
Copy link
Member

We have a regression test for DevCom-538510 (internal VSO-850856) "std::filesystem::weakly_canonical erroneously throws an exception":

// Also test VSO-850856
// "std::filesystem::weakly_canonical should not throw an exception when a prefix is an existing file"
const path fileWithSuffix = meow_txt.native() + L"/";
const path weaklyCanonicalFileWithSuffix = weakly_canonical(fileWithSuffix); // this should not throw
assert(weaklyCanonicalFileWithSuffix.native().back() == L'\\');

This has regressed on the upcoming Win11 24H2, where we throw an exception (again). The relevant code is:

STL/stl/inc/filesystem

Lines 4127 to 4144 in ecbc1ef

_EXPORT_STD _NODISCARD inline path weakly_canonical(const path& _Input, error_code& _Ec) {
// eventually calls GetFinalPathNameByHandleW
_Ec.clear(); // for exception safety
path _Temp;
{
const auto _Err = _Canonical(_Temp, _Input.native());
if (_Err == __std_win_error::_Success) {
return _Temp;
}
if (!__std_is_file_not_found(_Err)) {
_Ec = _Make_ec(_Err);
return {};
}
}

After calling _Canonical(), anything other than _Err == __std_win_error::_Success or __std_is_file_not_found(_Err) is considered a hard error (which will cause the non-error_code overload to throw a filesystem_error).

In current OSes, the fileWithSuffix scenario results in _Invalid_name here. (Locally verified on Win11 23H2. Presumably we get the same behavior in the GitHub PR/CI system running Server 2022 21H2, although I didn't verify that.)

In Win11 24H2, we get _Directory_name_is_invalid, which isn't recognized by __std_is_file_not_found. 💥

The fix is backwards-compatible and a simple one-liner: add this error code enumerator to the list of synonyms that __std_is_file_not_found recognizes. The plain meaning of _Directory_name_is_invalid is clearly a subset of _Invalid_name.

I've manually verified this (with significant but perhaps not surprising logistical difficulty) in an Azure VM with Win11 24H2. Regular test coverage will be provided by the MSVC-internal test harness (which is how we found this), and eventually by the GitHub test harness once Server 2025 24H2 is officially released.

Additionally, this uncaught exception from a non-error_code overload manifested itself as an abort() with no useful logs. To aid in future investigations, I'm including a simple change to the filesystem test. Now, we wrap the whole test in try ... catch and print the contents of any exception before failing.

@StephanTLavavej StephanTLavavej added bug Something isn't working high priority Important! filesystem C++17 filesystem labels Jul 19, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner July 19, 2024 23:58
@StephanTLavavej
Copy link
Member Author

Overriding policy as @zacklj89 approved the internal mirror MSVC-PR-566203.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working filesystem C++17 filesystem high priority Important!
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant