From: Vincent Pit <vince@profvince.com>
Date: Sat, 18 Aug 2012 13:48:41 +0000 (+0200)
Subject: Properly propagate exceptions when a free callback dies at the end of eval
X-Git-Tag: v0.51~2
X-Git-Url: http://git.vpit.fr/?p=perl%2Fmodules%2FVariable-Magic.git;a=commitdiff_plain;h=7f3b1ec01ca3856b1d5670ab9795cf0e91ba8928

Properly propagate exceptions when a free callback dies at the end of eval
---

diff --git a/Magic.xs b/Magic.xs
index 15d51ca..9bdcca5 100644
--- a/Magic.xs
+++ b/Magic.xs
@@ -187,6 +187,11 @@
 # define VMG_COMPAT_GLOB_GET 0
 #endif
 
+#define VMG_PROPAGATE_ERRSV_NEEDS_TRAMPOLINE (VMG_HAS_PERL(5, 10, 0) && !VMG_HAS_PERL(5, 10, 1))
+
+/* NewOp() isn't public in perl 5.8.0. */
+#define VMG_RESET_RMG_NEEDS_TRAMPOLINE (VMG_UVAR && (VMG_THREADSAFE || !VMG_HAS_PERL(5, 8, 1)))
+
 /* ... Bug-free mg_magical ................................................. */
 
 /* See the discussion at http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2008-01/msg00036.html */
