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

mkitzan · 2020-01-13T05:05:50Z

Summary of the Pull Request

WT crashes when an unparseable/invalid backgroundImage resource path is provided in profiles.json. This PR averts the crash by the validating and correcting backgroundImage 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 resource path is detected this warning screen is presented:

References

#4002 which identified a consistent repro for the crash

PR Checklist

Closes Windows Terminal crashes when a profile's icon URL or background is unparseable/invalid #2329
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: Windows Terminal crashes when a profile's icon URL or background is unparseable/invalid #2329

Detailed Description of the Pull Request / Additional comments

To validate the resource, the 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.

Validation Steps Performed

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).

zadjii-msft

This seems reasonable to me, but for some reason I have it in my head that the background image could also be a URL to an image on the web. Was that scenario ever possible, and does it still work with this?

I know there are some people who are also using ms-appdata: paths as their background image, does that still work as well?

mkitzan · 2020-01-13T18:16:54Z

Good catch, URLs and ms-appdata/ms-appx paths fail under this scheme. I'll add a check to validate the image path as an URI.

This seems like a good method of validating resource paths.

mkitzan · 2020-01-13T19:02:27Z

@zadjii-msft, I found a pretty clean way of validating resource paths. By constructing a Windows::Foundation::Uri object with the path, the ctor will throw if the path is invalid.

(the build failure seems to be due to some anomalous behavior unrelated to this PR)

carlos-zamora

Just a minor issue. Other than that, I think it's good. Thanks!

carlos-zamora · 2020-01-13T23:47:49Z

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

+  <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>


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

mkitzan · 2020-01-14T00:51:18Z

@carlos-zamora, I removed the weird auto-formatting changes on the Resources.resw.

miniksa · 2020-01-14T17:17:21Z

Nnnnn this crash also happens with the icons for profiles if they're not found too. Can we check that one too or somehow make this check more generic?

mkitzan · 2020-01-14T18:48:13Z

@miniksa, I originally didn't validate/correct icons because it appears that it no longer repros. However, might as well validate/correct invalid icon image paths (the check is painless). The latest commit performs the same checks to icons as background images, and displays this error message:

miniksa · 2020-01-14T21:19:43Z

@miniksa Michael Niksa FTE, I originally didn't validate/correct icons because it appears that it no longer repros. However, might as well validate/correct invalid icon image paths (the check is painless). The latest commit performs the same checks to icons as background images, and displays this error message:

Whoa, ok. Something must have changed since a month or two ago when I got crashes for missing icons. Either way, thanks for covering it.

miniksa

Looks good to me.

DHowett-MSFT

Thanks so much!

It looks like your review comment has been addressed.

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 (cherry picked from commit 77dd51a)

ghost · 2020-01-27T18:49:13Z

🎉Windows Terminal Preview v0.8.10261.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2020-02-13T18:01:38Z

🎉Windows Terminal Preview v0.9.433.0 has been released which incorporates this pull request.:tada:

Handy links:

mkitzan added 2 commits January 12, 2020 19:46

Initial fix to bad background image crash

71cd42c

Small change to function name

20ca96f

mkitzan changed the title ~~Fix crash related to unparseable/invalid background image~~ Fix crash related to unparseable/invalid background image file path Jan 13, 2020

zadjii-msft reviewed Jan 13, 2020

View reviewed changes

zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Jan 13, 2020

Validates URI paths

bc5215e

This seems like a good method of validating resource paths.

carlos-zamora previously requested changes Jan 13, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 13, 2020

Removed autoformatting changes

109868c

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 14, 2020

Validate icon paths

ea3858c

mkitzan changed the title ~~Fix crash related to unparseable/invalid background image file path~~ Fix crash related to unparseable/invalid media resource paths Jan 14, 2020

Remove trailing space in comment

7bff233

miniksa approved these changes Jan 14, 2020

View reviewed changes

DHowett-MSFT approved these changes Jan 17, 2020

View reviewed changes

DHowett-MSFT merged commit 77dd51a into microsoft:master Jan 17, 2020

mkitzan deleted the validate-background-image branch January 17, 2020 02:58

ghost mentioned this pull request Jan 27, 2020

Windows Terminal crashes when a profile's icon URL or background is unparseable/invalid #2329

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mkitzan commented Jan 13, 2020 •

edited

Loading

zadjii-msft left a comment

mkitzan commented Jan 13, 2020

mkitzan commented Jan 13, 2020 •

edited

Loading

carlos-zamora left a comment

carlos-zamora Jan 13, 2020

mkitzan commented Jan 14, 2020 •

edited

Loading

miniksa commented Jan 14, 2020

mkitzan commented Jan 14, 2020

miniksa commented Jan 14, 2020

miniksa left a comment

DHowett-MSFT left a comment

ghost commented Jan 27, 2020

ghost commented Feb 13, 2020

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

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

Conversation

mkitzan commented Jan 13, 2020 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

mkitzan commented Jan 13, 2020

mkitzan commented Jan 13, 2020 • edited Loading

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora Jan 13, 2020

Choose a reason for hiding this comment

mkitzan commented Jan 14, 2020 • edited Loading

miniksa commented Jan 14, 2020

mkitzan commented Jan 14, 2020

miniksa commented Jan 14, 2020

miniksa left a comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

ghost commented Jan 27, 2020

ghost commented Feb 13, 2020

mkitzan commented Jan 13, 2020 •

edited

Loading

mkitzan commented Jan 13, 2020 •

edited

Loading

mkitzan commented Jan 14, 2020 •

edited

Loading