* [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir' @ 2022-10-22 5:34 Takashi Yano 2022-10-22 5:58 ` Johannes Schindelin 0 siblings, 1 reply; 8+ messages in thread From: Takashi Yano @ 2022-10-22 5:34 UTC (permalink / raw) To: cygwin-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir' 2022-10-22 5:34 [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir' Takashi Yano @ 2022-10-22 5:58 ` Johannes Schindelin 2022-10-22 6:12 ` Takashi Yano 0 siblings, 1 reply; 8+ messages in thread From: Johannes Schindelin @ 2022-10-22 5:58 UTC (permalink / raw) To: cygwin-patches, Takashi Yano 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir' 2022-10-22 5:58 ` Johannes Schindelin @ 2022-10-22 6:12 ` Takashi Yano 2022-10-23 20:42 ` Johannes Schindelin 0 siblings, 1 reply; 8+ messages in thread From: Takashi Yano @ 2022-10-22 6:12 UTC (permalink / raw) To: cygwin-patches; +Cc: Johannes Schindelin 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir' 2022-10-22 6:12 ` Takashi Yano @ 2022-10-23 20:42 ` Johannes Schindelin 2022-10-24 9:28 ` Corinna Vinschen 0 siblings, 1 reply; 8+ messages in thread From: Johannes Schindelin @ 2022-10-23 20:42 UTC (permalink / raw) To: Takashi Yano; +Cc: cygwin-patches 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir' 2022-10-23 20:42 ` Johannes Schindelin @ 2022-10-24 9:28 ` Corinna Vinschen 2022-11-18 8:23 ` Johannes Schindelin 0 siblings, 1 reply; 8+ messages in thread From: Corinna Vinschen @ 2022-10-24 9:28 UTC (permalink / raw) To: cygwin-patches; +Cc: Takashi Yano, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir' 2022-10-24 9:28 ` Corinna Vinschen @ 2022-11-18 8:23 ` Johannes Schindelin 2022-11-19 21:38 ` Jeremy Drake 2022-11-21 11:47 ` Corinna Vinschen 0 siblings, 2 replies; 8+ messages in thread From: Johannes Schindelin @ 2022-11-18 8:23 UTC (permalink / raw) To: cygwin-patches; +Cc: Takashi Yano 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir' 2022-11-18 8:23 ` Johannes Schindelin @ 2022-11-19 21:38 ` Jeremy Drake 2022-11-21 11:47 ` Corinna Vinschen 1 sibling, 0 replies; 8+ messages in thread From: Jeremy Drake @ 2022-11-19 21:38 UTC (permalink / raw) To: Johannes Schindelin; +Cc: cygwin-patches, Takashi Yano 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir' 2022-11-18 8:23 ` Johannes Schindelin 2022-11-19 21:38 ` Jeremy Drake @ 2022-11-21 11:47 ` Corinna Vinschen 1 sibling, 0 replies; 8+ messages in thread From: Corinna Vinschen @ 2022-11-21 11:47 UTC (permalink / raw) To: cygwin-patches 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-11-21 11:47 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-22 5:34 [PATCH] Cygwin: pty: Fix 'Bad address' error when running 'cmd.exe /c dir' Takashi Yano 2022-10-22 5:58 ` Johannes Schindelin 2022-10-22 6:12 ` Takashi Yano 2022-10-23 20:42 ` Johannes Schindelin 2022-10-24 9:28 ` Corinna Vinschen 2022-11-18 8:23 ` Johannes Schindelin 2022-11-19 21:38 ` Jeremy Drake 2022-11-21 11:47 ` Corinna Vinschen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).