[PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir'

[PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir'

[Takashi Yano]

- If the command executed is 'cmd.exe /c [...]', runpath in spawn.cc
  will be NULL. In this case, is_console_app(runpath) check causes
  access violation. This case also the command executed is obviously
  console app., therefore, treat it as console app to fix this issue.

  Addresses: https://github.com/msys2/msys2-runtime/issues/108
---
 winsup/cygwin/spawn.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 5aa52ab1e..4fc842a2b 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -215,6 +215,8 @@ handle (int fd, bool writing)
 static bool
 is_console_app (WCHAR *filename)
 {
+  if (filename == NULL)
+    return true; /* The command executed is command.com or cmd.exe. */
   HANDLE h;
   const int id_offset = 92;
   h = CreateFileW (filename, GENERIC_READ, FILE_SHARE_READ,
-- 
2.37.2

Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir'

[Johannes Schindelin]

On October 22, 2022 7:34:20 AM GMT+02:00, Takashi Yano <takashi.yano@nifty.ne.jp> wrote:
>- If the command executed is 'cmd.exe /c [...]', runpath in spawn.cc
>  will be NULL. In this case, is_console_app(runpath) check causes
>  access violation. This case also the command executed is obviously
>  console app., therefore, treat it as console app to fix this issue.
>
>  Addresses: https://github.com/msys2/msys2-runtime/issues/108
>---
> winsup/cygwin/spawn.cc | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
>index 5aa52ab1e..4fc842a2b 100644
>--- a/winsup/cygwin/spawn.cc
>+++ b/winsup/cygwin/spawn.cc
>@@ -215,6 +215,8 @@ handle (int fd, bool writing)
> static bool
> is_console_app (WCHAR *filename)
> {
>+  if (filename == NULL)
>+    return true; /* The command executed is command.com or cmd.exe. */
>   HANDLE h;
>   const int id_offset = 92;
>   h = CreateFileW (filename, GENERIC_READ, FILE_SHARE_READ,

The commit message of the original patch was substantially clearer and offered a thorough analysis. This patch lost that.

Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir'

[Takashi Yano]

On Sat, 22 Oct 2022 07:58:37 +0200
Johannes Schindelin wrote:
> On October 22, 2022 7:34:20 AM GMT+02:00, Takashi Yano <takashi.yano@nifty.ne.jp> wrote:
> >- If the command executed is 'cmd.exe /c [...]', runpath in spawn.cc
> >  will be NULL. In this case, is_console_app(runpath) check causes
> >  access violation. This case also the command executed is obviously
> >  console app., therefore, treat it as console app to fix this issue.
> >
> >  Addresses: https://github.com/msys2/msys2-runtime/issues/108
> >---
> > winsup/cygwin/spawn.cc | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> >diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> >index 5aa52ab1e..4fc842a2b 100644
> >--- a/winsup/cygwin/spawn.cc
> >+++ b/winsup/cygwin/spawn.cc
> >@@ -215,6 +215,8 @@ handle (int fd, bool writing)
> > static bool
> > is_console_app (WCHAR *filename)
> > {
> >+  if (filename == NULL)
> >+    return true; /* The command executed is command.com or cmd.exe. */
> >   HANDLE h;
> >   const int id_offset = 92;
> >   h = CreateFileW (filename, GENERIC_READ, FILE_SHARE_READ,
> 
> The commit message of the original patch was substantially clearer and offered a thorough analysis. This patch lost that.

The reason which I did not apply your patch as-is is:
is_console_app() returns false for 'cmd.exe /c [...]' case
with your patch, while it should return true. 

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir'

[Johannes Schindelin]

Hi Takashi,

On Sat, 22 Oct 2022, Takashi Yano wrote:

> On Sat, 22 Oct 2022 07:58:37 +0200
> Johannes Schindelin wrote:
> > On October 22, 2022 7:34:20 AM GMT+02:00, Takashi Yano <takashi.yano@nifty.ne.jp> wrote:
> > >- If the command executed is 'cmd.exe /c [...]', runpath in spawn.cc
> > >  will be NULL. In this case, is_console_app(runpath) check causes
> > >  access violation. This case also the command executed is obviously
> > >  console app., therefore, treat it as console app to fix this issue.
> > >
> > >  Addresses: https://github.com/msys2/msys2-runtime/issues/108
> > >---
> > > winsup/cygwin/spawn.cc | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > >diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> > >index 5aa52ab1e..4fc842a2b 100644
> > >--- a/winsup/cygwin/spawn.cc
> > >+++ b/winsup/cygwin/spawn.cc
> > >@@ -215,6 +215,8 @@ handle (int fd, bool writing)
> > > static bool
> > > is_console_app (WCHAR *filename)
> > > {
> > >+  if (filename == NULL)
> > >+    return true; /* The command executed is command.com or cmd.exe. */
> > >   HANDLE h;
> > >   const int id_offset = 92;
> > >   h = CreateFileW (filename, GENERIC_READ, FILE_SHARE_READ,
> >
> > The commit message of the original patch was substantially clearer and offered a thorough analysis. This patch lost that.
>
> The reason which I did not apply your patch as-is is:
> is_console_app() returns false for 'cmd.exe /c [...]' case
> with your patch, while it should return true.

Sure. And a simple "can you please modify the patch to return `true` in
the `cmd /c <command>` case" feedback would have avoided all the
contention.

Having said that, I fear that you completely misread what I wrote, as I
did not comment on your diff but on your quite improvable commit message.

Ciao,
Johannes

Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir'

[Corinna Vinschen]

On Oct 23 22:42, Johannes Schindelin wrote:
> On Sat, 22 Oct 2022, Takashi Yano wrote:
> > On Sat, 22 Oct 2022 07:58:37 +0200
> > Johannes Schindelin wrote:
> > > On October 22, 2022 7:34:20 AM GMT+02:00, Takashi Yano <takashi.yano@nifty.ne.jp> wrote:
> > > >- If the command executed is 'cmd.exe /c [...]', runpath in spawn.cc
> > > >  will be NULL. In this case, is_console_app(runpath) check causes
> > > >  access violation. This case also the command executed is obviously
> > > >  console app., therefore, treat it as console app to fix this issue.
> > > >
> > > >  Addresses: https://github.com/msys2/msys2-runtime/issues/108
> > > >---
> > > > winsup/cygwin/spawn.cc | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > >diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> > > >index 5aa52ab1e..4fc842a2b 100644
> > > >--- a/winsup/cygwin/spawn.cc
> > > >+++ b/winsup/cygwin/spawn.cc
> > > >@@ -215,6 +215,8 @@ handle (int fd, bool writing)
> > > > static bool
> > > > is_console_app (WCHAR *filename)
> > > > {
> > > >+  if (filename == NULL)
> > > >+    return true; /* The command executed is command.com or cmd.exe. */
> > > >   HANDLE h;
> > > >   const int id_offset = 92;
> > > >   h = CreateFileW (filename, GENERIC_READ, FILE_SHARE_READ,
> > >
> > > The commit message of the original patch was substantially clearer and offered a thorough analysis. This patch lost that.
> >
> > The reason which I did not apply your patch as-is is:
> > is_console_app() returns false for 'cmd.exe /c [...]' case
> > with your patch, while it should return true.

I'm a bit concerned here.

Did you notice the fact that a NULL filename *also* results in calling
CreateFileW with a NULL filename, in calling ReadFile and CloseHandle
with a INVALID_HANDLE_VALUE, and running memmem on an uninitialized
buffer?  This doesn't result in an immediate crash, but it's a serious
problem nevertheless.

Johannes' patch didn't fix that.  Takashi's patch does, but somehow you
both don't even mention it.

> Sure. And a simple "can you please modify the patch to return `true` in
> the `cmd /c <command>` case" feedback would have avoided all the
> contention.

Well...

> Having said that, I fear that you completely misread what I wrote, as I
> did not comment on your diff but on your quite improvable commit message.

Sorry, but we can't change the commit message retroactively because of
the commit hooks which disallow forced commits on non-topic branches.

However, two points:

- I'm wondering if the patch (both of yours) doesn't actually just cover
  a problem in child_info_spawn::worker().  Different runpath values,
  depending on the app path being "cmd" or "cmd.exe"?  That sounds like
  worker() is doing weird stuff.  And it does in line 400ff.

  So, if the else branch of this code is apparently working fine for
  "cmd" per Takashi's observation in
  https://cygwin.com/pipermail/cygwin-patches/2022q4/012032.html, how
  much sense is in the if branch handling "command.com" and "cmd.exe"
  specially?  Wouldn't a better patch get rid of this extra if and
  the null_app_name variable instead?

- While we never had a rule for that, it would be great in future,
  if the commit message contains a "Fixes:" tag, if it's clear that
  the patch fixes an older patch, along the lines as the Linux
  kernel does it.  As an example, for this patch it would have been
  great to see a footer in the commit message like this:

  Fixes: 2b4f986e499f ("Cygwin: pty: Treat *.bat and *.cmd as a non-cygwin console app.")


Corinna

Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir'

[Johannes Schindelin]

Hi Corinna,

On Mon, 24 Oct 2022, Corinna Vinschen wrote:

> However, two points:
>
> - I'm wondering if the patch (both of yours) doesn't actually just cover
>   a problem in child_info_spawn::worker().  Different runpath values,
>   depending on the app path being "cmd" or "cmd.exe"?  That sounds like
>   worker() is doing weird stuff.  And it does in line 400ff.
>
>   So, if the else branch of this code is apparently working fine for
>   "cmd" per Takashi's observation in
>   https://cygwin.com/pipermail/cygwin-patches/2022q4/012032.html, how
>   much sense is in the if branch handling "command.com" and "cmd.exe"
>   specially?  Wouldn't a better patch get rid of this extra if and
>   the null_app_name variable instead?

I never understood why the pcon code was allowed to be so Hydra-like as to
sprawl into corners far, far beyond `winsup/cygwin/fhandler*`.

FWIW I would be in favor of getting rid of this special handling (unless
it causes a regression). Given the recent experience, I expect Takashi to
want to work on this without any interference from my side.

Ciao,
Dscho

Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir'

[Jeremy Drake]

On Fri, 18 Nov 2022, Johannes Schindelin wrote:

> Hi Corinna,
>
> On Mon, 24 Oct 2022, Corinna Vinschen wrote:
>
> > However, two points:
> >
> > - I'm wondering if the patch (both of yours) doesn't actually just cover
> >   a problem in child_info_spawn::worker().  Different runpath values,
> >   depending on the app path being "cmd" or "cmd.exe"?  That sounds like
> >   worker() is doing weird stuff.  And it does in line 400ff.
> >
> >   So, if the else branch of this code is apparently working fine for
> >   "cmd" per Takashi's observation in
> >   https://cygwin.com/pipermail/cygwin-patches/2022q4/012032.html, how
> >   much sense is in the if branch handling "command.com" and "cmd.exe"
> >   specially?  Wouldn't a better patch get rid of this extra if and
> >   the null_app_name variable instead?
>
> FWIW I would be in favor of getting rid of this special handling (unless
> it causes a regression). Given the recent experience, I expect Takashi to
> want to work on this without any interference from my side.


I was thinking maybe this check was intended to handle the, umm, "special"
quoting rules that "cmd /c" has (especially without "/s").  I don't know
why that would have anything to do with pcon though, so I may be totally
off the mark.

Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir'

[Corinna Vinschen]

On Nov 18 09:23, Johannes Schindelin wrote:
> Hi Corinna,
> 
> On Mon, 24 Oct 2022, Corinna Vinschen wrote:
> 
> > However, two points:
> >
> > - I'm wondering if the patch (both of yours) doesn't actually just cover
> >   a problem in child_info_spawn::worker().  Different runpath values,
> >   depending on the app path being "cmd" or "cmd.exe"?  That sounds like
> >   worker() is doing weird stuff.  And it does in line 400ff.
> >
> >   So, if the else branch of this code is apparently working fine for
> >   "cmd" per Takashi's observation in
> >   https://cygwin.com/pipermail/cygwin-patches/2022q4/012032.html, how
> >   much sense is in the if branch handling "command.com" and "cmd.exe"
> >   specially?  Wouldn't a better patch get rid of this extra if and
> >   the null_app_name variable instead?
> 
> I never understood why the pcon code was allowed to be so Hydra-like as to
> sprawl into corners far, far beyond `winsup/cygwin/fhandler*`.
> 
> FWIW I would be in favor of getting rid of this special handling (unless
> it causes a regression).

I'm a bit surprised to read that, you should already have seen that.
I did so end of October:

https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=f33635ae6076
https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=213b53ed3557


Corinna