Initialize object caches in a safer manner#14333
Merged
Merged
Conversation
seanbudd
commented
Nov 4, 2022
feerrenrut
reviewed
Nov 4, 2022
See test results for failed build of commit 87a0aef2eb |
feerrenrut
reviewed
Nov 4, 2022
Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>
seanbudd
commented
Nov 4, 2022
seanbudd
commented
Nov 4, 2022
…/nvda into safer-object-cache-init
michaelDCurran
approved these changes
Nov 7, 2022
feerrenrut
approved these changes
Nov 7, 2022
feerrenrut
left a comment
Contributor
There was a problem hiding this comment.
It's worth noting that this couples isWindowsLocked() with NVDA initialization and each of:
api. setDesktopObject()
api.setForegroundObject()
api.setFocusObject()
api.setNavigatorObject()
api.setMouseObject()
The approach taken here allows for a smaller change, and lower risk in this PR.
Ideally, each code path that might be blocked by isWindowsLocked() is considered and explicitly allows an exception for initialization. I expect that when the api.setXXX functions are refactored to allow independent initialization without calling through to app modules etc. the new init versions of those methods will be the only place the exception (for not calling isWindowsLocked() is required.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to issue number:
none
Summary of the issue:
During NVDA initialization, the api module is used to set initial object caches.
These api functions check if an object is behind the lock screen when Windows is locked before setting the cache.
Until #14301, the api module initialised the object caches to the desktop object.
When NVDA started on the lock screen, it failed to set the desktop object due to the added security checks.
Now the object caches are initialised to the foreground window, to avoid leaking insecure information.
Unfortunately, setting those object caches causes side-effects, which may rely on uninitialized dependencies.
Description of user facing changes
None
Description of development approach
Revert to setting initial object caches to the desktop object.
Ensure security failures are not raised when initialising caches by ignoring the windows lock state.
Testing strategy:
Manual testing
Use speech, enable error sounds, and monitor the logs for errors/warnings.
Known issues with pull request:
None
Change log entries:
None, fixes unreleased regression
Code Review Checklist: