Fix crash related to unparseable/invalid media resource paths (#4194)

WT crashes when an unparseable/invalid `backgroundImage` or `icon`
resource path is provided in `profiles.json`. This PR averts the crash
by the validating and correcting resource paths as a part of the
`_ValidateSettings()` function in `CascadiaSettings`.
`_ValidateSettings()` is run on start up and any time `profiles.json` is
changed, so a user can not change a file path and avoid the validation
step. 

When a bad `backgroundImage` or `icon` resource path is detected, a
warning screen will be presented.

References #4002, which identified a consistent repro for the crash.

To validate the resource, a `Windows::Foundation::Uri` object is
constructed with the path. The ctor will throw if the resource path is
invalid. Whether or not this validation method is robust enough is a
subject worth review. The correction method for when a bad resource path
is detected is to reset the `std::optional<winrt::hstring>` holding the
file path. 

The text in the warning display was cribbed from the text used when an
invalid `colorScheme` is used. Whether or not the case of a bad
background image file path warrants a warning display is a subject worth
review.

Ensured the repro steps in #4002 did not trigger a crash. Additionally,
some potential backdoor paths to a crash were tested: 

- Deleting the file of a validated background image file path
- Changing the actual file name of a validated background image file
  path
- Replacing the file of a validated background image file path with a
  non-image file (of the same name)
- Using a non-image file as a background image

In all the above cases WT does not crash, and instead defaults to the
background color specified in the profile's `colorScheme`. This PR does
not implement this recovery behavior (existing error catching code
does).

Closes #2329
This commit is contained in:
Michael Kitzan 2020-01-16 17:48:37 -08:00 committed by Dustin L. Howett (MSFT)
parent 0586955c88
commit 77dd51af39
7 changed files with 94 additions and 3 deletions

View file

@ -27,10 +27,12 @@ namespace winrt
// !!! IMPORTANT !!!
// Make sure that these keys are in the same order as the
// SettingsLoadWarnings/Errors enum is!
static const std::array<std::wstring_view, 3> settingsLoadWarningsLabels {
static const std::array<std::wstring_view, 5> settingsLoadWarningsLabels {
USES_RESOURCE(L"MissingDefaultProfileText"),
USES_RESOURCE(L"DuplicateProfileText"),
USES_RESOURCE(L"UnknownColorSchemeText")
USES_RESOURCE(L"UnknownColorSchemeText"),
USES_RESOURCE(L"InvalidBackgroundImage"),
USES_RESOURCE(L"InvalidIcon")
};
static const std::array<std::wstring_view, 2> settingsLoadErrorsLabels {
USES_RESOURCE(L"NoProfilesText"),

View file

@ -198,6 +198,10 @@ void CascadiaSettings::_ValidateSettings()
// just use the hardcoded defaults
_ValidateAllSchemesExist();
// Ensure all profile's with specified images resources have valid file path.
// This validates icons and background images.
_ValidateMediaResources();
// TODO:GH#2548 ensure there's at least one key bound. Display a warning if
// there's _NO_ keys bound to any actions. That's highly irregular, and
// likely an indication of an error somehow.
@ -424,6 +428,64 @@ void CascadiaSettings::_ValidateAllSchemesExist()
}
}
// Method Description:
// - Ensures that all specified images resources (icons and background images) are valid URIs.
// This does not verify that the icon or background image files are encoded as an image.
// Arguments:
// - <none>
// Return Value:
// - <none>
// - Appends a SettingsLoadWarnings::InvalidBackgroundImage to our list of warnings if
// we find any invalid background images.
// - Appends a SettingsLoadWarnings::InvalidIconImage to our list of warnings if
// we find any invalid icon images.
void CascadiaSettings::_ValidateMediaResources()
{
bool invalidBackground{ false };
bool invalidIcon{ false };
for (auto& profile : _profiles)
{
if (profile.HasBackgroundImage())
{
// Attempt to convert the path to a URI, the ctor will throw if it's invalid/unparseable.
// This covers file paths on the machine, app data, URLs, and other resource paths.
try
{
winrt::Windows::Foundation::Uri imagePath{ profile.GetExpandedBackgroundImagePath() };
}
catch (...)
{
profile.ResetBackgroundImagePath();
invalidBackground = true;
}
}
if (profile.HasIcon())
{
try
{
winrt::Windows::Foundation::Uri imagePath{ profile.GetExpandedIconPath() };
}
catch (...)
{
profile.ResetIconPath();
invalidIcon = true;
}
}
}
if (invalidBackground)
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::InvalidBackgroundImage);
}
if (invalidIcon)
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::InvalidIcon);
}
}
// Method Description:
// - Create a TerminalSettings object for the provided newTerminalArgs. We'll
// use the newTerminalArgs to look up the profile that should be used to

View file

@ -115,6 +115,7 @@ private:
void _ReorderProfilesToMatchUserSettingsOrder();
void _RemoveHiddenProfiles();
void _ValidateAllSchemesExist();
void _ValidateMediaResources();
friend class TerminalAppLocalTests::SettingsTests;
friend class TerminalAppLocalTests::ProfileTests;

View file

@ -847,6 +847,14 @@ void Profile::SetIconPath(std::wstring_view path)
_icon.emplace(path);
}
// Method Description:
// - Resets the std::optional holding the icon file path string.
// HasIcon() will return false after the execution of this function.
void Profile::ResetIconPath()
{
_icon.reset();
}
// Method Description:
// - Returns this profile's icon path, if one is set. Otherwise returns the
// empty string. This method will expand any environment variables in the
@ -880,6 +888,14 @@ winrt::hstring Profile::GetExpandedBackgroundImagePath() const
return result;
}
// Method Description:
// - Resets the std::optional holding the background image file path string.
// HasBackgroundImage() will return false after the execution of this function.
void Profile::ResetBackgroundImagePath()
{
_backgroundImage.reset();
}
// Method Description:
// - Returns the name of this profile.
// Arguments:

View file

@ -89,9 +89,11 @@ public:
bool HasIcon() const noexcept;
winrt::hstring GetExpandedIconPath() const;
void SetIconPath(std::wstring_view path);
void ResetIconPath();
bool HasBackgroundImage() const noexcept;
winrt::hstring GetExpandedBackgroundImagePath() const;
void ResetBackgroundImagePath();
CloseOnExitMode GetCloseOnExitMode() const noexcept;
bool GetSuppressApplicationTitle() const noexcept;

View file

@ -210,4 +210,10 @@ Temporarily using the Windows Terminal default settings.
<data name="CloseWindowWarningTitle" xml:space="preserve">
<value>Do you want to close all tabs?</value>
</data>
<data name="InvalidBackgroundImage" xml:space="preserve">
<value>Found a profile with an invalid "backgroundImage". Defaulting that profile to have no background image. Make sure that when setting a "backgroundImage", the value is a valid file path to an image.</value>
</data>
<data name="InvalidIcon" xml:space="preserve">
<value>Found a profile with an invalid "icon". Defaulting that profile to have no icon. Make sure that when setting an "icon", the value is a valid file path to an image.</value>
</data>
</root>

View file

@ -23,7 +23,9 @@ namespace TerminalApp
{
MissingDefaultProfile = 0,
DuplicateProfile = 1,
UnknownColorScheme = 2
UnknownColorScheme = 2,
InvalidBackgroundImage = 3,
InvalidIcon = 4
};
// SettingsLoadWarnings are scenarios where the settings had invalid state