Bug in Safe 2.21 (or Opcode) re propagating exceptions

In the context of PostgreSQL PL/Perl, which uses Safe, David Wheeler
pointed me to a bug in Safe 2.21. The effect is that an exception thrown
from a closure gets lost.

I've boiled it down to this:

    perl -MSafe -e 'Safe->new->reval(q{sub { die @_ }})->(qq{ok\n})'

That should die with "ok" but doesn't on 5.10.1 or 5.11.4 or 5.8.6.
It does die, correctly, on a whole bunch of other versions I have
lying around locally.

This is a heads up as I'm starting to dig. Any thoughts most welcome...

Tim.

Turns out to be a threads issue. Fails with USE_ITHREADS, passes otherwise.
(I had a mix of perl lying around with various configs.)

Still digging...

Tim.

git bisect is probably your friend.

There's a section describing how to do it in perlrepository.pod:

http://perl5.git.perl.org/perl.git/blob/0972ecdf:/pod/perlrepository.pod#l626

Nicholas Clark

Thanks. But I din't think a bisect will be helpful here.  Safe 2.20+
fails for all perl versions (with threads enabled) that I've tried it on.

The problem is that the closure that wraps any returned code ref if
threads are enabled is acting as an eval block so hiding the exception.

This mostly fixes it:

--- a/Safe.pm
+++ b/Safe.pm
@@ -311,7 +311,15 @@ sub reval {
             $ret = sub {
                 my @args = @_; # lexical to close over
                 my $sub_with_args = sub { $sub->(@args) };
- return Opcode::_safe_call_sv($root, $obj->{Mask}, $sub_with_args)
+                undef $@; # needed due to perl_call_sv(sv, G_EVAL|G_KEEPERR)
+                my @subret = (wantarray)
+ ? Opcode::_safe_call_sv($root, $obj->{Mask}, $sub_with_args)
+ : scalar Opcode::_safe_call_sv($root, $obj->{Mask}, $sub_with_args);
+ if ($@) { # rethrow exception after removing prefix added by G_KEEPERR
+                    $@ =~ s/\t\(in cleanup\) //;
+                    die $@;
+                }
+                return (wantarray) ? @subret : $subret[0];
             };
         }
     }

One remaining problem is that G_KEEPERR 'helpfully' generates a warning.
Per perlcall docs:

    In addition, a warning is generated using the appended string.
    This can be disabled using "no warnings 'misc'".

I tried adding no warnings 'misc' in various places by I couldn't
silence it. Any thoughts?

Tim.

I've attached a more complete patch.

Rafaël, I think this is ready to apply and release as Safe 2.22.
(Which should then be inclded in 5.11.5.)

> One remaining problem is that G_KEEPERR 'helpfully' generates a warning.
> Per perlcall docs:
>
> In addition, a warning is generated using the appended string.
> This can be disabled using "no warnings 'misc'".
>
> I tried adding no warnings 'misc' in various places by I couldn't
> silence it. Any thoughts?

It looks like there isn't a good simple fix for this. The warning comes
from Perl_die_where(). So the prefix is added in, and controlled-by, the
lexical context that threw the exception - outside the control of this code.

Seems like G_KEEPERR is conflating separate issues :( Perhaps there's a
need for a G_KEEPERR_BUT_DONT_WARN, though that won't help existing perls.

Tim.

Attachment: safe-2.21-rethrow.patch

Goodness... For how simple I thought this idea was originally it sure
has caused a lot of grief :(

FWIW I took a quick look and it looks sane to me.  Although I dont
have any threaded perls lying around to test with atm...

On Wed, Feb 10, 2010 at 05:52:31PM +0000, Tim Bunce wrote:

> Rafaël, I think this is ready to apply and release as Safe 2.22.
> (Which should then be inclded in 5.11.5.)

I've forwarded it all to rt.perl.org so that it doesn't get lost.

http://rt.perl.org/rt3/Ticket/Display.html?id=72700

Nicholas Clark

On 10 February 2010 18:52, Tim Bunce <Tim.Bunce@pobox.com> wrote:
> On Wed, Feb 10, 2010 at 04:38:58PM +0000, Tim Bunce wrote:
>> On Tue, Feb 09, 2010 at 02:44:57PM +0000, Nicholas Clark wrote:
>> > On Tue, Feb 09, 2010 at 02:41:44PM +0000, Tim Bunce wrote:
>> > > On Mon, Feb 08, 2010 at 04:38:48PM +0000, Tim Bunce wrote:
>> > > > In the context of PostgreSQL PL/Perl, which uses Safe, David Wheel=
er
>> > > > pointed me to a bug in Safe 2.21. The effect is that an exception =
thrown
>> > > > from a closure gets lost.
>> > > >
>> > > > I've boiled it down to this:
>> > > >
>> > > > =A0 =A0 perl -MSafe -e 'Safe->new->reval(q{sub { die @_ }})->(qq{o=
k\n})'
>> > > >
>> > > > That should die with "ok" but doesn't on 5.10.1 or 5.11.4 or 5.8.6=
.
>> > > > It does die, correctly, on a whole bunch of other versions I have
>> > > > lying around locally.
>> > > >
>> > > > This is a heads up as I'm starting to dig. Any thoughts most welco=
me...
>> > >
>> > > Turns out to be a threads issue. Fails with USE_ITHREADS, passes oth=
erwise.
>> > > (I had a mix of perl lying around with various configs.)
>> > >
>> > > Still digging...
>> >
>> > git bisect is probably your friend.
>> >
>> > There's a section describing how to do it in perlrepository.pod:
>> >
>> > http://perl5.git.perl.org/perl.git/blob/0972ecdf:/pod/perlrepository.p=
od#l626
>>
>> Thanks. But I din't think a bisect will be helpful here. =A0Safe 2.20+
>> fails for all perl versions (with threads enabled) that I've tried it on=
.
>>
>> The problem is that the closure that wraps any returned code ref if
>> threads are enabled is acting as an eval block so hiding the exception.
>>
>> This mostly fixes it:
>
> I've attached a more complete patch.
>
> Rafa=EBl, I think this is ready to apply and release as Safe 2.22.
> (Which should then be inclded in 5.11.5.)

Thanks, applied to bleadperl. I'll release a new Safe to CPAN soonish.