%SIG handlers BLOCK/UNBLOCK signals rather than BLOCK+save / restore

[p5pRT]

Migrated from rt.perl.org#82040 (status was 'resolved')

Searchable as RT82040$

[p5pRT]

From @leonerd

Created by @leonerd

When running the code in a $SIG{CHLD} slot\, Perl does the following​:

rt_sigprocmask(SIG_BLOCK\, [CHLD]\, NULL\, 8) = 0 rt_sigaction(SIGCHLD\, NULL\, {0x80c5380\, []\, 0}\, 8) = 0 ... { code body here } rt_sigprocmask(SIG_UNBLOCK\, [CHLD]\, NULL\, 8) = 0

My process is running with SIGCHLD blocked (set by POSIX​::sigprocmask()); and should receive these signals only during the brief moment the mask is unlocked during a ppoll(2) or epoll_pwait(2) call; not at any other time.

Problem is\, the first time a SIGCHLD is recieved\, the above sequence manages to unblock SIGCHLD again\, meaning the signal could arrive at any time. My $SIG{CHLD} code is written with the assumption that it would only be invoked during such a ppoll(2) call.

Instead\, the mask should be saved by the first call\, and restored by the second; looking something like​:

rt_sigprocmask(SIG_BLOCK\, [CHLD]\, [CHLD]\, 8) = 0 rt_sigaction(....) ... { code body here } rt_sigprocmask(SIG_SETMASK\, [CHLD]\, NULL\, 8) = 0

Perl Info

