develooper Front page | perl.perl5.porters | Postings from May 2013

Re: [perl #118139] Storable in DESTROY blocks

Thread Previous | Thread Next
Nicholas Clark
May 28, 2013 15:29
Re: [perl #118139] Storable in DESTROY blocks
Message ID:
On Thu, May 23, 2013 at 06:07:16PM +0200, Leon Timmermans wrote:
> On Thu, May 23, 2013 at 5:30 PM,
> <> wrote:
> > Storable segfaults when being used in DESTROY blocks during global
> > destruction, when accessing an already freed PL_modglobal or the internal
> > ctx.
> > The best fix is to die if used in global destruction.
> > See the new test t/destroy.t, which crashes in all perl versions.
> >
> > Since this Storable fix needs to be released on CPAN also, I had to
> > add some compatibility fixes, tested back to 5.6.
> This doesn't make sense to me. perl deliberately destroys all objects
> before destroying anything else. PL_modglobal should still exist when
> any DESTROY is run.

It's the internal context.

# Running under perl version 5.019001 for linux
# Current time local: Tue May 28 17:07:37 2013
# Current time GMT:   Tue May 28 15:07:37 2013
# Using version 1.26
ok 1
==6600== Invalid read of size 4
==6600==    at 0x63859B7: do_retrieve (Storable.xs:6068)
==6600==    by 0x6386ABF: pretrieve (Storable.xs:6273)
==6600==    by 0x6387425: XS_Storable_pretrieve (Storable.xs:6482)
==6600==    by 0x57B31F: Perl_pp_entersub (pp_hot.c:2881)
==6600==    by 0x51EB9D: Perl_runops_debug (dump.c:2213)
==6600==    by 0x4564A1: Perl_call_sv (perl.c:2769)
==6600==    by 0x5B1164: S_curse (sv.c:6482)
==6600==    by 0x5AE451: Perl_sv_clear (sv.c:6117)
==6600==    by 0x5B175C: Perl_sv_free2 (sv.c:6583)
==6600==    by 0x57DE9D: S_SvREFCNT_dec_NN (inline.h:84)
==6600==    by 0x57E88E: do_clean_objs (sv.c:480)
==6600==    by 0x57E4B1: S_visit (sv.c:422)
==6600==  Address 0x604bb88 is 120 bytes inside a block of size 272 free'd
==6600==    at 0x4C2BA6C: free (in /usr/lib/valgrind/
==6600==    by 0x51F605: Perl_safesysfree (util.c:276)
==6600==    by 0x5B0355: Perl_sv_clear (sv.c:6313)
==6600==    by 0x5B175C: Perl_sv_free2 (sv.c:6583)
==6600==    by 0x57DE9D: S_SvREFCNT_dec_NN (inline.h:84)
==6600==    by 0x57E88E: do_clean_objs (sv.c:480)
==6600==    by 0x57E4B1: S_visit (sv.c:422)
==6600==    by 0x57F941: Perl_sv_clean_objs (sv.c:577)
==6600==    by 0x450BDD: perl_destruct (perl.c:766)
==6600==    by 0x41D794: main (perlmain.c:125)


The XS blesses the internal context object into class Storable::Cxt, and
that has a destructor:

MODULE = Storable	PACKAGE = Storable::Cxt

    SV *self
	stcxt_t *cxt = (stcxt_t *)SvPVX(SvRV(self));
	if (kbuf)
	if (!cxt->membuf_ro && mbase)
	if (cxt->membuf_ro && (cxt->msaved).arena)

To de-obfuscate the above code you need to know this:

#define kbuf	(cxt->keybuf).arena
#define mbase	(cxt->membuf).arena

Storable isn't helping itself by making a *reference* to the blessed context

I've not fully worked this out, but I think that it's only using bless to
ensure that it gets to run that code to call free(). In turn, given that
cxt->membuf_ro gets to select one of two places to have a pointer to
(seemingly) the same thing, I'm wondering if that can be simplified. At
which point there are only two dynamic sized buffers, and one fixed
structure. Which seems equivalent to two dynamic sized buffers. Which might
be possible to implement without needing malloc(), free(), bless and DESTROY.

However, I also *think* that Reini's patch is better than what we have now,
and does not conflict with getting to a better place. But I'm not doing
"thinking" very well today as my head is badly stuffed up.

Also, I'd be tempted to split the patch into the 5.6 fixups and the bug fix.
If I stop feeling stuffed up I'll do that.

Nicholas Clark

Thread Previous | Thread Next Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at | Group listing | About