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

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

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