poll()/select() on a tty fd in a backgrounded process raises SIGTTIN

Jon Turney jon.turney@dronecode.org.uk
Tue Jul 26 21:51:00 GMT 2016


On 25/07/2016 17:55, Corinna Vinschen wrote:
> On Jul 25 17:52, Jon Turney wrote:
>>>> This test case is reduced from a problem seen with gdb, since commit
>>>> 0b333c5e, where gdb stops with SIGTTIN when the inferior is started.
>>>
>>> Did you find out where it stops and do you have any ideas how we could
>>> fix this?
>>
>> Um.
>>
>> gdb stops because a poll() on stdin (even when stdin is not ready to read)
>> while backgrounded causes a SIGTTIN to be raised.
>>
>> I think this caused by select.cc:peek_console/peek_pipe calling
>> fhandler_termios::bg_check(SIGTTIN).
>>
>> My idea for fixing it is to remove that behaviour from cygwin, assuming I'm
>> right in thinking that the behaviour is incorrect, although I'm wary of
>> touching something that's so old, and I don't understand why that call to
>> bg_check is in there at all...

So, this is presumably so that the exception conditions which bg_check() 
knows about make the fd ready, irrespective of if peeking it shows some 
input.

> This may be an (old) misunderstanding how that's supposed to work.
>
> Just removing sounds a bit dangerous, but we can try it.  Maybe it
> just isn't necessary, but we need to check if it doesn't break
> something else.  Like, say, not stopping at all anymore...

Attached is the patch as discussed on IRC, with some additional commentary.

After writing these comments, it's perhaps a bit clearer to me that 
bg_check() could be re-written to take a bg_check_operation enum (read, 
write, change_settings, peek), rather than trying to encode that 
operation as a signal number, but that might be changing things too much.

Also, looking at the uses of bg_check(), it's odd that 
handler_console::ioctl for TIOCSWINSZ calls bg_check (SIGTTOU), which 
should probably be -SIGTTOU, otherwise a background process could resize 
a console window without causing SIGTTOU.  But then that operation 
doesn't seem to be implemented for console, but is for ttys, but it 
doesn't use bg_check...

-------------- next part --------------
From 2b1c9bd4fb73b514db69023daf031c7e0f6b176c Mon Sep 17 00:00:00 2001
From: Corinna Vinschen <corinna@vinschen.de>
Date: Tue, 26 Jul 2016 19:03:07 +0100
Subject: [PATCH] Don't raise SIGTTIN from poll/select

SIGTTIN should be raised when read() is made on a tty in a backgrounded
process, but not when it's tested with poll()/select().

I guess poll()/select() does need to call bg_check(), in order to detect the
error conditions that notices (that is, if bg_check() returns bg_eof or
bg_error, then fd is ready as an error condition exists) so add an optional
parameter to fhandler_base::bg_select() to indicate that signals aren't
desired.

See https://cygwin.com/ml/cygwin-developers/2016-07/msg00004.html
---
 winsup/cygwin/fhandler.h          |  4 ++--
 winsup/cygwin/fhandler_termios.cc | 43 +++++++++++++++++++++++++++++++++------
 winsup/cygwin/select.cc           |  4 ++--
 3 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 9f9d9bd..3321523 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -427,7 +427,7 @@ public:
   {
     return dev ().native ();
   }
-  virtual bg_check_types bg_check (int) {return bg_ok;}
+  virtual bg_check_types bg_check (int, bool = false) {return bg_ok;}
   void clear_readahead ()
   {
     raixput = raixget = ralen = rabuflen = 0;
@@ -1233,7 +1233,7 @@ class fhandler_termios: public fhandler_base
   void sigflush ();
   int tcgetpgrp ();
   int tcsetpgrp (int pid);
-  bg_check_types bg_check (int sig);
+  bg_check_types bg_check (int sig, bool dontsignal = false);
   virtual DWORD __acquire_output_mutex (const char *fn, int ln, DWORD ms) {return 1;}
   virtual void __release_output_mutex (const char *fn, int ln) {}
   void echo_erase (int force = 0);
diff --git a/winsup/cygwin/fhandler_termios.cc b/winsup/cygwin/fhandler_termios.cc
index 8bbe301..dc8a19b 100644
--- a/winsup/cygwin/fhandler_termios.cc
+++ b/winsup/cygwin/fhandler_termios.cc
@@ -166,14 +166,43 @@ tty_min::is_orphaned_process_group (int pgid)
   return 1;
 }
 