``` Flags: category=core severity=low Site configuration information for perl 5.10.1: Configured by Debian Project at Tue Nov 2 09:44:07 UTC 2010. Summary of my perl5 (revision 5 version 10 subversion 1) configuration: Platform: osname=linux, osvers=2.6.32.25-dsa-ia32, archname=i486-linux-gnu-thread-multi uname='linux murphy 2.6.32.25-dsa-ia32 #1 smp fri oct 29 10:49:58 cest 2010 i686 gnulinux ' config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN -Dcccdlflags=-fPIC -Darchname=i486-linux-gnu -Dprefix=/usr -Dprivlib=/usr/share/perl/5.10 -Darchlib=/usr/lib/perl/5.10 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.10.1 -Dsitearch=/usr/local/lib/perl/5.10.1 -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3 -Dman1ext=1 -Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Ud_ualarm -Uusesfio -Uusenm -DDEBUGGING=-g -Doptimize=-O2 -Duseshrplib -Dlibperl=libperl.so.5.10.1 -Dd_dosuid -des' hint=recommended, useposix=true, d_sigaction=define useithreads=define, usemultiplicity=define useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef use64bitint=undef, use64bitall=undef, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64', optimize='-O2 -g', cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion='', gccversion='4.4.5', gccosandvers='' intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12 ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=4, prototype=define Linker and Libraries: ld='cc', ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /lib /usr/lib /usr/lib64 libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt perllibs=-ldl -lm -lpthread -lc -lcrypt libc=/lib/libc-2.11.2.so, so=so, useshrplib=true, libperl=libperl.so.5.10.1 gnulibc_version='2.11.2' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E' cccdlflags='-fPIC', lddlflags='-shared -O2 -g -L/usr/local/lib -fstack-protector' Locally applied patches: DEBPKG:debian/arm_thread_stress_timeout - http://bugs.debian.org/501970 Raise the timeout of ext/threads/shared/t/stress.t to accommodate slower build hosts DEBPKG:debian/cpan_config_path - Set location of CPAN::Config to /etc/perl as /usr may not be writable. DEBPKG:debian/cpan_definstalldirs - Provide a sensible INSTALLDIRS default for modules installed from CPAN. DEBPKG:debian/db_file_ver - http://bugs.debian.org/340047 Remove overly restrictive DB_File version check. DEBPKG:debian/doc_info - Replace generic man(1) instructions with Debian-specific information. DEBPKG:debian/enc2xs_inc - http://bugs.debian.org/290336 Tweak enc2xs to follow symlinks and ignore missing @INC directories. DEBPKG:debian/errno_ver - http://bugs.debian.org/343351 Remove Errno version check due to upgrade problems with long-running processes. DEBPKG:debian/extutils_hacks - Various debian-specific ExtUtils changes DEBPKG:debian/fakeroot - Postpone LD_LIBRARY_PATH evaluation to the binary targets. DEBPKG:debian/instmodsh_doc - Debian policy doesn't install .packlist files for core or vendor. DEBPKG:debian/ld_run_path - Remove standard libs from LD_RUN_PATH as per Debian policy. DEBPKG:debian/libnet_config_path - Set location of libnet.cfg to /etc/perl/Net as /usr may not be writable. DEBPKG:debian/m68k_thread_stress - http://bugs.debian.org/495826 Disable some threads tests on m68k for now due to missing TLS. DEBPKG:debian/mod_paths - Tweak @INC ordering for Debian DEBPKG:debian/module_build_man_extensions - http://bugs.debian.org/479460 Adjust Module::Build manual page extensions for the Debian Perl policy DEBPKG:debian/perl_synopsis - http://bugs.debian.org/278323 Rearrange perl.pod DEBPKG:debian/prune_libs - http://bugs.debian.org/128355 Prune the list of libraries wanted to what we actually need. DEBPKG:debian/use_gdbm - Explicitly link against -lgdbm_compat in ODBM_File/NDBM_File. DEBPKG:fixes/assorted_docs - http://bugs.debian.org/443733 [384f06a] Math::BigInt::CalcEmu documentation grammar fix DEBPKG:fixes/net_smtp_docs - http://bugs.debian.org/100195 [rt.cpan.org #36038] Document the Net::SMTP 'Port' option DEBPKG:fixes/processPL - http://bugs.debian.org/357264 [rt.cpan.org #17224] Always use PERLRUNINST when building perl modules. DEBPKG:debian/perlivp - http://bugs.debian.org/510895 Make perlivp skip include directories in /usr/local DEBPKG:fixes/pod2man-index-backslash - http://bugs.debian.org/521256 Escape backslashes in .IX entries DEBPKG:debian/disable-zlib-bundling - Disable zlib bundling in Compress::Raw::Zlib DEBPKG:fixes/kfreebsd_cppsymbols - http://bugs.debian.org/533098 [3b910a0] Add gcc predefined macros to $Config{cppsymbols} on GNU/kFreeBSD. DEBPKG:debian/cpanplus_definstalldirs - http://bugs.debian.org/533707 Configure CPANPLUS to use the site directories by default. DEBPKG:debian/cpanplus_config_path - Save local versions of CPANPLUS::Config::System into /etc/perl. DEBPKG:fixes/kfreebsd-filecopy-pipes - http://bugs.debian.org/537555 [16f708c] Fix File::Copy::copy with pipes on GNU/kFreeBSD DEBPKG:fixes/anon-tmpfile-dir - http://bugs.debian.org/528544 [perl #66452] Honor TMPDIR when open()ing an anonymous temporary file DEBPKG:fixes/abstract-sockets - http://bugs.debian.org/329291 [89904c0] Add support for Abstract namespace sockets. DEBPKG:fixes/hurd_cppsymbols - http://bugs.debian.org/544307 [eeb92b7] Add gcc predefined macros to $Config{cppsymbols} on GNU/Hurd. DEBPKG:fixes/autodie-flock - http://bugs.debian.org/543731 Allow for flock returning EAGAIN instead of EWOULDBLOCK on linux/parisc DEBPKG:fixes/archive-tar-instance-error - http://bugs.debian.org/539355 [rt.cpan.org #48879] Separate Archive::Tar instance error strings from each other DEBPKG:fixes/positive-gpos - http://bugs.debian.org/545234 [perl #69056] [c584a96] Fix \\G crash on first match DEBPKG:debian/devel-ppport-ia64-optim - http://bugs.debian.org/548943 Work around an ICE on ia64 DEBPKG:fixes/trie-logic-match - http://bugs.debian.org/552291 [perl #69973] [0abd0d7] Fix a DoS in Unicode processing [CVE-2009-3626] DEBPKG:fixes/hppa-thread-eagain - http://bugs.debian.org/554218 make the threads-shared test suite more robust, fixing failures on hppa DEBPKG:fixes/crash-on-undefined-destroy - http://bugs.debian.org/564074 [perl #71952] [1f15e67] Fix a NULL pointer dereference when looking for a DESTROY method DEBPKG:fixes/tainted-errno - http://bugs.debian.org/574129 [perl #61976] [be1cf43] fix an errno stringification bug in taint mode DEBPKG:fixes/safe-upgrade - http://bugs.debian.org/582978 Upgrade Safe.pm to 2.25, fixing CVE-2010-1974 DEBPKG:fixes/tell-crash - http://bugs.debian.org/578577 [f4817f3] Fix a tell() crash on bad arguments. DEBPKG:fixes/format-write-crash - http://bugs.debian.org/579537 [perl #22977] [421f30e] Fix a crash in format/write DEBPKG:fixes/arm-alignment - http://bugs.debian.org/289884 [f1c7503] Prevent gcc from optimizing the alignment test away on armel DEBPKG:fixes/fcgi-test - Fix a failure in CGI/t/fast.t when FCGI is installed DEBPKG:fixes/hurd-ccflags - http://bugs.debian.org/587901 Make hints/gnu.sh append to $ccflags rather than overriding them DEBPKG:debian/squelch-locale-warnings - http://bugs.debian.org/508764 Squelch locale warnings in Debian package maintainer scripts DEBPKG:fixes/lc-numeric-docs - http://bugs.debian.org/379329 [perl #78452] [903eb63] LC_NUMERIC documentation fixes DEBPKG:fixes/lc-numeric-sprintf - http://bugs.debian.org/601549 [perl #78632] [b3fd614] Fix sprintf not to ignore LC_NUMERIC with constants DEBPKG:fixes/concat-stack-corruption - http://bugs.debian.org/596105 [perl #78674] [e3393f5] Fix stack pointer corruption in pp_concat() with 'use encoding' DEBPKG:patchlevel - http://bugs.debian.org/567489 List packaged patches for 5.10.1-16 in patchlevel.h @INC for perl 5.10.1: /home/leo/lib/perl5/i486-linux-gnu-thread-multi /home/leo/lib/perl5 /etc/perl /usr/local/lib/perl/5.10.1 /usr/local/share/perl/5.10.1 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.10 /usr/share/perl/5.10 /usr/local/lib/site_perl . Environment for perl 5.10.1: HOME=/home/leo LANG=en_GB.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH=/home/leo/lib:/home/leo/lib LOGDIR (unset) PATH=/home/leo/bin:/home/leo/perl5/perlbrew/bin:/home/leo/perl5/perlbrew/perls/current/bin:/usr/local/bin:/usr/bin:/bin:/usr/bin/X11:/usr/games PERL5LIB=/home/leo/lib/perl5 PERL_BADLANG (unset) SHELL=/bin/bash ```

[p5pRT]

From zefram@fysh.org

Paul LeoNerd Evans wrote​:

Instead\, the mask should be saved by the first call\, and restored by the second; looking something like​:

Restore the whole mask? What if you want to change the blocking status of a different signal\, in the body of this signal handler?

-zefram

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @Leont

On Tue\, Jan 11\, 2011 at 1​:23 PM\, Zefram \zefram@​fysh\.org wrote​:

Paul LeoNerd Evans wrote​: Restore the whole mask?  What if you want to change the blocking status of a different signal\, in the body of this signal handler?

How about this?

int was_blocked; sigset_t new\, old;

sigemptyset(&new); sigaddset(&new\, sig);

