[PATCH] Fix `Bad address` when running `cmd /c [...]`

[PATCH] Fix `Bad address` when running `cmd /c [...]`

[Johannes Schindelin]

In 2b4f986e49 (Cygwin: pty: Treat *.bat and *.cmd as a non-cygwin
console app., 2022-07-31), we introduced a bug fix that specifically
looks for a suffix of the command's file name.

However, that file name might be set to `NULL`, namely when
`null_app_name == true`, which is the case when we detected a
command-line `cmd /c [...]`.

But the commit mentioned above did not account for that possibility,
instead assuming that it always has to check the file name for a `.bat`
or `.cmd` suffix. As a consequence, `cmd /c [...]` invocations are
completely broken in v3.3.6, resulting in a `Bad address` error.

Let's guard the code properly so that it does not try to look at the
file name suffix of `(WCHAR *)NULL`.

This fixes https://github.com/msys2/msys2-runtime/issues/108

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/msys2-runtime/releases/tag/fix-bad-address-in-cmd-invocation-v1
Fetch-It-Via: git fetch https://github.com/dscho/msys2-runtime fix-bad-address-in-cmd-invocation-v1

 winsup/cygwin/spawn.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index d9d7716519..d4e7f10e2b 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -209,7 +209,7 @@ is_console_app (WCHAR *filename)
   char *p = (char *) memmem (buf, n, "PE\0\0", 4);
   if (p && p + id_offset < buf + n)
     return p[id_offset] == '\003'; /* 02: GUI, 03: console */
