develooper Front page | perl.perl5.porters | Postings from February 2003

RE: [perl #20613] Perl_magic_setsig/clearsig problems (patch included)

From:
Anders Johnson
Date:
February 11, 2003 01:16
Subject:
RE: [perl #20613] Perl_magic_setsig/clearsig problems (patch included)
Message ID:
000e01c2d151$2228ca90$9800a8c0@wis.com
This thread seems to have died out unexpectedly. I've included my latest
patch (relative to 5.8.0), which is alleged to do the following:

+ Signal handler isn't freed until after the new disposition is
installed.
+ Signal can't get deferred and then polled after the handler is
removed.
+ delete $SIG{...} is roughly equivalent to $SIG{...}="DEFAULT".
+ A couple of redundant statements were removed.

There is still the issue of a signal handler calling die() while the
main thread is inside a destructor. I'd like to discuss what can be done
about this, but it shouldn't necessarily preclude applying the patch
as-is.

Robust, clean signal handling is apparently a goal of Perl 5.7.3 and
later. I think it's important to make that a reality, and not just a
promise. I'm here to help if I can.

Thanks,
&ers

----- Begin Patch -----
diff -Naur perl-5.8.0/mg.c perl-5.8.0-sig3/mg.c
--- perl-5.8.0/mg.c	Sat Jun 15 13:16:44 2002
+++ perl-5.8.0-sig3/mg.c	Wed Jan 29 17:17:46 2003
@@ -1028,6 +1028,14 @@
 #endif
 
 #ifndef PERL_MICRO
+#ifdef HAS_SIGPROCMASK
+static void
+restore_sigmask(pTHX_ SV *save_sv)
+{
+    sigset_t *ossetp = (sigset_t *) SvPV_nolen( save_sv );
+    (void)sigprocmask(SIG_SETMASK, ossetp, (sigset_t *)0);
+}
+#endif
 int
 Perl_magic_getsig(pTHX_ SV *sv, MAGIC *mg)
 {
@@ -1061,19 +1069,67 @@
 int
 Perl_magic_clearsig(pTHX_ SV *sv, MAGIC *mg)
 {
-    I32 i;
+    /* XXX Some of this code was copied from Perl_magic_setsig. A
little
+     * refactoring might be in order.
+     */
+    register char *s;
     STRLEN n_a;
-    /* Are we clearing a signal entry? */
-    i = whichsig(MgPV(mg,n_a));
-    if (i) {
-    	if(PL_psig_ptr[i]) {
-    	    SvREFCNT_dec(PL_psig_ptr[i]);
-    	    PL_psig_ptr[i]=0;
-    	}
-    	if(PL_psig_name[i]) {
-    	    SvREFCNT_dec(PL_psig_name[i]);
-    	    PL_psig_name[i]=0;
-    	}
+    SV* to_dec;
+    s = MgPV(mg,n_a);
+    if (*s == '_') {
+	SV** svp;
+	if (strEQ(s,"__DIE__"))
+	    svp = &PL_diehook;
+	else if (strEQ(s,"__WARN__"))
+	    svp = &PL_warnhook;
+	else
+	    Perl_croak(aTHX_ "No such hook: %s", s);
+	if (*svp) {
+	    to_dec = *svp;
+	    *svp = 0;
+    	    SvREFCNT_dec(to_dec);
+	}
+    }
+    else {
+	I32 i;
+	/* Are we clearing a signal entry? */
+	i = whichsig(s);
+	if (i) {
+#ifdef HAS_SIGPROCMASK
+	    sigset_t set, save;
+	    SV* save_sv;
+	    /* Avoid having the signal arrive at a bad time, if
possible. */
+	    sigemptyset(&set);
+	    sigaddset(&set,i);
+	    sigprocmask(SIG_BLOCK, &set, &save);
+	    ENTER;
+	    save_sv = newSVpv((char *)(&save), sizeof(sigset_t));
+	    SAVEFREESV(save_sv);
+	    SAVEDESTRUCTOR_X(restore_sigmask, save_sv);
+#endif
+	    PERL_ASYNC_CHECK();
+#if defined(FAKE_PERSISTENT_SIGNAL_HANDLERS) ||
defined(FAKE_DEFAULT_SIGNAL_HANDLERS)
+	    if (!sig_handlers_initted) Perl_csighandler_init();
+#endif
+#ifdef FAKE_DEFAULT_SIGNAL_HANDLERS
+	    sig_defaulting[i] = 1;
+	    (void)rsignal(i, &Perl_csighandler);
+#else
+	    (void)rsignal(i, SIG_DFL);
+#endif
+    	    if(PL_psig_name[i]) {
+    		SvREFCNT_dec(PL_psig_name[i]);
+    		PL_psig_name[i]=0;
+    	    }
+    	    if(PL_psig_ptr[i]) {
+		to_dec=PL_psig_ptr[i];
+    		PL_psig_ptr[i]=0;
+		LEAVE;
+    		SvREFCNT_dec(to_dec);
+    	    }
+	    else
+		LEAVE;
+	}
     }
     return 0;
 }
@@ -1156,7 +1212,16 @@
     register char *s;
     I32 i;
     SV** svp = 0;
+    /* Need to be careful with SvREFCNT_dec(), because that can have
side
+     * effects (due to closures). We must make sure that the new
disposition
+     * is in place before it is called.
+     */
+    SV* to_dec = 0;
     STRLEN len;
+#ifdef HAS_SIGPROCMASK
+    sigset_t set, save;
+    SV* save_sv;
+#endif
 
     s = MgPV(mg,len);
     if (*s == '_') {
@@ -1168,7 +1233,7 @@
 	    Perl_croak(aTHX_ "No such hook: %s", s);
 	i = 0;
 	if (*svp) {
-	    SvREFCNT_dec(*svp);
+	    to_dec = *svp;
 	    *svp = 0;
 	}
     }
@@ -1179,6 +1244,17 @@
 		Perl_warner(aTHX_ packWARN(WARN_SIGNAL), "No such
signal: SIG%s", s);
 	    return 0;
 	}
+#ifdef HAS_SIGPROCMASK
+	/* Avoid having the signal arrive at a bad time, if possible. */
+	sigemptyset(&set);
+	sigaddset(&set,i);
+	sigprocmask(SIG_BLOCK, &set, &save);
+	ENTER;
+	save_sv = newSVpv((char *)(&save), sizeof(sigset_t));
+	SAVEFREESV(save_sv);
+	SAVEDESTRUCTOR_X(restore_sigmask, save_sv);
+#endif
+	PERL_ASYNC_CHECK();
 #if defined(FAKE_PERSISTENT_SIGNAL_HANDLERS) ||
defined(FAKE_DEFAULT_SIGNAL_HANDLERS)
 	if (!sig_handlers_initted) Perl_csighandler_init();
 #endif
@@ -1186,20 +1262,26 @@
 	sig_ignoring[i] = 0;
 #endif
 #ifdef FAKE_DEFAULT_SIGNAL_HANDLERS
-	  sig_defaulting[i] = 0;
+	sig_defaulting[i] = 0;
 #endif
 	SvREFCNT_dec(PL_psig_name[i]);
-	SvREFCNT_dec(PL_psig_ptr[i]);
+	to_dec = PL_psig_ptr[i];
 	PL_psig_ptr[i] = SvREFCNT_inc(sv);
 	SvTEMP_off(sv); /* Make sure it doesn't go away on us */
 	PL_psig_name[i] = newSVpvn(s, len);
 	SvREADONLY_on(PL_psig_name[i]);
     }
     if (SvTYPE(sv) == SVt_PVGV || SvROK(sv)) {
-	if (i)
+	if (i) {
 	    (void)rsignal(i, &Perl_csighandler);
+#ifdef HAS_SIGPROCMASK
+	    LEAVE;
+#endif
+	}
 	else
 	    *svp = SvREFCNT_inc(sv);
+	if(to_dec)
+	    SvREFCNT_dec(to_dec);
 	return 0;
     }
     s = SvPV_force(sv,len);
@@ -1211,8 +1293,7 @@
 #else
 	    (void)rsignal(i, SIG_IGN);
 #endif
-	} else
-	    *svp = 0;
+	}
     }
     else if (strEQ(s,"DEFAULT") || !*s) {
 	if (i)
@@ -1224,8 +1305,6 @@
 #else
 	    (void)rsignal(i, SIG_DFL);
 #endif
-	else
-	    *svp = 0;
     }
     else {
 	/*
@@ -1240,6 +1319,12 @@
 	else
 	    *svp = SvREFCNT_inc(sv);
     }
+#ifdef HAS_SIGPROCMASK
+    if(i)
+	LEAVE;
+#endif
+    if(to_dec)
+	SvREFCNT_dec(to_dec);
     return 0;
 }
 #endif /* !PERL_MICRO */
diff -Naur perl-5.8.0/t/op/magic.t perl-5.8.0-sig3/t/op/magic.t
--- perl-5.8.0/t/op/magic.t	Wed Jul 10 17:18:07 2002
+++ perl-5.8.0-sig3/t/op/magic.t	Wed Jan 29 13:00:40 2003
@@ -36,7 +36,7 @@
     return 1;
 }
 
