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.y...@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