-  else
+  else if (filename)
     {
       wchar_t *e = wcsrchr (filename, L'.');
       if (e && (wcscasecmp (e, L".bat") == 0 || wcscasecmp (e, L".cmd") == 0))

base-commit: 1ca46b22d6edb3d469b51475e8b096d86deaac67
--
2.38.1.windows.1

Re: [PATCH] Fix `Bad address` when running `cmd /c [...]`

[Takashi Yano]

On Fri, 21 Oct 2022 23:37:35 +0200 (CEST)
Johannes Schindelin wrote:
> In 2b4f986e49 (Cygwin: pty: Treat *.bat and *.cmd as a non-cygwin
> console app., 2022-07-31), we introduced a bug fix that specifically
> looks for a suffix of the command's file name.
> 
> However, that file name might be set to `NULL`, namely when
> `null_app_name == true`, which is the case when we detected a
> command-line `cmd /c [...]`.

It seems that this is msys2 specific issue.
I also noticed that
cmd //c 'echo AAA' instead of cmd /c 'echo AAA' works in msys2.

In cygwin, filename is
C:\WINDOWS\system32\cmd.exe
for cmd /c 'echo AAA'.

Why the filename can be NULL in msys2 in the case of cmd /c 'echo AAA'?

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

Re: [PATCH] Fix `Bad address` when running `cmd /c [...]`

[Takashi Yano]

On Sat, 22 Oct 2022 10:36:39 +0900
Takashi Yano wrote:
> On Fri, 21 Oct 2022 23:37:35 +0200 (CEST)
> Johannes Schindelin wrote:
> > In 2b4f986e49 (Cygwin: pty: Treat *.bat and *.cmd as a non-cygwin
> > console app., 2022-07-31), we introduced a bug fix that specifically
> > looks for a suffix of the command's file name.
> > 
> > However, that file name might be set to `NULL`, namely when
> > `null_app_name == true`, which is the case when we detected a
> > command-line `cmd /c [...]`.
> 
> It seems that this is msys2 specific issue.
> I also noticed that
> cmd //c 'echo AAA' instead of cmd /c 'echo AAA' works in msys2.
> 
> In cygwin, filename is
> C:\WINDOWS\system32\cmd.exe
> for cmd /c 'echo AAA'.
> 
> Why the filename can be NULL in msys2 in the case of cmd /c 'echo AAA'?

I can reproduce the issue in cygwin with cmd.exe /c 'echo AAA'
instead of cmd /c 'echo AAA'.

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

Re: [PATCH] Fix `Bad address` when running `cmd /c [...]`

[Takashi Yano]

On Sat, 22 Oct 2022 10:54:06 +0900
Takashi Yano wrote:
> On Sat, 22 Oct 2022 10:36:39 +0900
> Takashi Yano wrote:
> > On Fri, 21 Oct 2022 23:37:35 +0200 (CEST)
> > Johannes Schindelin wrote:
> > > In 2b4f986e49 (Cygwin: pty: Treat *.bat and *.cmd as a non-cygwin
> > > console app., 2022-07-31), we introduced a bug fix that specifically
> > > looks for a suffix of the command's file name.
> > > 
> > > However, that file name might be set to `NULL`, namely when
> > > `null_app_name == true`, which is the case when we detected a
> > > command-line `cmd /c [...]`.
> > 
> > It seems that this is msys2 specific issue.
> > I also noticed that
> > cmd //c 'echo AAA' instead of cmd /c 'echo AAA' works in msys2.
> > 
> > In cygwin, filename is
> > C:\WINDOWS\system32\cmd.exe
> > for cmd /c 'echo AAA'.
> > 
> > Why the filename can be NULL in msys2 in the case of cmd /c 'echo AAA'?
> 
> I can reproduce the issue in cygwin with cmd.exe /c 'echo AAA'
> instead of cmd /c 'echo AAA'.

I have just pushed a counter patch. Thanks for the report.

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

Re: [PATCH] Fix `Bad address` when running `cmd /c [...]`

[Johannes Schindelin]

On October 22, 2022 7:37:09 AM GMT+02:00, Takashi Yano <takashi.yano@nifty.ne.jp> wrote:
>On Sat, 22 Oct 2022 10:54:06 +0900
>Takashi Yano wrote:
>> On Sat, 22 Oct 2022 10:36:39 +0900
>> Takashi Yano wrote:
>> > On Fri, 21 Oct 2022 23:37:35 +0200 (CEST)
>> > Johannes Schindelin wrote:
>> > > In 2b4f986e49 (Cygwin: pty: Treat *.bat and *.cmd as a non-cygwin
>> > > console app., 2022-07-31), we introduced a bug fix that specifically
>> > > looks for a suffix of the command's file name.
>> > > 
>> > > However, that file name might be set to `NULL`, namely when
>> > > `null_app_name == true`, which is the case when we detected a
>> > > command-line `cmd /c [...]`.
>> > 
>> > It seems that this is msys2 specific issue.
>> > I also noticed that
>> > cmd //c 'echo AAA' instead of cmd /c 'echo AAA' works in msys2.
>> > 
>> > In cygwin, filename is
>> > C:\WINDOWS\system32\cmd.exe
>> > for cmd /c 'echo AAA'.
>> > 
>> > Why the filename can be NULL in msys2 in the case of cmd /c 'echo AAA'?
>> 
>> I can reproduce the issue in cygwin with cmd.exe /c 'echo AAA'
>> instead of cmd /c 'echo AAA'.
>
>I have just pushed a counter patch. Thanks for the report.
>

Thanks, I guess?

It's not very nice to simply drop my work, and then not even link to your "counter".

Re: [PATCH] Fix `Bad address` when running `cmd /c [...]`

[Takashi Yano]

On Sat, 22 Oct 2022 07:55:53 +0200
Johannes Schindelin wrote:
> It's not very nice to simply drop my work, and then not even link to your "counter".

I am sorry, however, your patch can be indirectly reached
via link https://github.com/msys2/msys2-runtime/issues/108
in my counter patch.

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

Re: [PATCH] Fix `Bad address` when running `cmd /c [...]`

[Johannes Schindelin]

Hi Takashi,

On Sat, 22 Oct 2022, Takashi Yano wrote:

> On Sat, 22 Oct 2022 07:55:53 +0200
> Johannes Schindelin wrote:
> > It's not very nice to simply drop my work, and then not even link to your "counter".
>
> I am sorry, however, your patch can be indirectly reached
> via link https://github.com/msys2/msys2-runtime/issues/108
> in my counter patch.

You misunderstand what I wrote.

You answered the question "where is the link _from_ your 'counter' to the
original patch?"

But the concern was "you did not link _to_ your 'counter'".

For posterity, here is that link:
https://github.com/cygwin/cygwin/commit/08281cf4cca9593adcc3d30184322dc60fa1cd61

Ciao,
Johannes

P.S.: I quote "counter" because what you meant is actually a counter
proposal, not a counter (which is typically a variable that is used to
count things). It's one more irritating thing in a thread that was not
exactly short on those things. Quite frustrating.