Hi Takashi,
On Tue, 16 Dec 2025, Takashi Yano wrote:
> After the commit f74dc93c6359, WSL cannot start by distribution name
> such as debian.exe, which has '.exe' extention but actually is a symlink.
> This is because is_console_app () returns false for that symlink due
> to open-failure. This patch fixes the issue using PC_SYM_FOLLOW option
> in path_conv.
The commit message still does not clarify that this has pretty nothing to
do with symlinks, but everything with app execution aliases.
In fact, the commit message still misleads the reader to think that
executing each any every symlink is broken, which could easily lead to a
lot of time wasted wondering why this bug hasn't been detected for a very,
very long time. Let's avoid being this inconsiderate of reviewers' time.
Speaking of reviewers' time: I implore you to rethink the practice of
tossing a v2 over the fence without saying what was wrong with v1 and how
v2 addresses that. If you truly care for the craft of software
engineering, you will realize that explaining your thought process is a
vital part of any contribution, and that is was I am missing here.
> Fixes: f74dc93c6359 ("fix native symlink spawn passing wrong arg0")
Hmm. Since this patch likely fixes the problem reported in
https://inbox.sourceware.org/cygwin/CAAM_cieBo_M76sqZMGgF+tXxswvT=juhl_pshff+arv9p1e...@mail.gmail.com
(and even **implicitly** talks about app execution aliases, because that's
what `debian.exe` resolves to), I am unsure that this footer refers to
**the** most related commit.
> Reviewed-by:
An empty footer? Was that an oversight or intentional?
> Signed-off-by: Takashi Yano <[email protected]>
> ---
> winsup/cygwin/fhandler/termios.cc | 19 ++++++++++++++-----
> winsup/cygwin/local_includes/fhandler.h | 2 +-
> winsup/cygwin/spawn.cc | 2 +-
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/winsup/cygwin/fhandler/termios.cc
> b/winsup/cygwin/fhandler/termios.cc
> index 19d6220bc..ff6c06015 100644
> --- a/winsup/cygwin/fhandler/termios.cc
> +++ b/winsup/cygwin/fhandler/termios.cc
> @@ -702,10 +702,19 @@ fhandler_termios::fstat (struct stat *buf)
> }
>
> static bool
> -is_console_app (const WCHAR *filename)
> +is_console_app (path_conv *pc)
For the sake of readers who aren't familiar with your v1: This is probably
the most interesting change between v1 and v2 of this patch.
Since `NULL` would not make sense in this function, I would deem
`path_conv &pc` more appropriate.
However, it is said that there are two categories of patches in which no
obvious bugs reside: those patches that are so clear and elegant that
there simply cannot be any obvious bug, and those so convoluted or so
overarching that there are at least no _obvious_ bugs.
The structure of this patch looks very little like the former, desirable
category to me. This is worrisome given the extended history of this part
of Cygwin's source code that has seen way more bugs and regressions than
I'd like to unleash on my users. Yet it would be so easy to gain more
confidence in this patch, simply by extracting out the signature change
(and making it a proper reference instead of a pointer)!
> {
> + const WCHAR *native_path = pc->get_nt_native_path ()->Buffer;
> + if (pc->issymlink ())
That's probably a bit too overarching. Have you tried whether this patch
is required to execute programs via regular symbolic links instead of app
execution aliases?
> + {
> + UNICODE_STRING upath;
> + RtlInitUnicodeString (&upath, native_path);
> + path_conv target (&upath, PC_SYM_FOLLOW);
> + native_path = target.get_nt_native_path ()->Buffer;
> + }
> +
> HANDLE h;
> - h = CreateFileW (filename, GENERIC_READ, FILE_SHARE_READ,
> + h = CreateFileW (native_path, GENERIC_READ, FILE_SHARE_READ,
How certain can we be that the `Buffer` of that now out-of-scope `target`
is still valid at this point? This code pattern is highly indicative of a
use-after-free problem (if not in the present, then quite likely in the
future).
> NULL, OPEN_EXISTING, 0, NULL);
> char buf[1024];
> DWORD n;
> @@ -716,7 +725,7 @@ is_console_app (const WCHAR *filename)
> IMAGE_NT_HEADERS32 *p = (IMAGE_NT_HEADERS32 *) memmem (buf, n, "PE\0\0",
> 4);
> if (p && (char *) &p->OptionalHeader.DllCharacteristics <= buf + n)
> return p->OptionalHeader.Subsystem == IMAGE_SUBSYSTEM_WINDOWS_CUI;
> - wchar_t *e = wcsrchr (filename, L'.');
> + wchar_t *e = wcsrchr (native_path, L'.');
> if (e && (wcscasecmp (e, L".bat") == 0 || wcscasecmp (e, L".cmd") == 0))
> return true;
In my patch series (which tries to target very specifically the problem of
app execution aliases _without_ risking to cause any regression in only
vaguely-related scenarios such as canonical symlinks), I move this check
up so that the comparatively expensive read operation can be avoided if we
already know that the file extension indicates a console application.
You can find my v2 (with change log since v1 and range-diff...) here:
https://inbox.sourceware.org/cygwin-patches/[email protected]/
I understand that it is technically "more correct" to resolve any symlink
and then look at the target file name instead of looking at `filename`
always. In practice, I highly doubt that any app execution alias exists
that has a different file extension than its target.
It would probably make more sense to collaborate and try to combine the
best of your patch with the best of my patch series. For example, I could
easily integrate a patch into my series that changes the signature of
`is_console_app()` to take a `path_conv &` instead of a `WCHAR *`.
Ciao,
Johannes
> return false;
> @@ -755,7 +764,7 @@ fhandler_termios::ioctl (unsigned int cmd, void *varg)
>
> void
> fhandler_termios::spawn_worker::setup (bool iscygwin, HANDLE h_stdin,
> - const WCHAR *runpath, bool nopcon,
> + path_conv *pc, bool nopcon,
> bool reset_sendsig,
> const WCHAR *envblock)
> {
> @@ -794,7 +803,7 @@ fhandler_termios::spawn_worker::setup (bool iscygwin,
> HANDLE h_stdin,
> ptys->setup_locale ();
> }
> }
> - if (!iscygwin && ptys_primary && is_console_app (runpath))
> + if (!iscygwin && ptys_primary && is_console_app (pc))
> {
> if (h_stdin == ptys_primary->get_handle_nat ())
> stdin_is_ptys = true;
> diff --git a/winsup/cygwin/local_includes/fhandler.h
> b/winsup/cygwin/local_includes/fhandler.h
> index 0de82163e..d2d724fb7 100644
> --- a/winsup/cygwin/local_includes/fhandler.h
> +++ b/winsup/cygwin/local_includes/fhandler.h
> @@ -2036,7 +2036,7 @@ class fhandler_termios: public fhandler_base
> spawn_worker () :
> ptys_need_cleanup (false), cons_need_cleanup (false),
> stdin_is_ptys (false), ptys_ttyp (NULL) {}
> - void setup (bool iscygwin, HANDLE h_stdin, const WCHAR *runpath,
> + void setup (bool iscygwin, HANDLE h_stdin, path_conv *pc,
> bool nopcon, bool reset_sendsig, const WCHAR *envblock);
> bool need_cleanup () { return ptys_need_cleanup || cons_need_cleanup; }
> void cleanup ();
> diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> index 71add8755..9c062d58f 100644
> --- a/winsup/cygwin/spawn.cc
> +++ b/winsup/cygwin/spawn.cc
> @@ -579,7 +579,7 @@ child_info_spawn::worker (const char *prog_arg, const
> char *const *argv,
>
> bool no_pcon = mode != _P_OVERLAY && mode != _P_WAIT;
> term_spawn_worker.setup (iscygwin (), handle (fileno_stdin, false),
> - runpath, no_pcon, reset_sendsig, envblock);
> + &real_path, no_pcon, reset_sendsig, envblock);
>
> /* Set up needed handles for stdio */
> si.dwFlags = STARTF_USESTDHANDLES;
> --
> 2.51.0
>
>
