Bug #133564 for Future-AsyncAwait: Attempt to free unreferenced scalar if die during pending async method

Mon Oct 19 13:13:44 2020 leonerd-cpan [...] leonerd.org.uk - Ticket created

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

Mon Oct 19 18:16:57 2020 leonerd-cpan [...] leonerd.org.uk - Correspondence added

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

Tue Oct 20 12:43:51 2020 leonerd-cpan [...] leonerd.org.uk - Correspondence added

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

Tue Oct 20 14:36:24 2020 leonerd-cpan [...] leonerd.org.uk - Correspondence added

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

Wed Oct 21 10:30:52 2020 leonerd-cpan [...] leonerd.org.uk - Correspondence added

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;

Wed Oct 21 10:30:52 2020 leonerd-cpan [...] leonerd.org.uk - Status changed from 'new' to 'patched'

Wed Oct 21 12:02:43 2020 leonerd-cpan [...] leonerd.org.uk - Queue changed from Object-Pad to Future-AsyncAwait

Sun Nov 01 12:19:19 2020 leonerd-cpan [...] leonerd.org.uk - Status changed from 'patched' to 'resolved'

Sun Nov 01 12:19:19 2020 leonerd-cpan [...] leonerd.org.uk - Fixed in 0.45 added