Make config1.db readable from AppContainer apps
#1080
Merged
Conversation
This file contains 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
There was a problem hiding this comment.
Hi yukawa,
Thank you for your PR.
We appreciate your fix against #1076.
yukawa
force-pushed
the
issue_1076_3
branch
3 times, most recently
from
6babeef to
353599a
Compare
This attempts to avoid the deadlock issue discussed in google#1076. Our current hypothesis to explain google#1076 is that "broadFileSystemAccess" capability given to SearchHost.exe is playing an interesting role. When TIP DLL calls CreateFileW API to open "config1.db", the process itself does not have sufficient permission to open it. Then "broadFileSystemAccess" capability will take place and Windows.Storage.OneCore.dll will attempts brokered file access after initializing the thread with RoInitialize() when not yet initialized. If this happens in a certain situation, TSF runtime gets confused and may start re-initializing Mozc TIP. 1. TSF calls back into Mozc TIP 2. Mozc TIP calls CreateFileW() 3. The system invokes RoInitialize() before returning from CreateFileW() 4. TSF calls back into Mozc TIP before returning from RoInitialize() 5. Now Mozc TIP is handling a reentrant callback. Given that whether the target app has "broadFileSystemAccess" capability or not is completely out of Mozc TIP's control, the safest option would be to ensure that the target file/dir is always accessible to AppContainer processes. As the first step, this commit makes 'config1.db' readable from AppContainer processes. There are two cases when the file permission of 'config1.db' is updated. A. When the installer is installing a new version of Mozc. B. When 'config1.db' is updated by mozc_server or mozc_tool. If this commit is not sufficient to address google#1076, we then need to take care of other cases such as calling GetFileAttributes() against the user profile directory.
|
We have merged your PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This attempts to avoid the deadlock issue discussed in #1076.
From what I can observe on Windows 11 23H2 (22631.4317), it looks to be dangerous for us to call File I/O Win32 APIs such as
CreateFilefrom AppContainer processes unless we are confident that the target file/dir is accessible to the AppContainer process. When such an access is not allowed at the ACL level, the debugger shows thatWindows.Storage.OneCore.dlltries to forward it to a broker process (e.g. viaWindows.Storage.OneCore.dll!BrokeredCreateFile2). The issue is that Windows.Storage.OneCore.dll is a WinRT component, which means that just callingCreateFile()results in dynamically invokingRoInitialize()internally, which can confuse TSF runtime as we have already seen in #856.To summarize, the safest option is to ensure that the target file/dir is always accessible to AppContainer so that
Windows.Storage.OneCore.dllwill not be involved if the file/dir is opened withCreateFile()fromMozcTip{32,64}.dll.If this commit is not sufficient to address #1076, we then need to take care of other File I/O APIs such as
CreateDirectoryW()andGetFileAttributes().Issue IDs
#1076.
Steps to test new behaviors
A clear and concise description about how to verify new behaviors.
cacls %LOCALAPPDATA%Low\Mozc\config1.dbMozc64.msiwith this commit to update Mozc.cacls %LOCALAPPDATA%Low\Mozc\config1.dbALL APPLICATION PACKAGES.ALL APPLICATION PACKAGESas follows.Steps to test new behaviors
Not yet confirmed to be a valid fix.