+/*
+  bg_check: check that this process is either in the foreground process group,
+  or if the terminal operation is allowed for processes which are in a
+  background process group.
+
+  If the operation is not permitted by the terminal configuration for processes
+  which are a member of a background process group, return an error or raise a
+  signal as appropriate.
+
+  This handles the following terminal operations:
+
+  write:                             sig = SIGTTOU
+  read:                              sig = SIGTTIN
+  change terminal settings:          sig = -SIGTTOU
+  (tcsetattr, tcsetpgrp, etc.)
+  peek (poll, select):               sig = SIGTTIN, dontsignal = TRUE
+*/
 bg_check_types
-fhandler_termios::bg_check (int sig)
+fhandler_termios::bg_check (int sig, bool dontsignal)
 {
+  /* Ignore errors:
+     - this process isn't in a process group
+     - tty is invalid
+
+     Everything is ok if:
+     - this process is in the foreground process group, or
+     - this tty is not the controlling tty for this process (???), or
+     - writing, when TOSTOP TTY mode is not set on this tty
+  */
   if (!myself->pgid || !tc () || tc ()->getpgid () == myself->pgid ||
 	myself->ctty != tc ()->ntty ||
 	((sig == SIGTTOU) && !(tc ()->ti.c_lflag & TOSTOP)))
     return bg_ok;
 
+  /* sig -SIGTTOU is used to indicate a change to terminal settings, where
+     TOSTOP TTY mode isn't considered when determining if we need to send a
+     signal. */
   if (sig < 0)
     sig = -sig;
 
@@ -197,19 +226,20 @@ fhandler_termios::bg_check (int sig)
     (_main_tls->sigmask & SIGTOMASK (sig));
   cygheap->unlock_tls (tl_entry);
 
-  /* If the process is ignoring SIGTT*, then background IO is OK.  If
-     the process is not ignoring SIGTT*, then the sig is to be sent to
-     all processes in the process group (unless the process group of the
-     process is orphaned, in which case we return EIO). */
+  /* If the process is blocking or ignoring SIGTT*, then signals are not sent
+     and background IO is allowed */
   if (sigs_ignored)
     return bg_ok;   /* Just allow the IO */
+  /* If the process group of the process is orphaned, return EIO */
   else if (tc ()->is_orphaned_process_group (myself->pgid))
     {
       termios_printf ("process group is orphaned");
       set_errno (EIO);   /* This is an IO error */
       return bg_error;
     }
-  else
+  /* Otherwise, if signalling is desired, the signal is sent to all processes in
+     the process group */
+  else if (!dontsignal)
     {
       /* Don't raise a SIGTT* signal if we have already been
 	 interrupted by another signal. */
@@ -222,6 +252,7 @@ fhandler_termios::bg_check (int sig)
 	}
       return bg_signalled;
     }
+  return bg_ok;
 }
 
 #define set_input_done(x) input_done = input_done || (x)
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 217544e..5789737 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -645,7 +645,7 @@ peek_pipe (select_record *s, bool from_select)
 	    }
 	}
 
-      if (fh->bg_check (SIGTTIN) <= bg_eof)
+      if (fh->bg_check (SIGTTIN, true) <= bg_eof)
 	{
 	  gotone = s->read_ready = true;
 	  goto out;
@@ -884,7 +884,7 @@ peek_console (select_record *me, bool)
   set_handle_or_return_if_not_open (h, me);
 
   for (;;)
-    if (fh->bg_check (SIGTTIN) <= bg_eof)
+    if (fh->bg_check (SIGTTIN, true) <= bg_eof)
       return me->read_ready = true;
     else if (!PeekConsoleInput (h, &irec, 1, &events_read) || !events_read)
       break;
-- 
2.8.3



More information about the Cygwin-developers mailing list