From 118e51be1d041b2fb7d77bba17bfd8eebb7e9307 Mon Sep 17 00:00:00 2001 From: Christopher Faylor Date: Thu, 31 Jan 2013 05:26:47 +0000 Subject: [PATCH] * DevNotes: Add entry cgf-000022. * cygtls.h (_cygtls::func): Define as a sa_sigaction style function. * exceptions.cc (sig_handle_tty_stop): Ditto. (_cygtls::interrupt_setup): Fix coercion to accommodate 'func' change. (ctrl_c_handler): Use tty kill_pgrp to send a signal. (sigpacket::process): Don't process sigflush here. (_cygtls::call_signal_handler): Reorganize to avoid a race. Always call sa_sigaction style function. * fhandler_termios.cc (is_flush_sig): Define new function. (tty_min::kill_pgrp): Handle tty flush when signal detected. (fhandler_termios::bg_check): Be slightly more paranoid about checking for valid tty. (fhandler_termios::sigflush): Don't flush unless tty owner. * fhandler_tty.cc (fhandler_pty_slave::ioctl): Use tty kill_pgrp to send signal. (fhandler_pty_master::ioctl): Ditto. * signal.cc (killsys): Delete definition. * sigproc.h (killsys): Delete declaration. * include/cygwin/signal.h (siginfo_t): Simplify union/struct nesting slightly. Implement mechanism to allow cygwin data passing. --- winsup/cygwin/ChangeLog | 23 ++++++++++++ winsup/cygwin/DevNotes | 15 ++++++++ winsup/cygwin/cygtls.h | 2 +- winsup/cygwin/exceptions.cc | 46 ++++++++---------------- winsup/cygwin/fhandler_termios.cc | 16 +++++++-- winsup/cygwin/fhandler_tty.cc | 4 +-- winsup/cygwin/include/cygwin/signal.h | 52 ++++++++++++++++----------- winsup/cygwin/signal.cc | 9 ----- winsup/cygwin/sigproc.h | 1 - 9 files changed, 100 insertions(+), 68 deletions(-) diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index 873b94346..a50875fca 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,26 @@ +2013-01-31 Christopher Faylor + + * DevNotes: Add entry cgf-000022. + * cygtls.h (_cygtls::func): Define as a sa_sigaction style function. + * exceptions.cc (sig_handle_tty_stop): Ditto. + (_cygtls::interrupt_setup): Fix coercion to accommodate 'func' change. + (ctrl_c_handler): Use tty kill_pgrp to send a signal. + (sigpacket::process): Don't process sigflush here. + (_cygtls::call_signal_handler): Reorganize to avoid a race. Always + call sa_sigaction style function. + * fhandler_termios.cc (is_flush_sig): Define new function. + (tty_min::kill_pgrp): Handle tty flush when signal detected. + (fhandler_termios::bg_check): Be slightly more paranoid about checking + for valid tty. + (fhandler_termios::sigflush): Don't flush unless tty owner. + * fhandler_tty.cc (fhandler_pty_slave::ioctl): Use tty kill_pgrp to + send signal. + (fhandler_pty_master::ioctl): Ditto. + * signal.cc (killsys): Delete definition. + * sigproc.h (killsys): Delete declaration. + * include/cygwin/signal.h (siginfo_t): Simplify union/struct nesting + slightly. Implement mechanism to allow cygwin data passing. + 2013-01-23 Christopher Faylor * miscfuncs.cc (__import_address): Check if malloc field points diff --git a/winsup/cygwin/DevNotes b/winsup/cygwin/DevNotes index e477e614a..70f4ec22a 100644 --- a/winsup/cygwin/DevNotes +++ b/winsup/cygwin/DevNotes @@ -1,3 +1,18 @@ +2013-01-31 cgf-000022 + +While researching the lftp behavior reported here: + +http://cygwin.com/ml/cygwin/2013-01/msg00390.html + +after a frenzy of rewriting sigflush handling to avoid blocking in the +signal thread (which is now and should ever have been illegal), it +dawned on me that we're not supposed to be flushing the tty input buffer +every time a signal is received. We're supposed to do this only when +the user hits a character (e.g., CTRL-C) which initiates a signal +action. So, I removed sigflush from sigpacket::process and moved it to +tc ()->kill_pgrp (). This function should only be called to send +signals related to the tty so this should have the desired effect. + 2013-01-11 cgf-000021 Apparently I got the signal handling semantics of select() wrong again diff --git a/winsup/cygwin/cygtls.h b/winsup/cygwin/cygtls.h index e2e926ff8..a5b622111 100644 --- a/winsup/cygwin/cygtls.h +++ b/winsup/cygwin/cygtls.h @@ -178,7 +178,7 @@ public: char __dontuse[8 * ((sizeof(struct _reent) + 4) / 8)]; }; /**/ - void (*func) /*gentls_offsets*/(int)/*gentls_offsets*/; + void (*func) /*gentls_offsets*/(int, siginfo_t *, void *)/*gentls_offsets*/; int saved_errno; int sa_flags; sigset_t oldmask; diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc index 56e9bc106..b3e327779 100644 --- a/winsup/cygwin/exceptions.cc +++ b/winsup/cygwin/exceptions.cc @@ -688,7 +688,7 @@ extern DWORD exec_exit; // Possible exit value for exec extern "C" { static void -sig_handle_tty_stop (int sig) +sig_handle_tty_stop (int sig, siginfo_t *, void *) { _my_tls.incyg = 1; /* Silently ignore attempts to suspend if there is no accommodating @@ -748,7 +748,7 @@ _cygtls::interrupt_setup (siginfo_t& si, void *handler, struct sigaction& siga) push ((__stack_t) sigdelayed); deltamask = siga.sa_mask & ~SIG_NONMASKABLE; sa_flags = siga.sa_flags; - func = (void (*) (int)) handler; + func = (void (*) (int, siginfo_t *, void *)) handler; if (siga.sa_flags & SA_RESETHAND) siga.sa_handler = SIG_DFL; saved_errno = -1; // Flag: no errno to save @@ -956,7 +956,7 @@ ctrl_c_handler (DWORD type) && t->ti.c_cc[VINTR] == 3 && t->ti.c_cc[VQUIT] == 3) sig = SIGQUIT; t->last_ctrl_c = GetTickCount (); - killsys (-myself->pid, sig); + t->kill_pgrp (sig); t->last_ctrl_c = GetTickCount (); return TRUE; } @@ -1145,19 +1145,6 @@ sigpacket::process () sig_clear (SIGTTOU); } - switch (si.si_signo) - { - case SIGINT: - case SIGQUIT: - case SIGSTOP: - case SIGTSTP: - if (cygheap->ctty) - cygheap->ctty->sigflush (); - break; - default: - break; - } - int rc = 1; sigproc_printf ("signal %d processing", si.si_signo); @@ -1301,28 +1288,23 @@ _cygtls::call_signal_handler () debug_only_printf ("dealing with signal %d", sig); this_sa_flags = sa_flags; + + /* Save information locally on stack to pass to handler. */ int thissig = sig; - void (*thisfunc) (int) = func; + siginfo_t thissi = infodata; + void (*thisfunc) (int, siginfo_t *, void *) = func; sigset_t this_oldmask = set_process_mask_delta (); int this_errno = saved_errno; - sig = 0; + sig = 0; /* Flag that we can accept another signal */ reset_signal_arrived (); - unlock (); // make sure synchronized - if (!(this_sa_flags & SA_SIGINFO)) - { - incyg = false; - thisfunc (thissig); - } - else - { - siginfo_t thissi = infodata; - void (*sigact) (int, siginfo_t *, void *) = (void (*) (int, siginfo_t *, void *)) thisfunc; - /* no ucontext_t information provided yet */ - incyg = false; - sigact (thissig, &thissi, NULL); - } + unlock (); /* unlock signal stack */ + + incyg = false; + /* no ucontext_t information provided yet, so third arg is NULL */ + thisfunc (thissig, &thissi, NULL); incyg = true; + set_signal_mask (_my_tls.sigmask, this_oldmask); if (this_errno >= 0) set_errno (this_errno); diff --git a/winsup/cygwin/fhandler_termios.cc b/winsup/cygwin/fhandler_termios.cc index bd75ee804..1383d5e37 100644 --- a/winsup/cygwin/fhandler_termios.cc +++ b/winsup/cygwin/fhandler_termios.cc @@ -114,14 +114,23 @@ fhandler_pty_master::tcgetpgrp () return tc ()->pgid; } +static inline bool +is_flush_sig (int sig) +{ + return sig == SIGINT || sig == SIGQUIT || sig == SIGTSTP; +} + void tty_min::kill_pgrp (int sig) { bool killself = false; + if (is_flush_sig (sig) && cygheap->ctty) + cygheap->ctty->sigflush (); winpids pids ((DWORD) PID_MAP_RW); siginfo_t si = {0}; si.si_signo = sig; si.si_code = SI_KERNEL; + for (unsigned i = 0; i < pids.npids; i++) { _pinfo *p = pids[i]; @@ -163,7 +172,7 @@ tty_min::is_orphaned_process_group (int pgid) bg_check_types fhandler_termios::bg_check (int sig) { - if (!myself->pgid || tc ()->getpgid () == myself->pgid || + if (!myself->pgid || !tc () || tc ()->getpgid () == myself->pgid || myself->ctty != tc ()->ntty || ((sig == SIGTTOU) && !(tc ()->ti.c_lflag & TOSTOP))) return bg_ok; @@ -396,8 +405,9 @@ fhandler_termios::sigflush () /* FIXME: Checking get_ttyp() for NULL is not right since it should not be NULL while this is alive. However, we can conceivably close a ctty while exiting and that will zero this. */ - if ((!have_execed || have_execed_cygwin) && get_ttyp () - && !(get_ttyp ()->ti.c_lflag & NOFLSH)) + if ((!have_execed || have_execed_cygwin) && tc () + && (tc ()->getpgid () == myself->pgid) + && !(tc ()->ti.c_lflag & NOFLSH)) tcflush (TCIFLUSH); } diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc index 06a64739d..e34707944 100644 --- a/winsup/cygwin/fhandler_tty.cc +++ b/winsup/cygwin/fhandler_tty.cc @@ -1053,7 +1053,7 @@ fhandler_pty_slave::ioctl (unsigned int cmd, void *arg) { get_ttyp ()->arg.winsize = *(struct winsize *) arg; get_ttyp ()->winsize = *(struct winsize *) arg; - killsys (-get_ttyp ()->getpgid (), SIGWINCH); + get_ttyp ()->kill_pgrp (SIGWINCH); } break; } @@ -1425,7 +1425,7 @@ fhandler_pty_master::ioctl (unsigned int cmd, void *arg) || get_ttyp ()->winsize.ws_col != ((struct winsize *) arg)->ws_col) { get_ttyp ()->winsize = *(struct winsize *) arg; - killsys (-get_ttyp ()->getpgid (), SIGWINCH); + get_ttyp ()->kill_pgrp (SIGWINCH); } break; case TIOCGPGRP: diff --git a/winsup/cygwin/include/cygwin/signal.h b/winsup/cygwin/include/cygwin/signal.h index 31fdf1296..268b5eb74 100644 --- a/winsup/cygwin/include/cygwin/signal.h +++ b/winsup/cygwin/include/cygwin/signal.h @@ -1,7 +1,7 @@ /* signal.h - Copyright 2003, 2004, 2005, 2006, 2007, 2008, 2010, 2011, 2012, 2013 Red - Hat, Inc. + Copyright 2003, 2004, 2005, 2006, 2007, 2008, 2010, 2011, 2012, 2013 + Red Hat, Inc. This file is part of Cygwin. @@ -90,6 +90,19 @@ struct _sigcommune }; }; +#define __SI_PAD_SIZE 32 +#ifdef __INSIDE_CYGWIN__ +# ifndef max +# define max(a,b) (((a) > (b)) ? (a) : (b)) +# endif /*max*/ +# define __uint32_size(__x) (max(sizeof (__x) / sizeof (uint32_t), 1)) + +/* This padding represents the elements of the last struct in siginfo_t, + aligning the elements to the end to avoid conflicts with other struct + members. */ +# define __SI_CYG_PAD (__SI_PAD_SIZE - __uint32_size (void *)) +#endif /*__INSIDE_CYGWIN__*/ + typedef struct { int si_signo; /* signal number */ @@ -100,26 +113,21 @@ typedef struct __extension__ union { - __uint32_t __pad[32]; /* plan for future growth */ + __uint32_t __pad[__SI_PAD_SIZE]; /* plan for future growth */ struct _sigcommune _si_commune; /* cygwin ipc */ - __extension__ union + __extension__ struct { - /* timers */ - struct + __extension__ union { - union - { - struct - { - timer_t si_tid; /* timer id */ - unsigned int si_overrun; /* overrun count */ - }; - sigval_t si_sigval; /* signal value */ - sigval_t si_value; /* signal value */ - }; + sigval_t si_sigval; /* signal value */ + sigval_t si_value; /* signal value */ + }; + __extension__ struct + { + timer_t si_tid; /* timer id */ + unsigned int si_overrun; /* overrun count */ }; }; - /* SIGCHLD */ __extension__ struct { @@ -128,13 +136,17 @@ typedef struct clock_t si_stime; /* system time */ }; - __extension__ struct + void *si_addr; /* faulting address for core dumping + signals */ + /* Cygwin internal fields */ +#ifdef __INSIDE_CYGWIN__ + __extension__ struct { - /* core dumping signals */ - void *si_addr; /* faulting address */ + __uint32_t __pad2[__SI_CYG_PAD]; /* Locate at end of struct */ void *si_cyg; /* pointer to block containing cygwin-special info */ }; +#endif /*__INSIDE_CYGWIN__*/ }; } siginfo_t; #pragma pack(pop) diff --git a/winsup/cygwin/signal.cc b/winsup/cygwin/signal.cc index 444d56a34..c99220d35 100644 --- a/winsup/cygwin/signal.cc +++ b/winsup/cygwin/signal.cc @@ -299,15 +299,6 @@ kill0 (pid_t pid, siginfo_t& si) return (pid > 0) ? pinfo (pid)->kill (si) : kill_pgrp (-pid, si); } -int -killsys (pid_t pid, int sig) -{ - siginfo_t si = {0}; - si.si_signo = sig; - si.si_code = SI_KERNEL; - return kill0 (pid, si); -} - int kill (pid_t pid, int sig) { diff --git a/winsup/cygwin/sigproc.h b/winsup/cygwin/sigproc.h index df10ed30c..9c70c2c2d 100644 --- a/winsup/cygwin/sigproc.h +++ b/winsup/cygwin/sigproc.h @@ -80,7 +80,6 @@ void __stdcall signal_fixup_after_exec (); void __stdcall sigalloc (); int kill_pgrp (pid_t, siginfo_t&); -int killsys (pid_t, int); void __reg1 exit_thread (DWORD) __attribute__ ((noreturn)); void __reg1 setup_signal_exit (int);