From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 96470 invoked by alias); 16 Aug 2019 14:48:44 -0000 Mailing-List: contact cygwin-help@cygwin.com; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: cygwin-owner@cygwin.com Mail-Followup-To: cygwin@cygwin.com Received: (qmail 59244 invoked by uid 89); 16 Aug 2019 14:48:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-116.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,GOOD_FROM_CORINNA_CYGWIN,KAM_NUMSUBJECT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=2868, revamped, Needed, MUST X-HELO: mout.kundenserver.de Received: from mout.kundenserver.de (HELO mout.kundenserver.de) (212.227.126.134) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 16 Aug 2019 14:48:30 +0000 Received: from calimero.vinschen.de ([24.134.7.25]) by mrelayeu.kundenserver.de (mreue009 [212.227.15.167]) with ESMTPSA (Nemesis) id 1MyseA-1iB5yR0aoc-00vxxm; Fri, 16 Aug 2019 16:48:13 +0200 Received: by calimero.vinschen.de (Postfix, from userid 500) id D9F39A80786; Fri, 16 Aug 2019 16:48:11 +0200 (CEST) Date: Fri, 16 Aug 2019 14:58:00 -0000 From: Corinna Vinschen To: Takashi Yano Cc: cygwin@cygwin.com Subject: Re: [ANNOUNCEMENT] TEST: Cygwin 3.1.0-0.1 Message-ID: <20190816144811.GW11632@calimero.vinschen.de> Reply-To: cygwin@cygwin.com Mail-Followup-To: Takashi Yano , cygwin@cygwin.com References: <20190812153613.GN11632@calimero.vinschen.de> <20190813104753.GU11632@calimero.vinschen.de> <20190814204100.659fe40d928eae15338198a7@nifty.ne.jp> <20190814204717.caf6884b1216bbeee2f586d6@nifty.ne.jp> <20190814134900.GY11632@calimero.vinschen.de> <20190815042126.7c2f0baf57b4a82f7d013f74@nifty.ne.jp> <20190815074930.GF11632@calimero.vinschen.de> <20190815103638.GO11632@calimero.vinschen.de> <20190815150436.GP11632@calimero.vinschen.de> <20190815150908.GQ11632@calimero.vinschen.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="+bwwFJPg6t3ya3US" Content-Disposition: inline In-Reply-To: <20190815150908.GQ11632@calimero.vinschen.de> User-Agent: Mutt/1.11.3 (2019-02-01) X-SW-Source: 2019-08/txt/msg00223.txt.bz2 --+bwwFJPg6t3ya3US Content-Type: multipart/mixed; boundary="Djp5PRGHu2Cmyd8M" Content-Disposition: inline --Djp5PRGHu2Cmyd8M Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-length: 3074 Hi Takashi, On Aug 15 17:09, Corinna Vinschen wrote: > On Aug 15 17:04, Corinna Vinschen wrote: > > On Aug 15 12:36, Corinna Vinschen wrote: > > > On Aug 15 09:49, Corinna Vinschen wrote: > > > > On Aug 15 04:21, Takashi Yano wrote: > > > > > On Wed, 14 Aug 2019 15:49:00 +0200 > > > > > Corinna Vinschen wrote: > > > > > > The only reason I can see is if sigwait_common() returns EINTR = because > > > > > > it was interrupted by an unrelated signal. This in turn lets t= he read() > > > > > > call fail with EINTR and that should be expected by the callers= , in > > > > > > theory. > > > > >=20 > > > > > Strangely, this problem also disappears with this patch. > > > > >=20 > > > > > diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc > > > > > index 9cf892801..82ac0674f 100644 > > > > > --- a/winsup/cygwin/select.cc > > > > > +++ b/winsup/cygwin/select.cc > > > > > @@ -1869,7 +1869,7 @@ thread_signalfd (void *arg) > > > > > switch (WaitForSingleObject (si->evt, INFINITE)) > > > > > { > > > > > case WAIT_OBJECT_0: > > > > > - tls->signalfd_select_wait =3D NULL; > > > > > + //tls->signalfd_select_wait =3D NULL; > > > > > event =3D true; > > > > > break; > > > > > default: > > > >=20 > > > > The problem with not setting signalfd_select_wait to NULL here is t= hat > > > > only a subsequent read or sigwaitinfo will do, so there's a time > > > > post-select which will reroute the signal wrongly. > > >=20 > > > Worse, thread_signalfd() closes the handle on exit, so keeping > > > signalfd_select_wait set may result in strange behaviour after select > > > returns. > > >=20 > > > > Any ideas greatly appreciated. > > [...] I now had an idea, but I'm not entirely sure if it's the right thing to do. Can you please test this? It consists of two patches, one with the revamped signalfd handling, and one with the revert of the signalfd patch I applied a couple of days ago. Quick description: I dropped signalfd_select_wait entirely. Instead, wait_sig sets or resets a manual event object to indicate if there are signals pending in the queue, even after trying to handle them the normal way. That usually means they are blocked. select() uses the event to wake up from WFMO, if at least one signalfd is present in the read descriptor set. The rest is done via the peek and verify functions in select, which basically just check if this signalfd is waiting for one of the pending signals. The reversion of my patch from a couple days ago is not required as such, but after thinking about this a while I'm convinced that this was just me not getting the full picture. Also, reverting this patch would revert to seeing a SEGV in your testcase and thus a bug in the new code, too. I attached both patches. It would be pretty nice if you could test them and point out any problems you get with this new code. Please note that you should ideally perform a full rebuild due to the slight change in TLS layout. Thanks a lot, Corinna --=20 Corinna Vinschen Cygwin Maintainer --Djp5PRGHu2Cmyd8M Content-Type: text/plain; charset=utf-8 Content-Disposition: attachment; filename="0001-Cygwin-select-revamp-non-polling-code-for-signalfd.patch" Content-Transfer-Encoding: quoted-printable Content-length: 10723 =46rom 0169458101d1c9e915f842a765a9afe4ee69867e Mon Sep 17 00:00:00 2001 From: Corinna Vinschen Date: Fri, 16 Aug 2019 16:36:06 +0200 Subject: [PATCH 1/2] Cygwin: select: revamp non-polling code for signalfd Rather than waiting for signalfd_select_wait in a thread, which is racy, create a global event "my_pendingsigs_evt" which is set and reset by wait_sig depending on the fact if signals are pending because they are blocked and no thread cares. This in turn allows to WFMO on this event in select as soon as signalfds are present in the read descriptor set, and the peek and verify functionality will then check if one of the present signalfds is affected. Signed-off-by: Corinna Vinschen --- winsup/cygwin/cygtls.h | 2 +- winsup/cygwin/exceptions.cc | 8 ---- winsup/cygwin/select.cc | 80 ++---------------------------------- winsup/cygwin/select.h | 11 +---- winsup/cygwin/signal.cc | 1 - winsup/cygwin/sigproc.cc | 17 ++++++++ winsup/cygwin/tlsoffsets.h | 16 ++++---- winsup/cygwin/tlsoffsets64.h | 16 ++++---- 8 files changed, 40 insertions(+), 111 deletions(-) diff --git a/winsup/cygwin/cygtls.h b/winsup/cygwin/cygtls.h index 65a905c32078..a2e3676fc722 100644 --- a/winsup/cygwin/cygtls.h +++ b/winsup/cygwin/cygtls.h @@ -189,8 +189,8 @@ public: stack_t altstack; siginfo_t *sigwait_info; HANDLE signal_arrived; - HANDLE signalfd_select_wait; bool will_wait_for_signal; + long __align; /* Needed to align context to 16 byte. */ /* context MUST be aligned to 16 byte, otherwise RtlCaptureContext fails. If you prepend cygtls members here, make sure context stays 16 byte aligned. */ diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc index 9bdf9f054c0a..f35411e3bb0e 100644 --- a/winsup/cygwin/exceptions.cc +++ b/winsup/cygwin/exceptions.cc @@ -1469,14 +1469,6 @@ sigpacket::process () if (issig_wait) { tls->sigwait_mask =3D 0; - /* If the catching thread is running select on a signalfd, don't call - the signal handler and don't remove the signal from the queue. */ - if (tls->signalfd_select_wait) - { - SetEvent (tls->signalfd_select_wait); - rc =3D 0; - goto done; - } goto dosig; } =20 diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc index 688859b50290..d29f3d2f4ffa 100644 --- a/winsup/cygwin/select.cc +++ b/winsup/cygwin/select.cc @@ -1867,80 +1867,6 @@ fhandler_windows::select_except (select_stuff *ss) return s; } =20 -static int start_thread_signalfd (select_record *, select_stuff *); - -static DWORD WINAPI -thread_signalfd (void *arg) -{ - select_signalfd_info *si =3D (select_signalfd_info *) arg; - bool event =3D false; - - while (!event) - { - sigset_t set =3D 0; - _cygtls *tls =3D si->start->tls; - - for (select_record *s =3D si->start; (s =3D s->next); ) - if (s->startup =3D=3D start_thread_signalfd) - set |=3D ((fhandler_signalfd *) s->fh)->get_sigset (); - set_signal_mask (tls->sigwait_mask, set); - tls->signalfd_select_wait =3D si->evt; - sig_dispatch_pending (true); - switch (WaitForSingleObject (si->evt, INFINITE)) - { - case WAIT_OBJECT_0: - tls->signalfd_select_wait =3D NULL; - event =3D true; - break; - default: - break; - } - if (si->stop_thread) - break; - if (!event) - Sleep (1L); - } - CloseHandle (si->evt); - return 0; -} - -static int -start_thread_signalfd (select_record *me, select_stuff *stuff) -{ - select_signalfd_info *si; - - if ((si =3D stuff->device_specific_signalfd)) - { - me->h =3D *si->thread; - return 1; - } - si =3D new select_signalfd_info; - si->start =3D &stuff->start; - si->stop_thread =3D false; - si->evt =3D CreateEventW (&sec_none_nih, TRUE, FALSE, NULL); - si->thread =3D new cygthread (thread_signalfd, si, "signalfdsel"); - me->h =3D *si->thread; - stuff->device_specific_signalfd =3D si; - return 1; -} - -static void -signalfd_cleanup (select_record *, select_stuff *stuff) -{ - select_signalfd_info *si; - - if (!(si =3D stuff->device_specific_signalfd)) - return; - if (si->thread) - { - si->stop_thread =3D true; - SetEvent (si->evt); - si->thread->detach (); - } - delete si; - stuff->device_specific_signalfd =3D NULL; -} - static int peek_signalfd (select_record *me, bool) { @@ -1960,17 +1886,19 @@ verify_signalfd (select_record *me, fd_set *rfds, f= d_set *wfds, fd_set *efds) return peek_signalfd (me, true); } =20 +extern HANDLE my_pendingsigs_evt; + select_record * fhandler_signalfd::select_read (select_stuff *stuff) { select_record *s =3D stuff->start.next; if (!s->startup) { - s->startup =3D start_thread_signalfd; + s->startup =3D no_startup; s->verify =3D verify_signalfd; - s->cleanup =3D signalfd_cleanup; } s->peek =3D peek_signalfd; + s->h =3D my_pendingsigs_evt; /* wait_sig sets this if signal are pending= */ s->read_selected =3D true; s->read_ready =3D false; return s; diff --git a/winsup/cygwin/select.h b/winsup/cygwin/select.h index 19f9d7dc2434..7d6dee753dba 100644 --- a/winsup/cygwin/select.h +++ b/winsup/cygwin/select.h @@ -71,12 +71,6 @@ struct select_serial_info: public select_info select_serial_info (): select_info () {} }; =20 -struct select_signalfd_info: public select_info -{ - HANDLE evt; - select_signalfd_info (): select_info () {} -}; - class select_stuff { public: @@ -97,7 +91,6 @@ public: select_fifo_info *device_specific_fifo; select_socket_info *device_specific_socket; select_serial_info *device_specific_serial; - select_signalfd_info *device_specific_signalfd; =20 bool test_and_set (int, fd_set *, fd_set *, fd_set *); int poll (fd_set *, fd_set *, fd_set *); @@ -110,8 +103,8 @@ public: device_specific_pipe (NULL), device_specific_fifo (NULL), device_specific_socket (NULL), - device_specific_serial (NULL), - device_specific_signalfd (NULL) {} + device_specific_serial (NULL) + {} }; =20 extern "C" int cygwin_select (int , fd_set *, fd_set *, fd_set *, diff --git a/winsup/cygwin/signal.cc b/winsup/cygwin/signal.cc index 920a533f3a2b..8ac59d4b9366 100644 --- a/winsup/cygwin/signal.cc +++ b/winsup/cygwin/signal.cc @@ -606,7 +606,6 @@ sigwait_common (const sigset_t *set, siginfo_t *info, P= LARGE_INTEGER waittime) __try { set_signal_mask (_my_tls.sigwait_mask, *set); - _my_tls.signalfd_select_wait =3D NULL; sig_dispatch_pending (true); =20 switch (cygwait (NULL, waittime, diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc index 8003e2db6259..91abb717c25b 100644 --- a/winsup/cygwin/sigproc.cc +++ b/winsup/cygwin/sigproc.cc @@ -57,6 +57,9 @@ _cygtls NO_COPY *_sig_tls; Static HANDLE my_sendsig; Static HANDLE my_readsig; =20 +/* Used in select if a signalfd is part of the read descriptor set */ +HANDLE NO_COPY my_pendingsigs_evt; + /* Function declarations */ static int __reg1 checkstate (waitq *); static __inline__ bool get_proc_lock (DWORD, DWORD); @@ -455,6 +458,10 @@ sigproc_init () } ProtectHandle (my_readsig); myself->sendsig =3D my_sendsig; + my_pendingsigs_evt =3D CreateEvent (NULL, TRUE, FALSE, NULL); + if (!my_pendingsigs_evt) + api_fatal ("couldn't create pending signal event, %E"); + /* sync_proc_subproc is used by proc_subproc. It serializes access to the children and proc arrays. */ sync_proc_subproc.init ("sync_proc_subproc"); @@ -1398,6 +1405,16 @@ wait_sig (VOID *) qnext->si.si_signo =3D 0; } } + /* At least one signal still queued? The event is used in select + only, and only to decide if WFMO should wake up in case a + signalfd is waiting via select/poll for being ready to read a + pending signal. This method wakes up all threads hanging in + select and having a signalfd, as soon as a pending signal is + available, but it's certainly better than constant polling. */ + if (sigq.start.next) + SetEvent (my_pendingsigs_evt); + else + ResetEvent (my_pendingsigs_evt); if (pack.si.si_signo =3D=3D SIGCHLD) clearwait =3D true; } diff --git a/winsup/cygwin/tlsoffsets.h b/winsup/cygwin/tlsoffsets.h index 8003a1fffdec..13d1003e334f 100644 --- a/winsup/cygwin/tlsoffsets.h +++ b/winsup/cygwin/tlsoffsets.h @@ -29,10 +29,10 @@ //; $tls::psigwait_info =3D 2852; //; $tls::signal_arrived =3D -9844; //; $tls::psignal_arrived =3D 2856; -//; $tls::signalfd_select_wait =3D -9840; -//; $tls::psignalfd_select_wait =3D 2860; -//; $tls::will_wait_for_signal =3D -9836; -//; $tls::pwill_wait_for_signal =3D 2864; +//; $tls::will_wait_for_signal =3D -9840; +//; $tls::pwill_wait_for_signal =3D 2860; +//; $tls::__align =3D -9836; +//; $tls::p__align =3D 2864; //; $tls::context =3D -9832; //; $tls::pcontext =3D 2868; //; $tls::thread_id =3D -9084; @@ -91,10 +91,10 @@ #define tls_psigwait_info (2852) #define tls_signal_arrived (-9844) #define tls_psignal_arrived (2856) -#define tls_signalfd_select_wait (-9840) -#define tls_psignalfd_select_wait (2860) -#define tls_will_wait_for_signal (-9836) -#define tls_pwill_wait_for_signal (2864) +#define tls_will_wait_for_signal (-9840) +#define tls_pwill_wait_for_signal (2860) +#define tls___align (-9836) +#define tls_p__align (2864) #define tls_context (-9832) #define tls_pcontext (2868) #define tls_thread_id (-9084) diff --git a/winsup/cygwin/tlsoffsets64.h b/winsup/cygwin/tlsoffsets64.h index 2e9d590c31df..d137408d0e0c 100644 --- a/winsup/cygwin/tlsoffsets64.h +++ b/winsup/cygwin/tlsoffsets64.h @@ -29,10 +29,10 @@ //; $tls::psigwait_info =3D 4144; //; $tls::signal_arrived =3D -8648; //; $tls::psignal_arrived =3D 4152; -//; $tls::signalfd_select_wait =3D -8640; -//; $tls::psignalfd_select_wait =3D 4160; -//; $tls::will_wait_for_signal =3D -8632; -//; $tls::pwill_wait_for_signal =3D 4168; +//; $tls::will_wait_for_signal =3D -8640; +//; $tls::pwill_wait_for_signal =3D 4160; +//; $tls::__align =3D -8632; +//; $tls::p__align =3D 4168; //; $tls::context =3D -8624; //; $tls::pcontext =3D 4176; //; $tls::thread_id =3D -7328; @@ -91,10 +91,10 @@ #define tls_psigwait_info (4144) #define tls_signal_arrived (-8648) #define tls_psignal_arrived (4152) -#define tls_signalfd_select_wait (-8640) -#define tls_psignalfd_select_wait (4160) -#define tls_will_wait_for_signal (-8632) -#define tls_pwill_wait_for_signal (4168) +#define tls_will_wait_for_signal (-8640) +#define tls_pwill_wait_for_signal (4160) +#define tls___align (-8632) +#define tls_p__align (4168) #define tls_context (-8624) #define tls_pcontext (4176) #define tls_thread_id (-7328) --=20 2.20.1 --Djp5PRGHu2Cmyd8M Content-Type: text/plain; charset=utf-8 Content-Disposition: attachment; filename="0002-Revert-Cygwin-fix-potential-SEGV-in-sigwaitinfo-sign.patch" Content-Transfer-Encoding: quoted-printable Content-length: 2831 =46rom 303842c059fc6d408998ccccac39bd94372ad25d Mon Sep 17 00:00:00 2001 From: Corinna Vinschen Date: Fri, 16 Aug 2019 16:36:20 +0200 Subject: [PATCH 2/2] Revert "Cygwin: fix potential SEGV in sigwaitinfo/signalfd scenario" This reverts commit 92115a83a4579635e253be2887d3706d28b477fd. This was utterly wrong. --- winsup/cygwin/exceptions.cc | 17 +++-------------- winsup/cygwin/release/3.1.0 | 3 --- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc index f35411e3bb0e..848f9bd68079 100644 --- a/winsup/cygwin/exceptions.cc +++ b/winsup/cygwin/exceptions.cc @@ -1620,7 +1620,7 @@ _cygtls::call_signal_handler () if (retaddr () =3D=3D (__tlsstack_t) sigdelayed) pop (); =20 - debug_only_printf ("dealing with signal %d, handler %p", sig, func); + debug_only_printf ("dealing with signal %d", sig); this_sa_flags =3D sa_flags; =20 sigset_t this_oldmask =3D set_process_mask_delta (); @@ -1639,12 +1639,8 @@ _cygtls::call_signal_handler () =20 ucontext_t *thiscontext =3D NULL, context_copy; =20 - /* Only make a context for SA_SIGINFO handlers, only if a handler - exists. If handler is NULL, drop SA_SIGINFO flag to avoid - accidental context access later in the function. */ - if (!thisfunc) - this_sa_flags &=3D ~SA_SIGINFO; - else if (this_sa_flags & SA_SIGINFO) + /* Only make a context for SA_SIGINFO handlers */ + if (this_sa_flags & SA_SIGINFO) { context.uc_link =3D 0; context.uc_flags =3D 0; @@ -1706,11 +1702,6 @@ _cygtls::call_signal_handler () sig =3D 0; /* Flag that we can accept another signal */ unlock (); /* unlock signal stack */ =20 - /* Handler may be NUll in sigwaitinfo/signalfd scenario. Avoid - crashing by calling a NULL function. */ - if (!thisfunc) - goto skip_calling_handler; - /* Alternate signal stack requested for this signal and alternate si= gnal stack set up for this thread? */ if (this_sa_flags & SA_ONSTACK @@ -1818,8 +1809,6 @@ _cygtls::call_signal_handler () signal handler. */ thisfunc (thissig, &thissi, thiscontext); =20 -skip_calling_handler: - incyg =3D true; =20 set_signal_mask (_my_tls.sigmask, (this_sa_flags & SA_SIGINFO) diff --git a/winsup/cygwin/release/3.1.0 b/winsup/cygwin/release/3.1.0 index c9cb7c011d7c..ccb63c317db3 100644 --- a/winsup/cygwin/release/3.1.0 +++ b/winsup/cygwin/release/3.1.0 @@ -71,6 +71,3 @@ Bug Fixes - 64 bit only: Avoid collisions between memory maps created with shmat and Windows datastructures during fork. Addresses: https://cygwin.com/ml/cygwin/2019-08/msg00107.html - -- Avoid a SEGV after using signalfd. - Addresses: https://cygwin.com/ml/cygwin/2019-08/msg00148.html --=20 2.20.1 --Djp5PRGHu2Cmyd8M-- --+bwwFJPg6t3ya3US Content-Type: application/pgp-signature; name="signature.asc" Content-length: 833 -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEoVYPmneWZnwT6kwF9TYGna5ET6AFAl1WwisACgkQ9TYGna5E T6ABMA//SjEthIuivOml+juvTdQv55rsw3RtIUZugXw5yAxWBk1cW8/ohS/GXM4/ SlE8QpVKckMR21WZwb4A3ZVydak4haslIHpg2R+vKjf6+V7IJLEPSVeTVDuQjlXJ 5EMuFG3wknF7XsQZEs3O1gJU6VYN9NCG1ulHxr6JDh2Id5lMKvYGwv21nS66Ls8e 2C3KB4Dmfsa0KBnpfA3KlLKCKKRlI7pUQTTrHYpzoBQbupmoMZRAB9VuceWS2dKY m08JpdTSAoEU+xUQCdlT2qGvduM+zD8AUbiRL1zyKcqonG6WhnRDBCl7yPmvEBXJ VtdBGanH0c+E0mNpvhddmlAn7zCES2FoW2HTsucAHn95gnXlJ6E1vPccm439trXl s6xIVBv988U2Gm0oc6x34lVe86PQ0AS6UMit94IHWdxHPFxt+in0l0/1R33TqgVp fuHanXQpVpSDb0joDGk+oKX3otlyfZnJyuTpvJGzrV8DfdDSsWLNHI+kQtzFztXl r3xvf+UWrkqgro15PlwBpNHtfjECBAFL+kJ+SvDYYK5uLY+aImByOT+m5o+fqagR GxjNEasDsLqSfWzBSktPu3L5vDWNXeZe8MgbuuhifnDoSzuXliugf/VOBxPm9T4f J1Z6S03IaO+G4XeTfdhcb+GjzFrVatZY7gjqvVanjJLhozYJ7iY= =zQYn -----END PGP SIGNATURE----- --+bwwFJPg6t3ya3US--