sigprocmask(SIG_BLOCK\, &new\, &old) = 0; was_blocked = sigismember(&old\, signal); ... { code body here } if (!was_blocked) {   sigprocmask(SIG_UNBLOCK\, new\, NULL); }

Signals can't block themselves that way\, but they couldn't do that before either anyway.

Leon

[p5pRT]

From @leonerd

On Tue\, Jan 11\, 2011 at 01​:55​:34PM +0100\, Leon Timmermans wrote​:

On Tue\, Jan 11\, 2011 at 1​:23 PM\, Zefram \zefram@​fysh\.org wrote​:

Paul LeoNerd Evans wrote​: Restore the whole mask?  What if you want to change the blocking status of a different signal\, in the body of this signal handler?

Ah\, an excellent point.

How about this?

int was_blocked; sigset_t new\, old;

sigemptyset(&new); sigaddset(&new\, sig);

sigprocmask(SIG_BLOCK\, &new\, &old) = 0; was_blocked = sigismember(&old\, signal); ... { code body here } if (!was_blocked) { sigprocmask(SIG_UNBLOCK\, new\, NULL); }

Signals can't block themselves that way\, but they couldn't do that before either anyway.

That looks much better yes.

Is there a plan for fixing this? Does anyone want to have a stab at fixing it (Leon's suggestion there looks good)\, or should I have a go?

-- Paul "LeoNerd" Evans

leonerd@​leonerd.org.uk ICQ# 4135350 | Registered Linux# 179460 http​://www.leonerd.org.uk/

[p5pRT]

From @Leont

On Wed\, Jan 12\, 2011 at 5​:23 PM\, Paul LeoNerd Evans \leonerd@​leonerd\.org\.uk wrote​:

Is there a plan for fixing this? Does anyone want to have a stab at fixing it (Leon's suggestion there looks good)\, or should I have a go?

I attached a quick patch\, but haven't written tests yet. I've skipped on the PERL_BLOCKSIG_* macro's because quite frankly they weren't helpful at all. Possibly they should be removed\, they aren't used anywhere else in core.

On a related issue\, most uses of sigprocmask (not just this one) should probably be replaced by a macro that uses pthread_sigmask on threaded perls\, as sigprocmask is unspecified in multi-threading applications. This probably requires a probe though.

Leon

[p5pRT]

From @Leont

sigproc.patch

```diff diff --git a/mg.c b/mg.c index a6912a0..31caf08 100644 --- a/mg.c +++ b/mg.c @@ -1443,15 +1443,25 @@ Perl_despatch_signals(pTHX) for (sig = 1; sig < SIG_SIZE; sig++) { if (PL_psig_pend[sig]) { dSAVE_ERRNO; - PERL_BLOCKSIG_ADD(set, sig); +#if defined PERL_BLOCK_SIGNALS + int was_blocked; + sigset_t new, old; + + sigemptyset(&new); + sigaddset(&new, sig); + sigprocmask(SIG_BLOCK, &new, &old); + was_blocked = sigismember(&old, sig); +#endif PL_psig_pend[sig] = 0; - PERL_BLOCKSIG_BLOCK(set); #if defined(HAS_SIGACTION) && defined(SA_SIGINFO) (*PL_sighandlerp)(sig, NULL, NULL); #else (*PL_sighandlerp)(sig); #endif - PERL_BLOCKSIG_UNBLOCK(set); +#ifdef PERL_BLOCK_SIGNALS + if (!was_blocked) + sigprocmask(SIG_UNBLOCK, &new, NULL); +#endif RESTORE_ERRNO; } } ```

[p5pRT]

From @nwc10

On Wed\, Jan 12\, 2011 at 06​:45​:58PM +0100\, Leon Timmermans wrote​:

On Wed\, Jan 12\, 2011 at 5​:23 PM\, Paul LeoNerd Evans \leonerd@&#8203;leonerd\.org\.uk wrote​:

Is there a plan for fixing this? Does anyone want to have a stab at fixing it (Leon's suggestion there looks good)\, or should I have a go?

I attached a quick patch\, but haven't written tests yet. I've skipped on the PERL_BLOCKSIG_* macro's because quite frankly they weren't helpful at all. Possibly they should be removed\, they aren't used anywhere else in core.

They aren't used anywhere else​:

$ ack PERL_BLOCKSIG_ `cat ../PERL_BLOCKSIG_ ` Convert-Binary-C/tests/include/perlinc/perl.h 5570​:# define PERL_BLOCKSIG_ADD(set\,sig) \ 5572​:# define PERL_BLOCKSIG_BLOCK(set) \ 5574​:# define PERL_BLOCKSIG_UNBLOCK(set) \ 5580​:#ifndef PERL_BLOCKSIG_ADD 5581​:# define PERL_BLOCKSIG_ADD(set\, sig) NOOP 5583​:#ifndef PERL_BLOCKSIG_BLOCK 5584​:# define PERL_BLOCKSIG_BLOCK(set) NOOP 5586​:#ifndef PERL_BLOCKSIG_UNBLOCK 5587​:# define PERL_BLOCKSIG_UNBLOCK(set) NOOP

perl/mg.c 1379​: PERL_BLOCKSIG_ADD(set\, sig); 1381​: PERL_BLOCKSIG_BLOCK(set); 1387​: PERL_BLOCKSIG_UNBLOCK(set);

perl/perl.h 6026​:# define PERL_BLOCKSIG_ADD(set\,sig) \ 6028​:# define PERL_BLOCKSIG_BLOCK(set) \ 6030​:# define PERL_BLOCKSIG_UNBLOCK(set) \ 6036​:#ifndef PERL_BLOCKSIG_ADD 6037​:# define PERL_BLOCKSIG_ADD(set\, sig) NOOP 6039​:#ifndef PERL_BLOCKSIG_BLOCK 6040​:# define PERL_BLOCKSIG_BLOCK(set) NOOP 6042​:#ifndef PERL_BLOCKSIG_UNBLOCK 6043​:# define PERL_BLOCKSIG_UNBLOCK(set) NOOP

Git blame shows that they were moved to perl.h by 16bd9a85dbc3be76\, having originally been in mg.c in 25da442874cf6136 (its parent).

Reducing levels of indirection (that don't otherwise add anything) good\, in my book.

[I've not really been following the ramifications of the rest of this thread]

Nicholas Clark

[p5pRT]

From @Leont

On Wed\, Jan 12\, 2011 at 6​:45 PM\, Leon Timmermans \fawaka@&#8203;gmail\.com wrote​:

I attached a quick patch\, but haven't written tests yet. I've skipped on the PERL_BLOCKSIG_* macro's because quite frankly they weren't helpful at all. Possibly they should be removed\, they aren't used anywhere else in core.

I've attached new patches. A fixed version of the previous patch (so it will still build on systems without sigprocmask) and a patch deleting the SIGBLOCK* macros.

Leon

[p5pRT]

From @Leont

0001-Conditionally-unblock-after-signal-handler-82040.patch

```diff From 43240ceed35119fe0d77fe0d05a12fd1e6686649 Mon Sep 17 00:00:00 2001 From: Leon Timmermans Date: Thu, 13 Jan 2011 18:30:29 +0100 Subject: [PATCH 1/2] Conditionally unblock after signal handler[#82040] Only unblock signal after a safe-signal handler is executed if that signal was also unblocked before the handler. --- mg.c | 23 ++++++++++++++++++++--- 1 files changed, 20 insertions(+), 3 deletions(-) diff --git a/mg.c b/mg.c index a6912a0..6d2b2d7 100644 --- a/mg.c +++ b/mg.c @@ -1443,15 +1443,32 @@ Perl_despatch_signals(pTHX) for (sig = 1; sig < SIG_SIZE; sig++) { if (PL_psig_pend[sig]) { dSAVE_ERRNO; - PERL_BLOCKSIG_ADD(set, sig); +#if defined(HAS_SIGPROCMASK) + /* From sigaction(2) (FreeBSD man page): + * | Signal routines normally execute with the signal that + * | caused their invocation blocked, but other signals may + * | yet occur. + * Emulation of this behavior (from within Perl) is enabled + * using sigprocmask + */ + int was_blocked; + sigset_t newset, oldset; + + sigemptyset(&newset); + sigaddset(&newset, sig); + sigprocmask(SIG_BLOCK, &newset, &oldset); + was_blocked = sigismember(&oldset, sig); +#endif PL_psig_pend[sig] = 0; - PERL_BLOCKSIG_BLOCK(set); #if defined(HAS_SIGACTION) && defined(SA_SIGINFO) (*PL_sighandlerp)(sig, NULL, NULL); #else (*PL_sighandlerp)(sig); #endif - PERL_BLOCKSIG_UNBLOCK(set); +#if defined(HAS_SIGPROCMASK) + if (!was_blocked) + sigprocmask(SIG_UNBLOCK, &newset, NULL); +#endif RESTORE_ERRNO; } } -- 1.7.1 ```

[p5pRT]

From @Leont

0002-Remove-obsolete-macros.patch

```diff From 33bf137de0e6074034dd6507ed3df4291025836e Mon Sep 17 00:00:00 2001 From: Leon Timmermans Date: Thu, 13 Jan 2011 18:30:59 +0100 Subject: [PATCH 2/2] Remove obsolete macros --- perl.h | 30 ------------------------------ 1 files changed, 0 insertions(+), 30 deletions(-) diff --git a/perl.h b/perl.h index 567150d..36da020 100644 --- a/perl.h +++ b/perl.h @@ -6089,36 +6089,6 @@ extern void moncontrol(int); #define PERL_SIGNALS_UNSAFE_FLAG 0x0001 -/* From sigaction(2) (FreeBSD man page): - * | Signal routines normally execute with the signal that - * | caused their invocation blocked, but other signals may - * | yet occur. - * Emulation of this behavior (from within Perl) is enabled - * by defining PERL_BLOCK_SIGNALS. - */ -#define PERL_BLOCK_SIGNALS - -#if defined(HAS_SIGPROCMASK) && defined(PERL_BLOCK_SIGNALS) -# define PERL_BLOCKSIG_ADD(set,sig) \ - sigset_t set; sigemptyset(&(set)); sigaddset(&(set), sig) -# define PERL_BLOCKSIG_BLOCK(set) \ - sigprocmask(SIG_BLOCK, &(set), NULL) -# define PERL_BLOCKSIG_UNBLOCK(set) \ - sigprocmask(SIG_UNBLOCK, &(set), NULL) -#endif /* HAS_SIGPROCMASK && PERL_BLOCK_SIGNALS */ - -/* How about the old style of sigblock()? */ - -#ifndef PERL_BLOCKSIG_ADD -# define PERL_BLOCKSIG_ADD(set, sig) NOOP -#endif -#ifndef PERL_BLOCKSIG_BLOCK -# define PERL_BLOCKSIG_BLOCK(set) NOOP -#endif -#ifndef PERL_BLOCKSIG_UNBLOCK -# define PERL_BLOCKSIG_UNBLOCK(set) NOOP -#endif - /* Use instead of abs() since abs() forces its argument to be an int, * but also beware since this evaluates its argument twice, so no x++. */ #define PERL_ABS(x) ((x) < 0 ? -(x) : (x)) -- 1.7.1 ```

[p5pRT]

From @Leont

On Thu\, Jan 13\, 2011 at 6​:54 PM\, Leon Timmermans \fawaka@&#8203;gmail\.com wrote​:

I've attached new patches. A fixed version of the previous patch (so it will still build on systems without sigprocmask) and a patch deleting the SIGBLOCK* macros.

Added a few tests. I've never written tests for core before\, so I hope I didn't screw up.

Leon

[p5pRT]

From @Leont

0003-Added-tests-for-conditional-unblocking.patch

```diff From 2d6d161011c767d1148cd2e03ab415a9d9b893e4 Mon Sep 17 00:00:00 2001 From: Leon Timmermans Date: Mon, 17 Jan 2011 16:29:11 +0100 Subject: [PATCH 3/3] Added tests for conditional unblocking --- t/op/sigdispatch.t | 23 ++++++++++++++++++++++- 1 files changed, 22 insertions(+), 1 deletions(-) diff --git a/t/op/sigdispatch.t b/t/op/sigdispatch.t index 5d9908e..657ff98 100644 --- a/t/op/sigdispatch.t +++ b/t/op/sigdispatch.t @@ -7,8 +7,9 @@ BEGIN { } use strict; +use Config; -plan tests => 4; +plan tests => 7; watchdog(10); @@ -36,3 +37,23 @@ eval { }; is($@, "Alarm!\n", 'after the second loop'); + +SKIP: { + skip('We can\'t test blocking without sigprocmask', 3) if $ENV{PERL_CORE_MINITEST} || !$Config{d_sigprocmask}; + + require POSIX; + my $new = POSIX::SigSet->new(&POSIX::SIGUSR1); + POSIX::sigprocmask(&POSIX::SIG_BLOCK, $new); + + my $gotit = 0; + $SIG{USR1} = sub { $gotit++ }; + kill SIGUSR1, $$; + is $gotit, 0, 'Haven\'t received signal yet'; + + my $old = POSIX::SigSet->new(); + POSIX::sigsuspend($old); + is $gotit, 1, 'Received signal'; + + POSIX::sigprocmask(&POSIX::SIG_BLOCK, undef, $old); + ok $old->ismember(&POSIX::SIGUSR1), 'SIGUSR1 is still blocked'; +} -- 1.7.1 ```

[p5pRT]

From @Leont

Improved the tests patch\, and a small documentation update for POSIX​::sigprocmask. Unless issues arise\, I think the only thing needed now is a perldelta entry.

Leon

[p5pRT]

From @Leont

0003-Added-tests-for-conditional-unblocking.patch

```diff From 622727478dd68c5824a721bab1da15045ab884b0 Mon Sep 17 00:00:00 2001 From: Leon Timmermans Date: Mon, 17 Jan 2011 16:29:11 +0100 Subject: [PATCH 3/4] Added tests for conditional unblocking --- t/op/sigdispatch.t | 26 +++++++++++++++++++++++++- 1 files changed, 25 insertions(+), 1 deletions(-) diff --git a/t/op/sigdispatch.t b/t/op/sigdispatch.t index 5d9908e..a86861e 100644 --- a/t/op/sigdispatch.t +++ b/t/op/sigdispatch.t @@ -7,8 +7,9 @@ BEGIN { } use strict; +use Config; -plan tests => 4; +plan tests => 9; watchdog(10); @@ -36,3 +37,26 @@ eval { }; is($@, "Alarm!\n", 'after the second loop'); + +SKIP: { + skip('We can\'t test blocking without sigprocmask', 3) if $ENV{PERL_CORE_MINITEST} || !$Config{d_sigprocmask}; + + require POSIX; + my $new = POSIX::SigSet->new(&POSIX::SIGUSR1); + POSIX::sigprocmask(&POSIX::SIG_BLOCK, $new); + + my $gotit = 0; + $SIG{USR1} = sub { $gotit++ }; + kill SIGUSR1, $$; + is $gotit, 0, 'Haven\'t third received signal yet'; + + my $old = POSIX::SigSet->new(); + POSIX::sigsuspend($old); + is $gotit, 1, 'Received third signal'; + + kill SIGUSR1, $$; + is $gotit, 1, 'Haven\'t fourth received signal yet'; + POSIX::sigprocmask(&POSIX::SIG_UNBLOCK, $new, $old); + ok $old->ismember(&POSIX::SIGUSR1), 'SIGUSR1 was still blocked'; + is $gotit, 2, 'Received fourth signal'; +} -- 1.7.1 ```

[p5pRT]

From @Leont

0004-Clarify-limitation-in-safe-signals.patch

```diff From 24dadeebe63036ac4e0de0e929ec4134cbb48c67 Mon Sep 17 00:00:00 2001 From: Leon Timmermans Date: Mon, 17 Jan 2011 17:59:33 +0100 Subject: [PATCH 4/4] Clarify limitation in safe signals. --- ext/POSIX/lib/POSIX.pod | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/ext/POSIX/lib/POSIX.pod b/ext/POSIX/lib/POSIX.pod index 64852e9..9df0cde 100644 --- a/ext/POSIX/lib/POSIX.pod +++ b/ext/POSIX/lib/POSIX.pod @@ -1196,6 +1196,10 @@ Synopsis: Returns C on failure. +Note that you can't reliably block or unblock a signal from its own signal +handler if you're using safe signals. Other signals can be blocked or unblocked +reliably. + =item sigsetjmp C is C-specific: use C instead, -- 1.7.1 ```

[p5pRT]

From @tamias

On Mon\, Jan 17\, 2011 at 06​:07​:06PM +0100\, Leon Timmermans wrote​:

Improved the tests patch\, and a small documentation update for POSIX​::sigprocmask. Unless issues arise\, I think the only thing needed now is a perldelta entry.

Leon

+ is $gotit\, 0\, 'Haven\'t third received signal yet';

+ is $gotit\, 1\, 'Haven\'t fourth received signal yet';

Those should be​:

  is $gotit\, 0\, 'Haven\'t received third signal yet';

  is $gotit\, 1\, 'Haven\'t received fourth signal yet';

I presume.

Ronald

[p5pRT]

From @Leont

On Mon\, Jan 17\, 2011 at 8​:14 PM\, Ronald J Kimball \rjk@&#8203;tamias\.net wrote​:

+    is $gotit\, 0\, 'Haven\'t third received signal yet';

+    is $gotit\, 1\, 'Haven\'t fourth received signal yet';

Those should be​:

   is $gotit\, 0\, 'Haven\'t received third signal yet';

   is $gotit\, 1\, 'Haven\'t received fourth signal yet';

I presume.

Yeah\, I evidently wasn't paying attention when I added that :-|

Leon

[p5pRT]

From @cpansprout

On Thu Jan 13 09​:55​:03 2011\, LeonT wrote​:

On Wed\, Jan 12\, 2011 at 6​:45 PM\, Leon Timmermans \fawaka@&#8203;gmail\.com wrote​:

I attached a quick patch\, but haven't written tests yet. I've skipped on the PERL_BLOCKSIG_* macro's because quite frankly they weren't helpful at all. Possibly they should be removed\, they aren't used anywhere else in core.

I've attached new patches. A fixed version of the previous patch (so it will still build on systems without sigprocmask) and a patch deleting the SIGBLOCK* macros.

Leon

On Mon Jan 17 09​:07​:28 2011\, LeonT wrote​:

Improved the tests patch\, and a small documentation update for POSIX​::sigprocmask. Unless issues arise\, I think the only thing needed now is a perldelta entry.

Leon

Thank you. Applied as​:

555344425f04e96a72e4d29eab96b34bff8f96ae f5a55acdcac97456fa66e83ac3d005677b14cc00 0c1bf4c7d433bb0ad80bfe5511b1301db32b7b95 faaf68361923e4bb95d1eb919bc724a0dcc5a4ce

[p5pRT]

@cpansprout - Status changed from 'open' to 'resolved'

[p5pRT]

From @leonerd

On Wed\, Jan 12\, 2011 at 06​:45​:58PM +0100\, Leon Timmermans wrote​:

I attached a quick patch\, but haven't written tests yet. I've skipped on the PERL_BLOCKSIG_* macro's because quite frankly they weren't helpful at all. Possibly they should be removed\, they aren't used anywhere else in core.

Thanks\, that looks just right for what I had in mind. :)

Now all I have to do is work out a workaround for older perls...

-- Paul "LeoNerd" Evans

leonerd@​leonerd.org.uk ICQ# 4135350 | Registered Linux# 179460 http​://www.leonerd.org.uk/

[p5pRT]

From @Leont

On Tue\, Jan 18\, 2011 at 7​:34 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

Thank you. Applied as​:

Seems I missed the special case of a signal handler that throws an exception. Fix and tests are attached.

Leon

[p5pRT]

From @Leont

0001-Also-unblock-signal-handlers-throwing-an-exception.patch

```diff From 8d341ae1dd4e083920e7f02e27695d90ecbcd94b Mon Sep 17 00:00:00 2001 From: Leon Timmermans Date: Tue, 18 Jan 2011 16:40:07 +0100 Subject: [PATCH] Also unblock signal handlers throwing an exception Also handle and test the edge case of a signal handler throwing an exception --- mg.c | 31 +++++++++++++++++++------------ t/op/sigdispatch.t | 21 ++++++++++++++++----- 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/mg.c b/mg.c index f51cd47..cc13531 100644 --- a/mg.c +++ b/mg.c @@ -1434,6 +1434,14 @@ Perl_csighandler_init(void) } #endif +#if defined HAS_SIGPROCMASK +static void +unblock_sigmask(pTHX_ void* newset) +{ + sigprocmask(SIG_UNBLOCK, (sigset_t*)newset, NULL); +} +#endif + void Perl_despatch_signals(pTHX) { @@ -1443,7 +1451,7 @@ Perl_despatch_signals(pTHX) for (sig = 1; sig < SIG_SIZE; sig++) { if (PL_psig_pend[sig]) { dSAVE_ERRNO; -#if defined(HAS_SIGPROCMASK) +#ifdef HAS_SIGPROCMASK /* From sigaction(2) (FreeBSD man page): * | Signal routines normally execute with the signal that * | caused their invocation blocked, but other signals may @@ -1458,6 +1466,12 @@ Perl_despatch_signals(pTHX) sigaddset(&newset, sig); sigprocmask(SIG_BLOCK, &newset, &oldset); was_blocked = sigismember(&oldset, sig); + if (!was_blocked) { + SV* save_sv = newSVpvn((char *)(&newset), sizeof(sigset_t)); + ENTER; + SAVEFREESV(save_sv); + SAVEDESTRUCTOR_X(unblock_sigmask, SvPV_nolen(save_sv)); + } #endif PL_psig_pend[sig] = 0; #if defined(HAS_SIGACTION) && defined(SA_SIGINFO) @@ -1465,9 +1479,9 @@ Perl_despatch_signals(pTHX) #else (*PL_sighandlerp)(sig); #endif -#if defined(HAS_SIGPROCMASK) +#ifdef HAS_SIGPROCMASK if (!was_blocked) - sigprocmask(SIG_UNBLOCK, &newset, NULL); + LEAVE; #endif RESTORE_ERRNO; } @@ -3092,22 +3106,15 @@ Perl_sighandler(int sig) POPSTACK; if (SvTRUE(ERRSV)) { -#ifndef PERL_MICRO -#ifdef HAS_SIGPROCMASK +#if !defined(PERL_MICRO) && !defined(HAS_SIGPROCMASK) /* Handler "died", for example to get out of a restart-able read(). * Before we re-do that on its behalf re-enable the signal which was * blocked by the system when we entered. */ - sigset_t set; - sigemptyset(&set); - sigaddset(&set,sig); - sigprocmask(SIG_UNBLOCK, &set, NULL); -#else /* Not clear if this will work */ (void)rsignal(sig, SIG_IGN); (void)rsignal(sig, PL_csighandlerp); -#endif -#endif /* !PERL_MICRO */ +#endif /* !PERL_MICRO && !HAS_SIGPROCMASK*/ die_sv(ERRSV); } cleanup: diff --git a/t/op/sigdispatch.t b/t/op/sigdispatch.t index a86861e..e3c8fdb 100644 --- a/t/op/sigdispatch.t +++ b/t/op/sigdispatch.t @@ -9,7 +9,7 @@ BEGIN { use strict; use Config; -plan tests => 9; +plan tests => 12; watchdog(10); @@ -39,7 +39,7 @@ eval { is($@, "Alarm!\n", 'after the second loop'); SKIP: { - skip('We can\'t test blocking without sigprocmask', 3) if $ENV{PERL_CORE_MINITEST} || !$Config{d_sigprocmask}; + skip('We can\'t test blocking without sigprocmask', 8) if $ENV{PERL_CORE_MINITEST} || !$Config{d_sigprocmask}; require POSIX; my $new = POSIX::SigSet->new(&POSIX::SIGUSR1); @@ -48,15 +48,26 @@ SKIP: { my $gotit = 0; $SIG{USR1} = sub { $gotit++ }; kill SIGUSR1, $$; - is $gotit, 0, 'Haven\'t third received signal yet'; + is $gotit, 0, 'Haven\'t received third signal yet'; my $old = POSIX::SigSet->new(); POSIX::sigsuspend($old); is $gotit, 1, 'Received third signal'; + { + kill SIGUSR1, $$; + local $SIG{USR1} = sub { die "FAIL\n" }; + POSIX::sigprocmask(&POSIX::SIG_BLOCK, undef, $old); + ok $old->ismember(&POSIX::SIGUSR1), 'SIGUSR1 is blocked'; + eval { POSIX::sigsuspend(POSIX::SigSet->new) }; + is $@, "FAIL\n", 'Exception is thrown, so received fourth signal'; + POSIX::sigprocmask(&POSIX::SIG_BLOCK, undef, $old); + ok $old->ismember(&POSIX::SIGUSR1), 'SIGUSR1 is still blocked'; + } + kill SIGUSR1, $$; - is $gotit, 1, 'Haven\'t fourth received signal yet'; + is $gotit, 1, 'Haven\'t received fifth signal yet'; POSIX::sigprocmask(&POSIX::SIG_UNBLOCK, $new, $old); ok $old->ismember(&POSIX::SIGUSR1), 'SIGUSR1 was still blocked'; - is $gotit, 2, 'Received fourth signal'; + is $gotit, 2, 'Received fifth signal'; } -- 1.7.1 ```

[p5pRT]

From @cpansprout

On Tue Jan 18 07​:43​:33 2011\, LeonT wrote​:

On Tue\, Jan 18\, 2011 at 7​:34 AM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

Thank you. Applied as​:

Seems I missed the special case of a signal handler that throws an exception. Fix and tests are attached.

Leon

Thank you. Applied as 7fe50b8.

[p5pRT]

From @Leont

ribasushi discovered an issue in this patch. It breaks unsafe signal handling. The summary​:

When an exception is thrown in a signal handler that results in a call to longjmp. Calling longjmp from a signal handler is undefined per C standard and POSIX (and has a CERT recommendation against it\, SIG32-C). On Linux\, and probably other systems too\, one of the consequences of jumping out of a a signal handler is that the signal mask won't get reset as it should. The previous code compensated for that by always unblocking the signal. Since we can't retroactively forbid exceptions in unsafe signal handlers\, we should solve this differently. We can either

a) Reinstate the old unblocking code for unsafe signals b) Don't block the current signal in the first place\, by using the SA_NODEFER flag in sigaction.

Both behaviors are buggy in my mind\, but hey\, we're talking about unsafe signals so that's all SNAFU.

Leon

[p5pRT]

From @leonerd

On Tue\, Jan 25\, 2011 at 11​:11​:30PM +0100\, Leon Timmermans wrote​:

a) Reinstate the old unblocking code for unsafe signals

Purely from my perspective\, I'd be OK with this one. I wasn't using an unsafe signal; a normal safe one is fine in my case. So I'm less fussed about the behaviour of unsafe ones.

-- Paul "LeoNerd" Evans

leonerd@​leonerd.org.uk ICQ# 4135350 | Registered Linux# 179460 http​://www.leonerd.org.uk/

[p5pRT]

From @Leont

On Tue\, Jan 25\, 2011 at 11​:11 PM\, Leon Timmermans \fawaka@&#8203;gmail\.com wrote​:

ribasushi discovered an issue in this patch. It breaks unsafe signal handling. The summary​:

When an exception is thrown in a signal handler that results in a call to longjmp. Calling longjmp from a signal handler is undefined per C standard and POSIX (and has a CERT recommendation against it\, SIG32-C). On Linux\, and probably other systems too\, one of the consequences of jumping out of a a signal handler is that the signal mask won't get reset as it should. The previous code compensated for that by always unblocking the signal. Since we can't retroactively forbid exceptions in unsafe signal handlers\, we should solve this differently. We can either

a) Reinstate the old unblocking code for unsafe signals b) Don't block the current signal in the first place\, by using the SA_NODEFER flag in sigaction.

