This queue is for tickets about the Future-AsyncAwait CPAN distribution.

Report information
The Basics
Id:
133564
Status:
resolved
Priority:
Low/Low

People
Owner:
Nobody in particular
Requestors:
leonerd-cpan [...] leonerd.org.uk
Cc:
AdminCc:

BugTracker
Severity:
(no value)
Broken in:
(no value)
Fixed in:
0.45



Subject: Attempt to free unreferenced scalar if die during pending async method
Minimal test case to reproduce second part of RT133554: #!/usr/bin/perl use strict; use warnings; use Future; use Future::AsyncAwait; use Object::Pad; class C { async method m () { await Future->new; } } my $obj = C->new; my $f = $obj->m(); die "Oopsie\n"; __END__ Oopsie Attempt to free unreferenced scalar: SV 0x55a5234296b0, Perl interpreter: 0x55a5234262a0 during global destruction. Critical is that `read_adc` is both `async` and `method`. If it's either a plain `sub`, or uses `->then` chaining without `async` then the message doesn't print. -- Paul Evans
Devel::MAT helps: Attempt to free unreferenced scalar: SV 0x56351745a6b0, Perl interpreter: 0x5635174572a0 during global destruction. $ pmat pre-die.pmat . show 0x56351745a6b0 ARRAY(0)=C at 0x56351745a6b0 with refcount 3 size 64 bytes blessed as C 0 elements (use 'elems' command to show) . inrefs 0x56351745a6b0 s a suspended SAVEt_FREESV CODE(PP) at 0x56351745a470 s a suspended SAVEt_SPTR current value CODE(PP) at 0x56351745a470 s the referrant REF() at 0x56351786a258 s the referrant REF() at 0x563517825ea0 . identify 0x56351745a6b0 ARRAY(0)=C at 0x56351745a6b0 is: ├─(via RV) a suspended pad slot of CODE(PP) at 0x56351745a470, which is (*A): │ └─(via RV) element [1] of ARRAY(2) at 0x56351786a510, which is: │ └─(via RV) element [0] of ARRAY(1) at 0x56351747d288, which is: │ └─(via RV) value {callbacks} of HASH(3)=Future at 0x56351747d2b8, which is: │ ├─(via RV) a suspended mortal of CODE(PP) at 0x56351745a470, which is: │ │ └─already found circularly as *A │ └─(via RV) element [0] of ARRAY(1) at 0x56351786a078, which is: │ └─(via RV) value {on_cancel} of HASH(3)=Future at 0x56351786a5a0, which is: │ └─(via RV) the lexical $f at depth 1 of CODE() at 0x56351745a6c8=main_cv, which is: │ └─the main code ├─(via RV) the lexical $obj at depth 1 of CODE() at 0x56351745a6c8=main_cv, which is: │ └─the main code ├─a suspended SAVEt_FREESV of CODE(PP) at 0x56351745a470, which is: │ └─already found as *A └─a suspended SAVEt_SPTR current value of CODE(PP) at 0x56351745a470, which is: └─already found as *A -- Paul Evans
Further insight, by way of adding more dump files into the test program. Here with annotations: my $obj = C->new; Devel::MAT::Dumper::dump( "d1.pmat" ); # 1: SV has refcount 1, one reference: # RV in lexical $obj of main_cv class C { async method m () { Devel::MAT::Dumper::dump( "d2.pmat" ); # 2: SV has refcount 3, three references: # RV in lexical $obj of main_cv # RV in lexical $self at depth 1 of &C::m # (directly) the lexical @(Object::Pad/slots) at depth 1 of &C::m await Future->new; } } my $f = $obj->m(); Devel::MAT::Dumper::dump( "d3.pmat" ); # 3: SV has refcount 3, four references: # RV in lexical $obj of main_cv # RV in lexical $self of suspended &C::m # a suspended SAVEt_FREESV at of suspended &C::m # a suspended SAVEt_SPTR current value of suspended &C::m die "Oopsie\n"; Devel::MAT can't (currently) see regular SAVEt_SPTR or _FREESV items on the savestack because the dumper doesn't write them out. I can add that sometime, but I suspect it would see them here. What seems to be the problem is that the refcount doesn't accurately reflect the handling of those two suspended SAVEt_* items. Unsure yet what the fix will be; whether AsyncAwait has to adjust them around suspending or cancellation of suspended; or whether Object::Pad should adjust them before it SAVE*()s them in the first place. -- Paul Evans
Some more code snippets: Object::Pad's method setup: slotsav = get_obj_slotsav(self, PL_op->op_private, create); SvREFCNT_inc(slotsav); SAVESPTR(PAD_SVl(PADIX_SLOTS)); PAD_SVl(PADIX_SLOTS) = slotsav; save_freesv(slotsav); So, a single SvREFCNT_inc() to account for the (AV)SV being now stored in the pad, a save_freesv() to tidy this up, and a SAVESPTR() to restore the prior value back into it and thus clear out the pointer when it returns. There's no method teardown code, but these things would be restored by perl's own unwind logic; e.g. this from 5.30: case SAVEt_FREESV: a0 = ap[0]; SvREFCNT_dec(a0.any_sv); break; case SAVEt_SPTR: /* SV* reference */ a0 = ap[0]; a1 = ap[1]; *a1.any_svp= a0.any_sv; break; So, while the SAVEt_FREESV keeps a strong ref on the SV, it seems SAVEt_SPTR does not. Therefore, if we cancel a suspended SAVEt_SPTR we should only SvREFCNT_dec() any counts we ourselves have artificially increased. In Future::AsyncAwait the suspend logic handles these: case SAVEt_FREESV: { PL_savestack_ix -= 2; void *sv = PL_savestack[PL_savestack_ix].any_ptr; saved->type = SAVEt_FREESV; saved->saved.sv = sv; break; } case SAVEt_SPTR: { PL_savestack_ix -= 3; SV *val = PL_savestack[PL_savestack_ix].any_ptr; SV **var = PL_savestack[PL_savestack_ix+1].any_ptr; PADOFFSET padix = var - PL_curpad; saved->type = SAVEt_SPTR; saved->u.padix = padix; saved->cur.sv = PL_curpad[padix]; /* steal ownership */ saved->saved.sv = val; /* steal ownership */ /* restore it for now */ PL_curpad[padix] = SvREFCNT_inc(val); break; } And resume does: case SAVEt_FREESV: save_freesv(saved->saved.sv); break; case SAVEt_SPTR: PL_curpad[saved->u.padix] = saved->saved.sv; SAVESPTR(PL_curpad[saved->u.padix]); SvREFCNT_dec(PL_curpad[saved->u.padix]); PL_curpad[saved->u.padix] = saved->cur.sv; break; If the suspended CV is cancelled, this does: case SAVEt_FREESV: SvREFCNT_dec(saved->saved.sv); break; case SAVEt_SPTR: SvREFCNT_dec(saved->saved.sv); SvREFCNT_dec(saved->cur.sv); break; Suspend and resume of a SAVEt_SPTR only adjust refcount of the saved->saved.sv, not ->cur; but cancellation does both. I suspect that should be changed. -- Paul Evans
Fixed by attached patch. -- Paul Evans
Subject: rt133564.patch
=== modified file 'lib/Future/AsyncAwait.xs' --- old/lib/Future/AsyncAwait.xs 2020-10-09 16:05:45 +0000 +++ new/lib/Future/AsyncAwait.xs 2020-10-21 14:27:56 +0000 @@ -498,9 +498,13 @@ break; case SAVEt_PADSV_AND_MORTALIZE: + SvREFCNT_dec(saved->saved.sv); + SvREFCNT_dec(saved->cur.sv); + break; + case SAVEt_SPTR: SvREFCNT_dec(saved->saved.sv); - SvREFCNT_dec(saved->cur.sv); + /* saved->cur.sv does not account for an extra refcount */ break; default: === modified file 't/80async-method.t' --- old/t/80async-method.t 2020-07-06 21:44:29 +0000 +++ new/t/80async-method.t 2020-10-21 14:27:56 +0000 @@ -56,4 +56,16 @@ is( $fret->get, "result", '$fret for await in async method' ); } +# RT133564 +{ + # Hard to test this one but running the test itself shouldn't produce any + # warnings of "Attempt to free unreferenced scalar ..." + my $thunker = Thunker->new; + eval { + my $f = $thunker->thunk( Future->new ); + die "Oopsie\n"; + }; + ok( 1, "No segfault for RT133564 test" ); +} + done_testing;


This service runs on Request Tracker, is sponsored by The Perl Foundation, and maintained by Best Practical Solutions.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.