[PATCH] Cygwin: console: Fix exit code for non-cygwin process.

[PATCH] Cygwin: console: Fix exit code for non-cygwin process.

[Takashi Yano]

If non-cygwin process is executed in console, the exit code is not
set correctly. This is because the stub process for non-cygwin app
crashes in fhandler_console::set_disable_master_thread() due to NULL
pointer dereference. This bug was introduced by the commit:
3721a756b0d8 ("Cygwin: console: Make the console accessible from
other terminals."), that the pointer cons is accessed before fixing
when it is NULL. This patch fixes the issue.

Fixes: 3721a756b0d8 ("Cygwin: console: Make the console accessible from other terminals.")
Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 winsup/cygwin/fhandler/console.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/fhandler/console.cc b/winsup/cygwin/fhandler/console.cc
index b924a6bf3..6a42b4949 100644
--- a/winsup/cygwin/fhandler/console.cc
+++ b/winsup/cygwin/fhandler/console.cc
@@ -4537,9 +4537,6 @@ fhandler_console::need_console_handler ()
 void
 fhandler_console::set_disable_master_thread (bool x, fhandler_console *cons)
 {
-  const _minor_t unit = cons->get_minor ();
-  if (con.disable_master_thread == x)
-    return;
   if (cons == NULL)
     {
       if (cygheap->ctty && cygheap->ctty->get_major () == DEV_CONS_MAJOR)
@@ -4547,6 +4544,9 @@ fhandler_console::set_disable_master_thread (bool x, fhandler_console *cons)
       else
 	return;
     }
+  const _minor_t unit = cons->get_minor ();
+  if (con.disable_master_thread == x)
+    return;
   cons->acquire_input_mutex (mutex_timeout);
   con.disable_master_thread = x;
   cons->release_input_mutex ();
-- 
2.43.0

Re: [PATCH] Cygwin: console: Fix exit code for non-cygwin process.

[Johannes Schindelin]

Hi Takashi,

On Fri, 2 Feb 2024, Takashi Yano wrote:

> If non-cygwin process is executed in console, the exit code is not
> set correctly. This is because the stub process for non-cygwin app
> crashes in fhandler_console::set_disable_master_thread() due to NULL
> pointer dereference. This bug was introduced by the commit:
> 3721a756b0d8 ("Cygwin: console: Make the console accessible from
> other terminals."), that the pointer cons is accessed before fixing
> when it is NULL. This patch fixes the issue.
>
> Fixes: 3721a756b0d8 ("Cygwin: console: Make the console accessible from other terminals.")
> Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>

Thank you for fixing this so swiftly. I still wish the logic was
drastically simpler to understand so that even mere humans like myself
would stand a chance to fix such bugs, but I am happy that it is fixed
now.

Ciao,
Johannes

Re: [PATCH] Cygwin: console: Fix exit code for non-cygwin process.

[Johannes Schindelin]

Hi Takashi,

On Sat, 3 Feb 2024, Johannes Schindelin wrote:

> On Fri, 2 Feb 2024, Takashi Yano wrote:
>
> > If non-cygwin process is executed in console, the exit code is not
> > set correctly. This is because the stub process for non-cygwin app
> > crashes in fhandler_console::set_disable_master_thread() due to NULL
> > pointer dereference. This bug was introduced by the commit:
> > 3721a756b0d8 ("Cygwin: console: Make the console accessible from
> > other terminals."), that the pointer cons is accessed before fixing
> > when it is NULL. This patch fixes the issue.
> >
> > Fixes: 3721a756b0d8 ("Cygwin: console: Make the console accessible from other terminals.")
> > Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
>
> Thank you for fixing this so swiftly. I still wish the logic was
> drastically simpler to understand so that even mere humans like myself
> would stand a chance to fix such bugs, but I am happy that it is fixed
> now.

On IRC, you reported that the thread would crash if `cons` was not fixed
up. The symptom was that that crash would apparently prevent the exit code
from being read, and it would be left at 0, indicating potentially
incorrectly that the non-Cygwin process succeeded.

I wonder: What would it take to change this logic so that the crash would
be detected (and not be misinterpreted as exit code 0)?

Ciao,
Johannes

Re: [PATCH] Cygwin: console: Fix exit code for non-cygwin process.

[Takashi Yano]

On Sat, 3 Feb 2024 15:27:06 +0100 (CET)
Johannes Schindelin wrote:
> On IRC, you reported that the thread would crash if `cons` was not fixed
> up. The symptom was that that crash would apparently prevent the exit code
> from being read, and it would be left at 0, indicating potentially
> incorrectly that the non-Cygwin process succeeded.
> 
> I wonder: What would it take to change this logic so that the crash would
> be detected (and not be misinterpreted as exit code 0)?

I am not sure, but I think it is necessary to modify:
pinfo::exit()
pinfo::meybe_set_exit_code_from_windows()
pinfo::set_exit_code()

I guess detecting crash of sbub process needs modification of
spawn.cc.

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

Re: [PATCH] Cygwin: console: Fix exit code for non-cygwin process.

[Corinna Vinschen]

On Feb  4 00:04, Takashi Yano wrote:
> On Sat, 3 Feb 2024 15:27:06 +0100 (CET)
> Johannes Schindelin wrote:
> > On IRC, you reported that the thread would crash if `cons` was not fixed
> > up. The symptom was that that crash would apparently prevent the exit code
> > from being read, and it would be left at 0, indicating potentially
> > incorrectly that the non-Cygwin process succeeded.
> > 
> > I wonder: What would it take to change this logic so that the crash would
> > be detected (and not be misinterpreted as exit code 0)?
> 
> I am not sure, but I think it is necessary to modify:
> pinfo::exit()
> pinfo::meybe_set_exit_code_from_windows()
> pinfo::set_exit_code()
> 
> I guess detecting crash of sbub process needs modification of
> spawn.cc.

Dumb question: If, as Johannes said, the error code cannot be fetched,
can't we set the error code to a POSIX return code indicating a signal?

I.e., checking for WIFSIGNALED() returns 1 and WTERMSIG() returns, say,
SIGKILL or something?


Corinna