This is a live mirror of the Perl 5 development currently hosted at https://github.com/perl/perl5
[perl #112358] Storable: Don’t create RV with no refcnt
authorFather Chrysostomos <sprout@cpan.org>
Tue, 24 Apr 2012 23:00:36 +0000 (16:00 -0700)
committerFather Chrysostomos <sprout@cpan.org>
Mon, 21 May 2012 23:51:37 +0000 (16:51 -0700)
Otherwise assigning to it will cause the referent to be freed, because
nothing but Storable knows that it has no reference count.

Storable.xs was creating a new RV without increasing the refe-
rence count on the referent.  It was then using it to call the
STORABLE_freeze method on the object.  Since Perl passes arguments
by reference, it was possible to unref the reference by assigning to
$_[0] within STORABLE_freeze.  That would cause the object’s reference
count to go down.

dist/Storable/Storable.xs
dist/Storable/t/blessed.t

index ca6f9b4..30f9281 100644 (file)
@@ -2916,9 +2916,8 @@ static int store_hook(
 
        TRACEME(("about to call STORABLE_freeze on class %s", classname));
 
-       ref = newRV_noinc(sv);                          /* Temporary reference */
+       ref = newRV_inc(sv);                            /* Temporary reference */
        av = array_call(aTHX_ ref, hook, clone);        /* @a = $object->STORABLE_freeze($c) */
-       SvRV_set(ref, NULL);
        SvREFCNT_dec(ref);                                      /* Reclaim temporary reference */
 
        count = AvFILLp(av) + 1;
index 6657e3c..6b25f37 100644 (file)
@@ -27,7 +27,7 @@ use Storable qw(freeze thaw store retrieve);
 );
 
 my $test = 12;
-my $tests = $test + 22 + 2 * 6 * keys %::immortals;
+my $tests = $test + 23 + 2 * 6 * keys %::immortals;
 plan(tests => $tests);
 
 package SHORT_NAME;
@@ -249,3 +249,12 @@ is($STRESS_THE_STACK::freeze_count, 1);
 is($STRESS_THE_STACK::thaw_count, 1);
 isnt($t, undef);
 is(ref $t, 'STRESS_THE_STACK');
+
+{
+    package ModifyARG112358;
+    sub STORABLE_freeze { $_[0] = "foo"; }
+    my $o= {str=>bless {}};
+    my $f= ::freeze($o);
+    ::is ref $o->{str}, __PACKAGE__,
+       'assignment to $_[0] in STORABLE_freeze does not corrupt things';
+}