Fix stdio with app execution aliases (Microsoft Store applications) #122
Conversation
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
dscho
force-pushed
the
fix-stdio-with-app-exec-aliases-msys2
branch
from
d77ba7e to
cf0d96a
Compare
If is_console_app() returns false, it means the app is GUI. In this case, standard handles would not be setup for non-cygwin app. Therefore, it is safer to return true for unknown case. Setting-up standard handles for GUI apps is poinless indeed, but not unsafe. Fixes: bb42852 ("Cygwin: pty: Implement new pseudo console support.") Reviewed-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
When that function was introduced in bb42852 (Cygwin: pty: Implement new pseudo console support., 2020-08-19) (back then, it was added to `spawn.cc`, later it was moved to `fhandler/termios.cc` in 32d6a6c (Cygwin: pty, console: Encapsulate spawn.cc code related to pty/console., 2022-11-19)), it was implemented with strong assumptions that neither creating the file handle nor reading 1024 bytes from said handle could fail. This assumption, however, is incorrect. Concretely, I encountered the case where `is_console_app()` needed to open an app execution alias, failed to do so, and still tried to read from the invalid handle. Let's add some error handling to that function. Fixes: bb42852 (Cygwin: pty: Implement new pseudo console support., 2020-08-19) Co-authored-by: Takashi Yano <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
… first This function contains special handling of these file extensions, treating them as console applications always, even if the first 1024 bytes do not contain a PE header with the console bits set. However, Batch and Command files are never expected to have such a header, therefore opening them and reading their first bytes is a waste of time. Let's honor the best practice to deal with easy conditions that allow early returns first. Fixes: bb42852 (Cygwin: pty: Implement new pseudo console suppot., 2020-08-19) Reviewed-by: Takashi Yano <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
An app execution alias cannot be opened for read (CreateFile() with GENERIC_READ fails with ERROR_CANT_ACCESS_FILE) because it does not resolve the reparse point for app execution alias. Therefore, we need to know if the path is an app execution alias when opening it. This patch adds new api path_conv::is_app_execution_alias() for that purpose. Reviewed-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
This patch changes the argument for passsing a path to an app to fhandler_termios::spawn_worker() from const WCHAR *runpath to path_conv &pc. The purpose of this patch is to prepare for a subsequent patch, that is intended to fix a bug in executing Microsoft Store apps. Reviewed-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
Microsoft Store apps are run via app execution aliases, i.e. special reparse points. Currently, spawn.cc does not resolve a reparse point when retrieving the path of app after the commit f74dc93, that disabled to follow windows reparse point by adding PC_SYM_NOFOLLOW_REP flag. However, unlike proper reparse point, app execution aliases are not resolved when trying to open the file via CreateFile(). As a result, if the path, that is_console_app() received, is the reparse point for an app execution alias, the func retuned false due to open-failure because CreateFile() cannot open an app execution alias, while it can open normal reparse point. If is_console_app() returns false, standard handles for console app (such as WSL) would not be setup. This causes that the console input cannot be transfered to the non-cygwin app. This patch fixes the issue by locally converting the path once again using option PC_SYM_FOLLOW (without PC_SYM_NOFOLLOW_REP), which is used inside is_console_app() to resolve the reparse point, if the path is an app execution alias. Fixes: f74dc93 ("fix native symlink spawn passing wrong arg0") Reviewed-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
dscho
force-pushed
the
fix-stdio-with-app-exec-aliases-msys2
branch
from
cf0d96a to
f27c915
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
When I introduced support for executing Microsoft Store applications through their "app execution aliases" (i.e. special reparse points installed into
%LOCALAPPDATA%\Microsoft\WindowsApps), I had missed that it failed to spawn the process with the correct handles to the terminal, breaking interactive usage of, say, the Python interpreter.This was later reported on the
cygwinmailing list, and also in python/pymanager#210 (which was then re-reported in msys2/MSYS2-packages#1943 (comment)).The root cause is that the
is_console_app()function required quite a bit of TLC, which this here PR tries to provide.This is a companion of cygwingitgadget/cygwin#5 and of msys2/msys2-runtime#322