@@ -220,12 +225,9 @@ STATIC void vmg_mg_magical(SV *sv) {
 
 #endif
 
-/* ... Trampoline ops ...................................................... */
-
-/* NewOp() isn't public in perl 5.8.0. */
-#define VMG_RESET_RMG_NEEDS_TRAMPOLINE (VMG_UVAR && (VMG_THREADSAFE || !VMG_HAS_PERL(5, 8, 1)))
+/* --- Trampoline ops ------------------------------------------------------ */
 
-#define VMG_NEEDS_TRAMPOLINE VMG_RESET_RMG_NEEDS_TRAMPOLINE
+#define VMG_NEEDS_TRAMPOLINE VMG_PROPAGATE_ERRSV_NEEDS_TRAMPOLINE || VMG_RESET_RMG_NEEDS_TRAMPOLINE
 
 #if VMG_NEEDS_TRAMPOLINE
 
@@ -262,11 +264,11 @@ STATIC OP *vmg_trampoline_bump(pTHX_ vmg_trampoline *t, SV *sv, OP *o) {
 
 #endif /* VMG_NEEDS_TRAMPOLINE */
 
-/* ... Safe version of call_sv() ........................................... */
+/* --- Safe version of call_sv() ------------------------------------------- */
 
 STATIC I32 vmg_call_sv(pTHX_ SV *sv, I32 flags, int (*cleanup)(pTHX_ void *), void *ud) {
 #define vmg_call_sv(S, F, C, U) vmg_call_sv(aTHX_ (S), (F), (C), (U))
- I32 ret, cxix, in_eval = 0;
+ I32 ret, cxix;
  PERL_CONTEXT saved_cx;
  SV *old_err = NULL;
 
@@ -275,12 +277,7 @@ STATIC I32 vmg_call_sv(pTHX_ SV *sv, I32 flags, int (*cleanup)(pTHX_ void *), vo
   ERRSV   = newSV(0);
  }
 
- if (cxstack_ix < cxstack_max) {
-  cxix    = cxstack_ix + 1;
-  in_eval = CxTYPE(cxstack + cxix) == CXt_EVAL;
- } else {
-  cxix    = Perl_cxinc(aTHX);
- }
+ cxix     = (cxstack_ix < cxstack_max) ? (cxstack_ix + 1) : Perl_cxinc(aTHX);
  /* The last popped context will be reused by call_sv(), but our callers may
   * still need its previous value. Back it up so that it isn't clobbered. */
  saved_cx = cxstack[cxix];
@@ -311,7 +308,7 @@ STATIC I32 vmg_call_sv(pTHX_ SV *sv, I32 flags, int (*cleanup)(pTHX_ void *), vo
 #else
    ++PL_Ierror_count;
 #endif
-  } else if (!in_eval) {
+  } else {
    if (!cleanup || cleanup(aTHX_ ud))
     croak(NULL);
   }
@@ -446,6 +443,9 @@ typedef struct {
  HV             *b__op_stashes[OPc_MAX];
  I32             depth;
  MAGIC          *freed_tokens;
+#if VMG_PROPAGATE_ERRSV_NEEDS_TRAMPOLINE
+ vmg_trampoline  propagate_errsv;
+#endif
 #if VMG_RESET_RMG_NEEDS_TRAMPOLINE
  vmg_trampoline  reset_rmg;
 #endif
@@ -1349,26 +1349,116 @@ STATIC int vmg_svt_clear(pTHX_ SV *sv, MAGIC *mg) {
 
 /* ... free magic .......................................................... */
 
-STATIC int vmg_svt_free_cleanup(pTHX_ void *ud) {
- SV    *sv = VOID2(SV *, ud);
- MAGIC *mg;
+#if VMG_PROPAGATE_ERRSV_NEEDS_TRAMPOLINE
+
+STATIC OP *vmg_pp_propagate_errsv(pTHX) {
+ SVOP *o = cSVOPx(PL_op);
 
- /* We are about to croak() while sv is being destroyed. Try to clean up
-  * things a bit. */
- mg = SvMAGIC(sv);
- if (mg) {
-  vmg_mg_del(sv, NULL, mg, mg->mg_moremagic);
-  mg_magical(sv);
+ if (o->op_sv) {
+  SvREFCNT_dec(ERRSV);
+  ERRSV    = o->op_sv;
+  o->op_sv = NULL;
  }
- SvREFCNT_dec(sv);
 
- vmg_dispell_guard_oncroak(aTHX_ ud);
+ return NORMAL;
+}
 
- /* After that, propagate the error upwards. */
- return 1;
+#endif /* VMG_PROPAGATE_ERRSV_NEEDS_TRAMPOLINE */
+
+STATIC int vmg_propagate_errsv_free(pTHX_ SV *sv, MAGIC *mg) {
+ if (mg->mg_obj) {
+  ERRSV         = mg->mg_obj;
+  mg->mg_obj    = NULL;
+  mg->mg_flags &= ~MGf_REFCOUNTED;
+ }
+
+ return 0;
+}
+
+/* perl is already kind enough to handle the cloning of the mg_obj member,
+   hence we don't need to define a dup magic callback. */
+
+STATIC MGVTBL vmg_propagate_errsv_vtbl = {
+ 0,                        /* get */
+ 0,                        /* set */
+ 0,                        /* len */
+ 0,                        /* clear */
+ vmg_propagate_errsv_free, /* free */
+ 0,                        /* copy */
+ 0,                        /* dup */
+#if MGf_LOCAL
+ 0,                        /* local */
+#endif /* MGf_LOCAL */
+};
+
+typedef struct {
+ SV  *sv;
+ int  in_eval;
+ I32  base;
+} vmg_svt_free_cleanup_ud;
+
+STATIC int vmg_svt_free_cleanup(pTHX_ void *ud_) {
+ vmg_svt_free_cleanup_ud *ud = VOID2(vmg_svt_free_cleanup_ud *, ud_);
+
+ if (ud->in_eval) {
+  U32 optype = PL_op ? PL_op->op_type : OP_NULL;
+
+  if (optype == OP_LEAVETRY || optype == OP_LEAVEEVAL) {
+   SV *errsv = newSVsv(ERRSV);
+
+   FREETMPS;
+   LEAVE_SCOPE(ud->base);
+
+#if VMG_PROPAGATE_ERRSV_NEEDS_TRAMPOLINE
+   if (optype == OP_LEAVETRY) {
+    dMY_CXT;
+    PL_op = vmg_trampoline_bump(&MY_CXT.propagate_errsv, errsv, PL_op);
+   } else if (optype == OP_LEAVEEVAL) {
+    SV *guard = sv_newmortal();
+    sv_magicext(guard, errsv, PERL_MAGIC_ext, &vmg_propagate_errsv_vtbl,
+                              NULL, 0);
+   }
+#else /* !VMG_PROPAGATE_ERRSV_NEEDS_TRAMPOLINE */
+# if !VMG_HAS_PERL(5, 8, 9)
+   {
+    SV *guard = sv_newmortal();
+    sv_magicext(guard, errsv, PERL_MAGIC_ext, &vmg_propagate_errsv_vtbl,
+                              NULL, 0);
+   }
+# else
+   sv_magicext(ERRSV, errsv, PERL_MAGIC_ext, &vmg_propagate_errsv_vtbl,
+                             NULL, 0);
+   SvREFCNT_dec(errsv);
+# endif
+#endif /* VMG_PROPAGATE_ERRSV_NEEDS_TRAMPOLINE */
+
+   SAVETMPS;
+  }
+
+  /* Don't propagate */
+  return 0;
+ } else {
+  SV    *sv = ud->sv;
+  MAGIC *mg;
+
+  /* We are about to croak() while sv is being destroyed. Try to clean up
+   * things a bit. */
+  mg = SvMAGIC(sv);
+  if (mg) {
+   vmg_mg_del(sv, NULL, mg, mg->mg_moremagic);
+   mg_magical(sv);
+  }
+  SvREFCNT_dec(sv);
+
+  vmg_dispell_guard_oncroak(aTHX_ NULL);
+
+  /* After that, propagate the error upwards. */
+  return 1;
+ }
 }
 
 STATIC int vmg_svt_free(pTHX_ SV *sv, MAGIC *mg) {
+ vmg_svt_free_cleanup_ud ud;
  const vmg_wizard *w;
  int ret = 0;
  SV *svr;
@@ -1392,6 +1482,15 @@ STATIC int vmg_svt_free(pTHX_ SV *sv, MAGIC *mg) {
  SvMAGIC_set(sv, mg);
 #endif
 
+ ud.sv = sv;
+ if (cxstack_ix < cxstack_max) {
+  ud.in_eval = (CxTYPE(cxstack + cxstack_ix + 1) == CXt_EVAL);
+  ud.base    = ud.in_eval ? PL_scopestack[PL_scopestack_ix] : 0;
+ } else {
+  ud.in_eval = 0;
+  ud.base    = 0;
+ }
+
  ENTER;
  SAVETMPS;
 
@@ -1406,7 +1505,7 @@ STATIC int vmg_svt_free(pTHX_ SV *sv, MAGIC *mg) {
  {
   dMY_CXT;
   MY_CXT.depth++;
-  vmg_call_sv(w->cb_free, G_SCALAR, vmg_svt_free_cleanup, sv);
+  vmg_call_sv(w->cb_free, G_SCALAR, vmg_svt_free_cleanup, &ud);
   MY_CXT.depth--;
   if (MY_CXT.depth == 0 && MY_CXT.freed_tokens) {
    /* Free all the tokens in the chain but the current one (if it's present).
@@ -1680,6 +1779,9 @@ BOOT:
  MY_CXT.freed_tokens = NULL;
 
  /* XS doesn't like a blank line here */
+#if VMG_PROPAGATE_ERRSV_NEEDS_TRAMPOLINE
+ vmg_trampoline_init(&MY_CXT.propagate_errsv, vmg_pp_propagate_errsv);
+#endif
 #if VMG_RESET_RMG_NEEDS_TRAMPOLINE
  vmg_trampoline_init(&MY_CXT.reset_rmg, vmg_pp_reset_rmg);
 #endif
diff --git a/t/15-self.t b/t/15-self.t
index a205ea5..0550dc4 100644
--- a/t/15-self.t
+++ b/t/15-self.t
@@ -162,10 +162,7 @@ my $c = 0;
      }
 
      if ($die) {
-      SKIP: {
-       skip 'error not propagated' => 1 if $magic eq 'free';
-       like $err, qr/^oops at/, "$desc: correct error";
-      }
+      like $err, qr/^oops at/, "$desc: correct error";
       is $res, undef, "$desc: returned undef";
      } else {
       is $err, '', "$desc: no error";
diff --git a/t/17-ctl.t b/t/17-ctl.t
index 2500a94..f781a67 100644
--- a/t/17-ctl.t
+++ b/t/17-ctl.t
@@ -3,7 +3,7 @@
 use strict;
 use warnings;
 
-use Test::More tests => 4 * 8 + 10 + 1 + 1;
+use Test::More tests => 4 * 8 + 4 * (2 * 4 + 1) + 10 + 1 + 1;
 
 use Variable::Magic qw<wizard cast VMG_UVAR>;
 
@@ -128,6 +128,101 @@ for my $t (@scalar_tests) {
 
 # Free
 
+{
+ my $wiz   = wizard free => sub { die 'avocado' };
+ my $check = sub { like $@, expect('avocado', $0), $_[0] };
+
+ for my $local_out (0, 1) {
+  for my $local_in (0, 1) {
+   my $desc   = "die in free callback";
+   if ($local_in or $local_out) {
+    $desc .= ' with $@ localized ';
+    if ($local_in and $local_out) {
+     $desc .= 'inside and outside';
+    } elsif ($local_in) {
+     $desc .= 'inside';
+    } else {
+     $desc .= 'outside';
+    }
+   }
+
+   local $@ = $local_out ? 'xxx' : undef;
+   eval {
+    local $@ = 'yyy' if $local_in;
+    my $x;
+    cast $x, $wiz;
+   };
+   $check->("$desc at eval BLOCK 1");
+
+   local $@ = $local_out ? 'xxx' : undef;
+   eval q{
+    local $@ = 'yyy' if $local_in;
+    my $x;
+    cast $x, $wiz;
+   };
+   $check->("$desc at eval STRING 1");
+
+   local $@ = $local_out ? 'xxx' : undef;
+   eval {
+    local $@ = 'yyy' if $local_in;
+    my $x;
+    my $y = \$x;
+    &cast($y, $wiz);
+   };
+   $check->("$desc at eval BLOCK 2");
+
+   local $@ = $local_out ? 'xxx' : undef;
+   eval q{
+    local $@ = 'yyy' if $local_in;
+    my $x;
+    my $y = \$x;
+    &cast($y, $wiz);
+   };
+   $check->("$desc at eval STRING 2");
+
+   local $@ = $local_out ? 'xxx' : undef;
+   eval {
+    local $@ = 'yyy' if $local_in;
+    my $x;
+    cast $x, $wiz;
+    my $y = 1;
+   };
+   $check->("$desc at eval BLOCK 3");
+
+   local $@ = $local_out ? 'xxx' : undef;
+   eval q{
+    local $@ = 'yyy' if $local_in;
+    my $x;
+    cast $x, $wiz;
+    my $y = 1;
+   };
+   $check->("$desc at eval STRING 3");
+
+   local $@ = $local_out ? 'xxx' : undef;
+   eval {
+    local $@ = 'yyy' if $local_in;
+    {
+     my $x;
+     cast $x, $wiz;
+    }
+   };
+   $check->("$desc at block in eval BLOCK");
+
+   local $@ = $local_out ? 'xxx' : undef;
+   eval q{
+    local $@ = 'yyy' if $local_in;
+    {
+     my $x;
+     cast $x, $wiz;
+    }
+   };
+   $check->("$desc at block in eval STRING");
+
+   ok defined($desc), "$desc did not over-unwind the save stack";
+  }
+ }
+}
+
 my $wiz;
 
 eval {
@@ -137,7 +232,7 @@ eval {
  cast $x, $wiz, sub { die "spinach" };
 };
 
-like $@, expect('spinach', $0), 'die in free callback';
+like $@, expect('spinach', $0), 'die in sub in free callback';
 
 eval {
  $wiz = wizard free => sub { die 'zucchini' };