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

UIA: Prevent crash from invalid UTR endpoint comparison #5399

Merged
2 commits merged into from
Apr 21, 2020

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Apr 17, 2020

Summary of the Pull Request

This is a quick-and-easy solution to #5309. If the ITextRangeProvider API allows us to take in two UiaTextRanges, we need to verify that they are both valid.

With this PR, we make sure they both fit in the current TextBuffer. If not, we return E_FAIL. Though this doesn't prove that both UiaTextRanges are from the same TextBuffer, at the very least we don't crash and in cases where we can't make a valid comparison, we return an HRESULT failure.

References

#5406 - This should be the proper solution to this problem. Each UiaTextRange needs to be aware of which TextBuffer it came from.

PR Checklist

Validation Steps Performed

  1. generate enough output to cause the terminal to scroll
  2. execute nano to make us go into the alternate buffer
    This previously crashed, now NVDA seems to detect that there was an error and keeps moving along.

@codeofdusk
Copy link
Contributor

@carlos-zamora I'm assuming NVDA plays an error sound when switching into the alt mode? We can fix this (by just dropping that exception) but there really should be separate UIA objects for the standard and alt buffer.

@codeofdusk
Copy link
Contributor

See #5406.

@carlos-zamora carlos-zamora added Area-Accessibility Issues related to accessibility Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Apr 20, 2020
@carlos-zamora
Copy link
Member Author

Thanks for filing that! If this is acceptable in the meantime, I'll mark it ready for review then 😊

@carlos-zamora carlos-zamora marked this pull request as ready for review April 20, 2020 17:11
@codeofdusk
Copy link
Contributor

Could you please provide a test build?

@DHowett-MSFT
Copy link
Contributor

@codeofdusk I'm working on that -- our build agents are somewhat backed up.

@DHowett-MSFT
Copy link
Contributor

Copy link
Contributor

@codeofdusk codeofdusk left a comment

Choose a reason for hiding this comment

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

Fixes the crash on my system (when attempting to use nano). NVDA-side changes implemented in nvaccess/nvda#11039.

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

k fix the XXXX comments and we'll be golden

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 20, 2020
Co-Authored-By: Bill Dengler <codeofdusk@gmail.com>
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 20, 2020
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

lgtm

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 21, 2020
@ghost
Copy link

ghost commented Apr 21, 2020

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit e3e5708 into master Apr 21, 2020
@ghost ghost deleted the dev/cazamor/acc/ch/endpoint-comparison-crash branch April 21, 2020 16:45
DHowett-MSFT pushed a commit that referenced this pull request Apr 21, 2020
Improve wide glyph support in UIA (GH-4946)
Add enhanced key support for ConPty (GH-5021)
Set DxRenderer non-text alias mode (GH-5149)
Reduce CursorChanged Events for Accessibility (GH-5196)
Add more object ID tracing for Accessibility (GH-5215)
Add SS3 cursor key encoding to ConPty (GH-5383)
UIA: Prevent crash from invalid UTR endpoint comparison (GH-5399)
Make CodepointWidthDetector::GetWidth faster (CC-3727)
add til::math, use it for float conversions to point, size (GH-5150)
Add support for renderer backoff, don't FAIL_FAST on 3x failures, add UI (GH-5353)
Fix a deadlock and a bounding rects issue in UIA (GH-5385)
Don't duplicate spaces from potentially-wrapped EOL-deferred lines (GH-5398)
Reimplement the VT tab stop functionality (CC-5173)
Clamp parameter values to a maximum of 32767. (CC-5200)
Prevent the cursor type being reset when changing the visibility (CC-5251)
Make RIS switch back to the main buffer (CC-5248)
Add support for the DSR-OS operating status report (CC-5300)
Update the virtual bottom location if the cursor moves below it (CC-5317)
ci: run spell check in CI, fix remaining issues (CC-4799) (CC-5352)
Set Cascadia Code as default font (GH-5121)
Show a double width cursor for double width characters (GH-5319)
Delegate all character input to the character event handler (CC-4192)
Update til::bitmap to use dynamic_bitset<> + libpopcnt (GH-5092)
Merged PR 4465022: [Git2Git] Merged PR 4464559: Console: Ingest OSS changes up to e055079
Correct scrolling invalidation region for tmux in pty w/ bitmap (GH-5122)
Render row-by-row instead of invalidating entire screen (GH-5185)
Make conechokey use ReadConsoleInputW by default (GH-5148)
Manually pass mouse wheel messages to TermControls (GH-5131)
This fixes C-M-space for WSL but not for Win32, but I'm not sure there's a problem in Win32 quite yet. (GH-5208)
Fix copying wrapped lines by implementing better scrolling (GH-5181)
Emit lines wrapped due to spaces at the end correctly (GH-5294)
Remove unneeded whitespace (CC-5162)
@ghost
Copy link

ghost commented May 5, 2020

🎉Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1) has been released which incorporates this pull request.:tada:

Handy links:

@codeofdusk
Copy link
Contributor

As this fixes a crash, could this please be prioritized for inclusion in the inbox console? Thanks.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Accessibility Issues related to accessibility AutoMerge Marked for automatic merge by the bot when requirements are met Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when attempting to compare selection endpoints with UIA
4 participants