Patch doing option a attached. Not extensively tested though.

Leon

[p5pRT]

From @Leont

0001-Unblock-signal-mask-on-error-for-unsafe-signals.patch

```diff From 2b14040da489ea1d784fa12fadbcf1e3d8666940 Mon Sep 17 00:00:00 2001 From: Leon Timmermans Date: Sat, 12 Feb 2011 22:19:57 +0100 Subject: [PATCH] Unblock signal-mask on error for unsafe signals --- mg.c | 16 ++++++++++++++-- 1 files changed, 14 insertions(+), 2 deletions(-) diff --git a/mg.c b/mg.c index cc13531..6a6473a 100644 --- a/mg.c +++ b/mg.c @@ -3106,15 +3106,27 @@ Perl_sighandler(int sig) POPSTACK; if (SvTRUE(ERRSV)) { -#if !defined(PERL_MICRO) && !defined(HAS_SIGPROCMASK) +#ifndef PERL_MICRO /* Handler "died", for example to get out of a restart-able read(). * Before we re-do that on its behalf re-enable the signal which was * blocked by the system when we entered. */ +#ifdef HAS_SIGPROCMASK +#ifdef HAS_SIGACTION + if (sip) +#endif + { + sigset_t set; + sigemptyset(&set); + sigaddset(&set,sig); + sigprocmask(SIG_UNBLOCK, &set, NULL); + } +#else /* Not clear if this will work */ (void)rsignal(sig, SIG_IGN); (void)rsignal(sig, PL_csighandlerp); -#endif /* !PERL_MICRO && !HAS_SIGPROCMASK*/ +#endif +#endif /* !PERL_MICRO */ die_sv(ERRSV); } cleanup: -- 1.7.1 ```

