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 crash related to unparseable/invalid media resource paths #4194

Merged
merged 6 commits into from
Jan 17, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/cascadia/TerminalApp/AppLogic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,11 @@ 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, 4> settingsLoadWarningsLabels {
USES_RESOURCE(L"MissingDefaultProfileText"),
USES_RESOURCE(L"DuplicateProfileText"),
USES_RESOURCE(L"UnknownColorSchemeText")
USES_RESOURCE(L"UnknownColorSchemeText"),
USES_RESOURCE(L"InvalidBackgroundImage")
};
static const std::array<std::wstring_view, 2> settingsLoadErrorsLabels {
USES_RESOURCE(L"NoProfilesText"),
Expand Down
40 changes: 40 additions & 0 deletions src/cascadia/TerminalApp/CascadiaSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,9 @@ void CascadiaSettings::_ValidateSettings()
// just use the hardcoded defaults
_ValidateAllSchemesExist();

// Ensure all profile's with specified background images have valid files
_ValidateBackgroundImages();

// 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.
Expand Down Expand Up @@ -424,6 +427,43 @@ void CascadiaSettings::_ValidateAllSchemesExist()
}
}

// Method Description:
// - Ensures that all specified background images are valid files existing on the local machine.
// This does not verify that the background image file is 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.
void CascadiaSettings::_ValidateBackgroundImages()
{
bool invalidImage{ 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();
invalidImage = true;
}
}
}

if (invalidImage)
{
_warnings.push_back(::TerminalApp::SettingsLoadWarnings::InvalidBackgroundImage);
}
}

// 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
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/CascadiaSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ class TerminalApp::CascadiaSettings final
void _ReorderProfilesToMatchUserSettingsOrder();
void _RemoveHiddenProfiles();
void _ValidateAllSchemesExist();
void _ValidateBackgroundImages();

friend class TerminalAppLocalTests::SettingsTests;
friend class TerminalAppLocalTests::ProfileTests;
Expand Down
8 changes: 8 additions & 0 deletions src/cascadia/TerminalApp/Profile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,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:
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/Profile.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ class TerminalApp::Profile final

bool HasBackgroundImage() const noexcept;
winrt::hstring GetExpandedBackgroundImagePath() const;
void ResetBackgroundImagePath();

CloseOnExitMode GetCloseOnExitMode() const noexcept;
bool GetSuppressApplicationTitle() const noexcept;
Expand Down
59 changes: 31 additions & 28 deletions src/cascadia/TerminalApp/Resources/en-US/Resources.resw
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
<?xml version="1.0" encoding="utf-8"?>
<root>
<!--
Microsoft ResX Schema

<!--
Microsoft ResX Schema
Version 2.0

The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
associated with the data types.

Example:

... ado.net/XML headers & schema ...
<resheader name="resmimetype">text/microsoft-resx</resheader>
<resheader name="version">2.0</resheader>
Expand All @@ -26,36 +26,36 @@
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
<comment>This is a comment</comment>
</data>

There are any number of "resheader" rows that contain simple
There are any number of "resheader" rows that contain simple
name/value pairs.

Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
mimetype set.

The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
extensible. For a given mimetype the value must be set accordingly:

Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
read any of the formats listed below.

mimetype: application/x-microsoft.net.object.binary.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.soap.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.bytearray.base64
value : The object must be serialized into a byte array
value : The object must be serialized into a byte array
: using a System.ComponentModel.TypeConverter
: and then encoded with base64 encoding.
-->
Expand Down Expand Up @@ -210,4 +210,7 @@ Temporarily using the Windows Terminal default settings.
<data name="CloseWindowWarningTitle" xml:space="preserve">
<value>Do you want to close all tabs?</value>
</data>
</root>
<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>
</root>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a good amount of "changes" here that actually don't exist. Please be sure to fix that.

3 changes: 2 additions & 1 deletion src/cascadia/TerminalApp/TerminalWarnings.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ namespace TerminalApp
{
MissingDefaultProfile = 0,
DuplicateProfile = 1,
UnknownColorScheme = 2
UnknownColorScheme = 2,
InvalidBackgroundImage = 3
};

// SettingsLoadWarnings are scenarios where the settings had invalid state
Expand Down