RT #76248: double-freed SV with nested sig-handler

There was some buggy code in Perl_sighandler() related to getting an SV
with the signal name to pass to the perl-level handler function.
`
Basically:

on threaded builds, a sig handler that died leaked PL_psig_name[sig];
on unthreaded builds, in a recursive handler that died, PL_sig_sv was
prematurely freed.

PL_sig_sv was originally just a file static var that was not
recursion-save anyway, and got promoted to perlvars.h when it should
instead have been done away with. So I've got rid of it now, and
rationalised the code, which fixed the two issues listed above.

Also added an assert which makes the dodgy manual popping of the save
stack slightly less dodgy.

6 files changed, 23 insertions, 37 deletions

diff --git a/embed.fnc b/embed.fnc
index 4245409f83..88a5ed5851 100644
--- a/embed.fnc
+++ b/embed.fnc
@@ -1537,7 +1537,7 @@ s	|SV*	|magic_methcall1|NN SV *sv|NN const MAGIC *mg \
 				|NN const char *meth|U32 flags \
 				|int n|NULLOK SV *val
 s	|void	|restore_magic	|NULLOK const void *p
-s	|void	|unwind_handler_stack|NN const void *p
+s	|void	|unwind_handler_stack|NULLOK const void *p
 
 #endif
 
diff --git a/embedvar.h b/embedvar.h
index 262ddb0d3c..87099c13aa 100644
--- a/embedvar.h
+++ b/embedvar.h
@@ -771,8 +771,6 @@
 #define PL_Gsig_handlers_initted	(my_vars->Gsig_handlers_initted)
 #define PL_sig_ignoring		(my_vars->Gsig_ignoring)
 #define PL_Gsig_ignoring	(my_vars->Gsig_ignoring)
-#define PL_sig_sv		(my_vars->Gsig_sv)
-#define PL_Gsig_sv		(my_vars->Gsig_sv)
 #define PL_sig_trapped		(my_vars->Gsig_trapped)
 #define PL_Gsig_trapped		(my_vars->Gsig_trapped)
 #define PL_sigfpe_saved		(my_vars->Gsigfpe_saved)
@@ -832,7 +830,6 @@
 #define PL_Gsig_defaulting	PL_sig_defaulting
 #define PL_Gsig_handlers_initted	PL_sig_handlers_initted
 #define PL_Gsig_ignoring	PL_sig_ignoring
-#define PL_Gsig_sv		PL_sig_sv
 #define PL_Gsig_trapped		PL_sig_trapped
 #define PL_Gsigfpe_saved	PL_sigfpe_saved
 #define PL_Gsubversion		PL_subversion
diff --git a/mg.c b/mg.c
index 4a1a72b668..e90cd594ba 100644
--- a/mg.c
+++ b/mg.c
@@ -2943,6 +2943,7 @@ Perl_sighandler(int sig)
     OP *myop = PL_op;
     U32 flags = 0;
     XPV * const tXpv = PL_Xpv;
+    I32 old_ss_ix = PL_savestack_ix;
 
     if (PL_savestack_ix + 15 <= PL_savestack_max)
 	flags |= 1;
@@ -2961,7 +2962,7 @@ Perl_sighandler(int sig)
        infinity, so we fix 4 (in fact 5): */
     if (flags & 1) {
 	PL_savestack_ix += 5;		/* Protect save in progress. */
-	SAVEDESTRUCTOR_X(S_unwind_handler_stack, (void*)&flags);
+	SAVEDESTRUCTOR_X(S_unwind_handler_stack, NULL);
     }
     if (flags & 4)
 	PL_markstack_ptr++;		/* Protect mark. */
@@ -2983,16 +2984,15 @@ Perl_sighandler(int sig)
 	goto cleanup;
     }
 
-    if(PL_psig_name[sig]) {
-	sv = SvREFCNT_inc_NN(PL_psig_name[sig]);
-	flags |= 64;
-#if !defined(PERL_IMPLICIT_CONTEXT)
-	PL_sig_sv = sv;
-#endif
-    } else {
-	sv = sv_newmortal();
-	sv_setpv(sv,PL_sig_name[sig]);
-    }
+    sv = PL_psig_name[sig]
+	    ? SvREFCNT_inc_NN(PL_psig_name[sig])
+	    : newSVpv(PL_sig_name[sig],0);
+    flags |= 64;
+    SAVEFREESV(sv);
+
+    /* make sure our assumption about the size of the SAVEs are correct:
+     * 3 for SAVEDESTRUCTOR_X, 2 for SAVEFREESV */
+    assert(old_ss_ix + 2 + ((flags & 1) ? 3+5 : 0)  == PL_savestack_ix);
 
     PUSHSTACKi(PERLSI_SIGNAL);
     PUSHMARK(SP);
@@ -3050,8 +3050,8 @@ Perl_sighandler(int sig)
 	die_sv(ERRSV);
     }
 cleanup:
-    if (flags & 1)
-	PL_savestack_ix -= 8; /* Unprotect save in progress. */
+    /* pop any of SAVEFREESV, SAVEDESTRUCTOR_X and "save in progress" */
+    PL_savestack_ix = old_ss_ix;
     if (flags & 4)
 	PL_markstack_ptr--;
     if (flags & 16)
@@ -3124,20 +3124,19 @@ S_restore_magic(pTHX_ const void *p)
 
 }
 
+/* clean up the mess created by Perl_sighandler().
+ * Note that this is only called during an exit in a signal handler;
+ * a die is trapped by the call_sv() and the SAVEDESTRUCTOR_X manually
+ * skipped over. This is why we don't need to fix up the markstack and
+ * scopestack - they're going to be set to 0 anyway */
+
 static void
 S_unwind_handler_stack(pTHX_ const void *p)
 {
     dVAR;
-    const U32 flags = *(const U32*)p;
-
-    PERL_ARGS_ASSERT_UNWIND_HANDLER_STACK;
+    PERL_UNUSED_ARG(p);
 
-    if (flags & 1)
-	PL_savestack_ix -= 5; /* Unprotect save in progress. */
-#if !defined(PERL_IMPLICIT_CONTEXT)
-    if (flags & 64)
-	SvREFCNT_dec(PL_sig_sv);
-#endif
+    PL_savestack_ix -= 5; /* Unprotect save in progress. */
 }
 
 /*
diff --git a/perlapi.h b/perlapi.h
index 151eacc2c9..1c64ec72c3 100644
--- a/perlapi.h
+++ b/perlapi.h
@@ -177,8 +177,6 @@ END_EXTERN_C
 #define PL_sig_handlers_initted	(*Perl_Gsig_handlers_initted_ptr(NULL))
 #undef  PL_sig_ignoring
 #define PL_sig_ignoring		(*Perl_Gsig_ignoring_ptr(NULL))
-#undef  PL_sig_sv
-#define PL_sig_sv		(*Perl_Gsig_sv_ptr(NULL))
 #undef  PL_sig_trapped
 #define PL_sig_trapped		(*Perl_Gsig_trapped_ptr(NULL))
 #undef  PL_sigfpe_saved
diff --git a/perlvars.h b/perlvars.h
index e424913d95..1a44e22e4e 100644
--- a/perlvars.h
+++ b/perlvars.h
@@ -102,10 +102,6 @@ PERLVARA(Gsig_ignoring, SIG_SIZE, int)	/* which signals we are ignoring */
 PERLVARA(Gsig_defaulting, SIG_SIZE, int)
 #endif
 
-#ifndef PERL_IMPLICIT_CONTEXT
-PERLVAR(Gsig_sv, SV*)
-#endif
-
 /* XXX signals are process-wide anyway, so we
  * ignore the implications of this for threading */
 #ifndef HAS_SIGACTION
diff --git a/proto.h b/proto.h
index dc25fd2025..c17071b235 100644
--- a/proto.h
+++ b/proto.h
@@ -5644,11 +5644,7 @@ STATIC void	S_save_magic(pTHX_ I32 mgs_ix, SV *sv)
 #define PERL_ARGS_ASSERT_SAVE_MAGIC	\
 	assert(sv)
 
-STATIC void	S_unwind_handler_stack(pTHX_ const void *p)
-			__attribute__nonnull__(pTHX_1);
-#define PERL_ARGS_ASSERT_UNWIND_HANDLER_STACK	\
-	assert(p)
-
+STATIC void	S_unwind_handler_stack(pTHX_ const void *p);
 #endif
 #if defined(PERL_IN_MRO_C)
 STATIC AV*	S_mro_get_linear_isa_dfs(pTHX_ HV* stash, U32 level)