public inbox for cygwin-patches@cygwin.com
 help / color / mirror / Atom feed
* [PATCH v2] Cygwin: pinfo: Fix exit code for non-cygwin apps which reads console.
@ 2022-02-27  0:46 Takashi Yano
  2022-02-28  9:21 ` Corinna Vinschen
  0 siblings, 1 reply; 4+ messages in thread
From: Takashi Yano @ 2022-02-27  0:46 UTC (permalink / raw)
  To: cygwin-patches

- The recent commit "Cygwin: pinfo: Fix exit code when non-cygwin app
  exits by Ctrl-C." did not fix enough the issue. If a non-cygwin app
  is reading the console, it will not return STATUS_CONTROL_C_EXIT
  even if it is terminated by Ctrl-C. As a result, the previous patch
  does not take effect.
  This patch solves this issue by setting sigExeced to SIGINT in
  ctrl_c_handler(). In addition, sigExeced will be cleared if the app
  does not terminated within predetermined time period. The reason is
  that the app does not seem to be terminated by the signal sigExeced.
---
 winsup/cygwin/exceptions.cc |  6 +++++-
 winsup/cygwin/globals.cc    |  2 +-
 winsup/cygwin/spawn.cc      | 15 ++++++++++++++-
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 6e0b862c7..070e52e76 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -1139,7 +1139,11 @@ ctrl_c_handler (DWORD type)
     }
 
   if (ch_spawn.set_saw_ctrl_c ())
-    return TRUE;
+    {
+      if (myself->process_state & PID_NOTCYGWIN)
+	sigExeced = SIGINT;
+      return TRUE;
+    }
 
   /* We're only the process group leader when we have a valid pinfo structure.
      If we don't have one, then the parent "stub" will handle the signal. */
diff --git a/winsup/cygwin/globals.cc b/winsup/cygwin/globals.cc
index ac5ad0307..d3a2e11a4 100644
--- a/winsup/cygwin/globals.cc
+++ b/winsup/cygwin/globals.cc
@@ -20,7 +20,7 @@ HANDLE NO_COPY hProcImpToken;
 HANDLE my_wr_proc_pipe;
 HMODULE NO_COPY cygwin_hmodule;
 HMODULE NO_COPY hntdll;
-int NO_COPY sigExeced;
+LONG NO_COPY sigExeced;
 WCHAR windows_system_directory[MAX_PATH];
 UINT windows_system_directory_length;
 WCHAR system_wow64_directory[MAX_PATH];
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 3647580a6..3b54309a2 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -953,7 +953,15 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
 	  if (sem)
 	    __posix_spawn_sem_release (sem, 0);
 	  if (ptys_need_cleanup || cons_need_cleanup)
-	    WaitForSingleObject (pi.hProcess, INFINITE);
+	    {
+	      LONG prev_sigExeced = sigExeced;
+	      while (WaitForSingleObject (pi.hProcess, 100) == WAIT_TIMEOUT)
+		/* If child process does not exit in predetermined time
+		   period, the process does not seem to be terminated by
+		   the signal sigExeced. Therefore, clear sigExeced here. */
+		prev_sigExeced =
+		  InterlockedCompareExchange (&sigExeced, 0, prev_sigExeced);
+	    }
 	  if (ptys_need_cleanup)
 	    {
 	      fhandler_pty_slave::cleanup_for_non_cygwin_app (&ptys_handle_set,
@@ -966,6 +974,11 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
 	      fhandler_console::cleanup_for_non_cygwin_app (&cons_handle_set);
 	      fhandler_console::close_handle_set (&cons_handle_set);
 	    }
+	  /* Make sure that ctrl_c_handler() is not on going. Calling
+	     init_console_handler(false) locks until returning from
+	     ctrl_c_handler(). This insures that setting sigExeced
+	     on Ctrl-C key has been completed. */
+	  init_console_handler (false);
 	  myself.exit (EXITCODE_NOSET);
 	  break;
 	case _P_WAIT:
-- 
2.35.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] Cygwin: pinfo: Fix exit code for non-cygwin apps which reads console.
  2022-02-27  0:46 [PATCH v2] Cygwin: pinfo: Fix exit code for non-cygwin apps which reads console Takashi Yano
@ 2022-02-28  9:21 ` Corinna Vinschen
  2022-02-28  9:32   ` Takashi Yano
  0 siblings, 1 reply; 4+ messages in thread
From: Corinna Vinschen @ 2022-02-28  9:21 UTC (permalink / raw)
  To: cygwin-patches

Hi Takashi,

On Feb 27 09:46, Takashi Yano wrote:
> - The recent commit "Cygwin: pinfo: Fix exit code when non-cygwin app
>   exits by Ctrl-C." did not fix enough the issue. If a non-cygwin app
>   is reading the console, it will not return STATUS_CONTROL_C_EXIT
>   even if it is terminated by Ctrl-C. As a result, the previous patch
>   does not take effect.
>   This patch solves this issue by setting sigExeced to SIGINT in
>   ctrl_c_handler(). In addition, sigExeced will be cleared if the app
>   does not terminated within predetermined time period. The reason is
>   that the app does not seem to be terminated by the signal sigExeced.
> [...]
> --- a/winsup/cygwin/spawn.cc
> +++ b/winsup/cygwin/spawn.cc
> @@ -953,7 +953,15 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
>  	  if (sem)
>  	    __posix_spawn_sem_release (sem, 0);
>  	  if (ptys_need_cleanup || cons_need_cleanup)
> -	    WaitForSingleObject (pi.hProcess, INFINITE);
> +	    {
> +	      LONG prev_sigExeced = sigExeced;
> +	      while (WaitForSingleObject (pi.hProcess, 100) == WAIT_TIMEOUT)
> +		/* If child process does not exit in predetermined time
> +		   period, the process does not seem to be terminated by
> +		   the signal sigExeced. Therefore, clear sigExeced here. */
> +		prev_sigExeced =
> +		  InterlockedCompareExchange (&sigExeced, 0, prev_sigExeced);
> +	    }
>  	  if (ptys_need_cleanup)
>  	    {
>  	      fhandler_pty_slave::cleanup_for_non_cygwin_app (&ptys_handle_set,

Is it really necessary to run the InterlockedCompareExchange in a loop?
What about

  if (WFMO(..., 100) == WAIT_TIMEOUT)
    {
      InterlockedCompareExchange (&sigExeced, 0, prev_sigExeced);
      WFMO(..., INFINITE);
    }

?


Corinna

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] Cygwin: pinfo: Fix exit code for non-cygwin apps which reads console.
  2022-02-28  9:21 ` Corinna Vinschen