[p5pRT]

From @cpansprout

On Sat Feb 12 13​:22​:51 2011\, LeonT wrote​:

On Tue\, Jan 25\, 2011 at 11​:11 PM\, Leon Timmermans \fawaka@&#8203;gmail\.com wrote​:

ribasushi discovered an issue in this patch. It breaks unsafe signal handling. The summary​:

When an exception is thrown in a signal handler that results in a call to longjmp. Calling longjmp from a signal handler is undefined per C standard and POSIX (and has a CERT recommendation against it\, SIG32-C). On Linux\, and probably other systems too\, one of the consequences of jumping out of a a signal handler is that the signal mask won't get reset as it should. The previous code compensated for that by always unblocking the signal. Since we can't retroactively forbid exceptions in unsafe signal handlers\, we should solve this differently. We can either

a) Reinstate the old unblocking code for unsafe signals b) Don't block the current signal in the first place\, by using the SA_NODEFER flag in sigaction.

Patch doing option a attached. Not extensively tested though.

Not understanding this area very well (you know it far better than I)\, I do not know how to go about testing it. But I did notice that it does not fix #83646 (Net​::Daemon)\, which was broken by the commit in question (7fe50b8). (There could have been something wrong with my setup\, though.)

[p5pRT]

From @Leont

On Sun\, Feb 13\, 2011 at 10​:31 PM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