-print "1..46\n";
+print "1..48\n";
 
 $Is_MSWin32 = $^O eq 'MSWin32';
 $Is_NetWare = $^O eq 'NetWare';
@@ -67,7 +67,7 @@
 close FOO; # just mention it, squelch used-only-once
 
 if ($Is_MSWin32 || $Is_NetWare || $Is_Dos || $Is_MPE || $Is_MacOS) {
-    skip('SIGINT not safe on this platform') for 1..2;
+    skip('SIGINT not safe on this platform') for 1..4;
 }
 else {
   # the next tests are done in a subprocess because sh spits out a
@@ -98,7 +98,35 @@
 
     close CMDPIPE;
 
-    $test += 2;
+    open( CMDPIPE, "| $PERL");
+    print CMDPIPE <<'END';
+
+    { package X;
+	sub DESTROY {
+	    kill "INT",$$;
+	}
+    }
+    sub x {
+	my $x=bless [], 'X';
+	return sub { $x };
+    }
+    $| = 1;		# command buffering
+    $SIG{"INT"} = "ok5";
+    {
+	local $SIG{"INT"}=x();
+	print ""; # Needed to expose failure in 5.8.0 (why?)
+    }
+    sleep 1;
+    delete $SIG{"INT"};
+    kill "INT",$$; sleep 1;
+    sub ok5 {
+	print "ok 5\n";
+    }
+END
+    close CMDPIPE;
+    print $? & 0xFF ? "ok 6\n" : "not ok 6\n";
+
+    $test += 4;
 }
 
 # can we slice ENV?





nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About