Skip to content

Commit

Permalink
Utility: handle Path::{configuration,home}Directory() failures on Win…
Browse files Browse the repository at this point in the history
…dows.

These can happen on system accounts without a home folder, for example.
Seems like I didn't read the documentation properly when I made this
into an assert in e1e813e.

Original code had a *silly* check for the appdata path being empty, and
since e1e813e failing in that case, but
the check dates back to e1e813e from
2011, and unfortunately I don't remember what was the logic behind it
back then. Windows XP compatibility or some such? Instead, it's now
morphed into checking the result of SHGetFolderPathW(). Hopefully that
won't cause some other regression again.
  • Loading branch information
mosra committed May 27, 2022
1 parent c5102b3 commit 94f841e
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 11 deletions.
23 changes: 15 additions & 8 deletions src/Corrade/Utility/Path.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -595,9 +595,15 @@ Containers::Optional<Containers::String> homeDirectory() {
#elif defined(CORRADE_TARGET_WINDOWS) && !defined(CORRADE_TARGET_WINDOWS_RT)
/** @todo get rid of MAX_PATH */
wchar_t h[MAX_PATH + 1];
/* There doesn't seem to be any possibility how this could fail, so just
assert */
CORRADE_INTERNAL_ASSERT(SHGetFolderPathW(nullptr, CSIDL_PERSONAL, nullptr, 0, h) == S_OK);
/* This could fail for example with E_INVALIDARG for system accounts
without a home folder */
if(SHGetFolderPathW(nullptr, CSIDL_PERSONAL, nullptr, 0, h) != S_OK) {
Error err;
err << "Utility::Path::homeDirectory(): can't retrieve CSIDL_PERSONAL:";
Utility::Implementation::printWindowsErrorString(err, GetLastError());
return {};
}

return fromNativeSeparators(Unicode::narrow(h));

/* Other */
Expand Down Expand Up @@ -641,11 +647,12 @@ Containers::Optional<Containers::String> configurationDirectory(const Containers
#elif defined(CORRADE_TARGET_WINDOWS) && !defined(CORRADE_TARGET_WINDOWS_RT)
/** @todo get rid of MAX_PATH */
wchar_t path[MAX_PATH];
/* There doesn't seem to be any possibility how this could fail, so just
assert */
CORRADE_INTERNAL_ASSERT(SHGetFolderPathW(nullptr, CSIDL_APPDATA, nullptr, 0, path) == S_OK);
if(path[0] == L'\0') {
Error{} << "Utility::Path::configurationDirectory(): can't retrieve CSIDL_APPDATA";
/* This could fail for example with E_INVALIDARG for system accounts
without a home folder */
if(SHGetFolderPathW(nullptr, CSIDL_APPDATA, nullptr, 0, path) != S_OK) {
Error err;
err << "Utility::Path::configurationDirectory(): can't retrieve CSIDL_APPDATA:";
Utility::Implementation::printWindowsErrorString(err, GetLastError());
return {};
}
return join(fromNativeSeparators(Unicode::narrow(path)), applicationName);
Expand Down
38 changes: 35 additions & 3 deletions src/Corrade/Utility/Test/PathTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1530,8 +1530,24 @@ void PathTest::homeDirectory() {
}

void PathTest::homeDirectoryInvalid() {
/* Could be tested by temporarily removing $HOME, but ... ahem */
#ifdef CORRADE_TARGET_WINDOWS
/* The query fails for system accounts, and system accounts apparently have
no access to environment, so checking if %HOMEPATH% is missing:
https://serverfault.com/questions/292040/win-service-running-under-localservice-account-cannot-access-environment-variabl */
if(std::getenv("HOMEPATH"))
CORRADE_SKIP("%HOMEPATH% exists, can't test.");

std::ostringstream out;
Error redirectError{&out};
CORRADE_VERIFY(!Path::homeDirectory());
CORRADE_COMPARE_AS(out.str(),
"Utility::Path::homeDirectory(): can't retrieve CSIDL_PERSONAL: error 666 (",
TestSuite::Compare::StringHasPrefix);

#else
/* On Unix could be tested by temporarily removing $HOME, but ... ahem */
CORRADE_SKIP("Not sure how to test this.");
#endif
}

void PathTest::homeDirectoryUtf8() {
Expand Down Expand Up @@ -1601,9 +1617,25 @@ void PathTest::configurationDirectory() {
}

void PathTest::configurationDirectoryInvalid() {
/* Could be tested by temporarily removing $XDG_CONFIG_HOME and $HOME, but
... ahem */
#ifdef CORRADE_TARGET_WINDOWS
/* The query fails for system accounts, and system accounts apparently have
no access to environment, so checking if %HOMEPATH% is missing:
https://serverfault.com/questions/292040/win-service-running-under-localservice-account-cannot-access-environment-variabl */
if(std::getenv("APPDATA"))
CORRADE_SKIP("%APPDATA% exists, can't test.");

std::ostringstream out;
Error redirectError{&out};
CORRADE_VERIFY(!Path::configurationDirectory("Corrade"));
CORRADE_COMPARE_AS(out.str(),
"Utility::Path::configurationDirectory(): can't retrieve CSIDL_APPDATA: error 666 (",
TestSuite::Compare::StringHasPrefix);

#else
/* On Unix could be tested by temporarily removing $XDG_CONFIG_HOME and
$HOME, but ... ahem */
CORRADE_SKIP("Not sure how to test this.");
#endif
}

void PathTest::configurationDirectoryUtf8() {
Expand Down

0 comments on commit 94f841e

Please # to comment.