@ 2022-02-28  9:32   ` Takashi Yano
  2022-02-28  9:44     ` Corinna Vinschen
  0 siblings, 1 reply; 4+ messages in thread
From: Takashi Yano @ 2022-02-28  9:32 UTC (permalink / raw)
  To: cygwin-patches

On Mon, 28 Feb 2022 10:21:32 +0100
Corinna Vinschen wrote:
> Hi Takashi,
> 
> On Feb 27 09:46, Takashi Yano wrote:
> > - The recent commit "Cygwin: pinfo: Fix exit code when non-cygwin app
> >   exits by Ctrl-C." did not fix enough the issue. If a non-cygwin app
> >   is reading the console, it will not return STATUS_CONTROL_C_EXIT
> >   even if it is terminated by Ctrl-C. As a result, the previous patch
> >   does not take effect.
> >   This patch solves this issue by setting sigExeced to SIGINT in
> >   ctrl_c_handler(). In addition, sigExeced will be cleared if the app
> >   does not terminated within predetermined time period. The reason is
> >   that the app does not seem to be terminated by the signal sigExeced.
> > [...]
> > --- a/winsup/cygwin/spawn.cc
> > +++ b/winsup/cygwin/spawn.cc
> > @@ -953,7 +953,15 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> >  	  if (sem)
> >  	    __posix_spawn_sem_release (sem, 0);
> >  	  if (ptys_need_cleanup || cons_need_cleanup)
> > -	    WaitForSingleObject (pi.hProcess, INFINITE);
> > +	    {
> > +	      LONG prev_sigExeced = sigExeced;
> > +	      while (WaitForSingleObject (pi.hProcess, 100) == WAIT_TIMEOUT)
> > +		/* If child process does not exit in predetermined time
> > +		   period, the process does not seem to be terminated by
> > +		   the signal sigExeced. Therefore, clear sigExeced here. */
> > +		prev_sigExeced =
> > +		  InterlockedCompareExchange (&sigExeced, 0, prev_sigExeced);
> > +	    }
> >  	  if (ptys_need_cleanup)
> >  	    {
> >  	      fhandler_pty_slave::cleanup_for_non_cygwin_app (&ptys_handle_set,
> 
> Is it really necessary to run the InterlockedCompareExchange in a loop?
> What about
> 
>   if (WFMO(..., 100) == WAIT_TIMEOUT)
>     {
>       InterlockedCompareExchange (&sigExeced, 0, prev_sigExeced);
>       WFMO(..., INFINITE);
>     }
> 
> ?

If non-cygwin app ignores Ctrl-C (like cmd.exe), and if
you hit Ctrl-C twice or more, sigExeced should be cleared
everytime on Ctrl-C. Your code clears sigExeced only once,
doesn't it?

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] Cygwin: pinfo: Fix exit code for non-cygwin apps which reads console.
  2022-02-28  9:32   ` Takashi Yano
@ 2022-02-28  9:44     ` Corinna Vinschen
  0 siblings, 0 replies; 4+ messages in thread
From: Corinna Vinschen @ 2022-02-28  9:44 UTC (permalink / raw)
  To: cygwin-patches

On Feb 28 18:32, Takashi Yano wrote:
> On Mon, 28 Feb 2022 10:21:32 +0100
> Corinna Vinschen wrote:
> > Hi Takashi,
> > 
> > On Feb 27 09:46, Takashi Yano wrote:
> > > - The recent commit "Cygwin: pinfo: Fix exit code when non-cygwin app
> > >   exits by Ctrl-C." did not fix enough the issue. If a non-cygwin app
> > >   is reading the console, it will not return STATUS_CONTROL_C_EXIT
> > >   even if it is terminated by Ctrl-C. As a result, the previous patch
> > >   does not take effect.
> > >   This patch solves this issue by setting sigExeced to SIGINT in
> > >   ctrl_c_handler(). In addition, sigExeced will be cleared if the app
> > >   does not terminated within predetermined time period. The reason is
> > >   that the app does not seem to be terminated by the signal sigExeced.
> > > [...]
> > > --- a/winsup/cygwin/spawn.cc
> > > +++ b/winsup/cygwin/spawn.cc
> > > @@ -953,7 +953,15 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> > >  	  if (sem)
> > >  	    __posix_spawn_sem_release (sem, 0);
> > >  	  if (ptys_need_cleanup || cons_need_cleanup)
> > > -	    WaitForSingleObject (pi.hProcess, INFINITE);
> > > +	    {
> > > +	      LONG prev_sigExeced = sigExeced;
> > > +	      while (WaitForSingleObject (pi.hProcess, 100) == WAIT_TIMEOUT)
> > > +		/* If child process does not exit in predetermined time
> > > +		   period, the process does not seem to be terminated by
> > > +		   the signal sigExeced. Therefore, clear sigExeced here. */
> > > +		prev_sigExeced =
> > > +		  InterlockedCompareExchange (&sigExeced, 0, prev_sigExeced);
> > > +	    }
> > >  	  if (ptys_need_cleanup)
> > >  	    {
> > >  	      fhandler_pty_slave::cleanup_for_non_cygwin_app (&ptys_handle_set,
> > 
> > Is it really necessary to run the InterlockedCompareExchange in a loop?
> > What about
> > 
> >   if (WFMO(..., 100) == WAIT_TIMEOUT)
> >     {
> >       InterlockedCompareExchange (&sigExeced, 0, prev_sigExeced);
> >       WFMO(..., INFINITE);
> >     }
> > 
> > ?
> 
> If non-cygwin app ignores Ctrl-C (like cmd.exe), and if
> you hit Ctrl-C twice or more, sigExeced should be cleared
> everytime on Ctrl-C. Your code clears sigExeced only once,
> doesn't it?

Oh, ok.  Then, please go ahead.


Thanks,
Corinna

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-02-28  9:44 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-27  0:46 [PATCH v2] Cygwin: pinfo: Fix exit code for non-cygwin apps which reads console Takashi Yano
2022-02-28  9:21 ` Corinna Vinschen
2022-02-28  9:32   ` Takashi Yano
2022-02-28  9:44     ` 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).