Not understanding this area very well (you know it far better than I)\, I do not know how to go about testing it.

Here's a new patch that includes a test.

But I did notice that it does not fix #83646 (Net​::Daemon)\, which was broken by the commit in question (7fe50b8). (There could have been something wrong with my setup\, though.)

That breakage is due to a bug in that module's unit test\, it's not related to this last patch.

Leon

[p5pRT]

From @Leont

0001-Unblock-signal-mask-on-error-for-unsafe-signals.patch

```diff From 2bc6c1aad7f99917ffec0ae188e9b775e1b660a7 Mon Sep 17 00:00:00 2001 From: Leon Timmermans Date: Sat, 12 Feb 2011 22:19:57 +0100 Subject: [PATCH] Unblock signal-mask on error for unsafe signals --- mg.c | 16 ++++++++++++++-- t/op/sigdispatch.t | 8 +++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/mg.c b/mg.c index cc13531..6a6473a 100644 --- a/mg.c +++ b/mg.c @@ -3106,15 +3106,27 @@ Perl_sighandler(int sig) POPSTACK; if (SvTRUE(ERRSV)) { -#if !defined(PERL_MICRO) && !defined(HAS_SIGPROCMASK) +#ifndef PERL_MICRO /* Handler "died", for example to get out of a restart-able read(). * Before we re-do that on its behalf re-enable the signal which was * blocked by the system when we entered. */ +#ifdef HAS_SIGPROCMASK +#ifdef HAS_SIGACTION + if (sip) +#endif + { + sigset_t set; + sigemptyset(&set); + sigaddset(&set,sig); + sigprocmask(SIG_UNBLOCK, &set, NULL); + } +#else /* Not clear if this will work */ (void)rsignal(sig, SIG_IGN); (void)rsignal(sig, PL_csighandlerp); -#endif /* !PERL_MICRO && !HAS_SIGPROCMASK*/ +#endif +#endif /* !PERL_MICRO */ die_sv(ERRSV); } cleanup: diff --git a/t/op/sigdispatch.t b/t/op/sigdispatch.t index e3c8fdb..5a5fc14 100644 --- a/t/op/sigdispatch.t +++ b/t/op/sigdispatch.t @@ -9,7 +9,7 @@ BEGIN { use strict; use Config; -plan tests => 12; +plan tests => 13; watchdog(10); @@ -70,4 +70,10 @@ SKIP: { POSIX::sigprocmask(&POSIX::SIG_UNBLOCK, $new, $old); ok $old->ismember(&POSIX::SIGUSR1), 'SIGUSR1 was still blocked'; is $gotit, 2, 'Received fifth signal'; + + # test unsafe signal handlers in combination with exceptions + my $action = POSIX::SigAction->new(sub { $gotit--, die }, POSIX::SigSet->new, 0); + POSIX::sigaction(&POSIX::SIGUSR1, $action); + eval { kill SIGUSR1, $$ } for 1..2; + is $gotit, 0, 'Received both signals'; } -- 1.7.1 ```

[p5pRT]

From @cpansprout

On Thu Feb 17 10​:16​:58 2011\, LeonT wrote​:

On Sun\, Feb 13\, 2011 at 10​:31 PM\, Father Chrysostomos via RT \perlbug\-followup@&#8203;perl\.org wrote​:

Not understanding this area very well (you know it far better than I)\, I do not know how to go about testing it.

Here's a new patch that includes a test.

But I did notice that it does not fix #83646 (Net​::Daemon)\, which was broken by the commit in question (7fe50b8). (There could have been something wrong with my setup\, though.)

That breakage is due to a bug in that module's unit test\, it's not related to this last patch.

Leon

Thank you. I’ve just applied it as c22d665.