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

Prevent crash when attempting to select an out-of-bounds UIA text range #7504

Merged
1 commit merged into from
Sep 3, 2020

Conversation

codeofdusk
Copy link
Contributor

@codeofdusk codeofdusk commented Sep 2, 2020

When attempting to select a text range from a different text buffer (such as a standard text range when in alt mode), conhost crashes. This PR checks for this case and returns E_FAIL instead, preventing this crash.

PR Checklist

  • Closes unfiled crash issue
  • CLA signed. If not, go over here and sign the CLA
  • Passes manual test below
  • 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: #xxx

Validation Steps Performed

Ran the following lines in the NVDA Python console (NVDA+control+z) before and after this PR, and observed that Conhost no longer crashes after the change:

>>> # SSH to a remote Linux system
>>> ti=nav.makeTextInfo("caret")
>>> ti.move("line", -2)
-2
>>> # Switch away from the NVDA Python console, and run Nano in conhost. Then:
>>> ti.updateSelection() # Calls select() on the underlying UIA text range
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "NVDAObjects\UIA\__init__.pyc", line 790, in updateSelection
  File "comtypesMonkeyPatches.pyc", line 26, in __call__
_ctypes.COMError: (-2147220991, 'An event was unable to invoke any of the subscribers', (None, None, None, 0, None))

@codeofdusk
Copy link
Contributor Author

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Good catch. Thanks!

@carlos-zamora carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Sep 3, 2020
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Beautiful. Thank you so much.

@miniksa miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 3, 2020
@ghost
Copy link

ghost commented Sep 3, 2020

Hello @miniksa!

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 c808ed9 into microsoft:master Sep 3, 2020
@codeofdusk codeofdusk deleted the dev/codeofdusk/uia-select-crash branch September 4, 2020 05:06
DHowett pushed a commit that referenced this pull request Sep 18, 2020
…ge (#7504)

When attempting to select a text range from a different text buffer (such as a standard text range when in alt mode), conhost crashes. This PR checks for this case and returns `E_FAIL` instead, preventing this crash.

## PR Checklist
* [x] Closes unfiled crash issue
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Passes manual test below
* [ ] 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: #xxx

## Validation Steps Performed
Ran the following lines in the NVDA Python console (NVDA+control+z) before and after this PR, and observed that Conhost no longer crashes after the change:

``` Python console
>>> # SSH to a remote Linux system
>>> ti=nav.makeTextInfo("caret")
>>> ti.move("line", -2)
-2
>>> # Switch away from the NVDA Python console, and run Nano in conhost. Then:
>>> ti.updateSelection() # Calls select() on the underlying UIA text range
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "NVDAObjects\UIA\__init__.pyc", line 790, in updateSelection
  File "comtypesMonkeyPatches.pyc", line 26, in __call__
_ctypes.COMError: (-2147220991, 'An event was unable to invoke any of the subscribers', (None, None, None, 0, None))
```

(cherry picked from commit c808ed9)
@ghost
Copy link

ghost commented Sep 22, 2020

🎉Windows Terminal v1.3.2651.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Sep 22, 2020

🎉Windows Terminal Preview v1.4.2652.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants