Front page | perl.perl5.porters |
Postings from May 2013
On Thu, May 23, 2013 at 06:07:16PM +0200, Leon Timmermans wrote:
> On Thu, May 23, 2013 at 5:30 PM, rurban@cpanel.net
> <perlbug-followup@perl.org> 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.
1..1
# 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 Test.pm 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/vgpreload_memcheck-amd64-linux.so)
==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)
etc.
The XS blesses the internal context object into class Storable::Cxt, and
that has a destructor:
MODULE = Storable PACKAGE = Storable::Cxt
void
DESTROY(self)
SV *self
PREINIT:
stcxt_t *cxt = (stcxt_t *)SvPVX(SvRV(self));
PPCODE:
if (kbuf)
Safefree(kbuf);
if (!cxt->membuf_ro && mbase)
Safefree(mbase);
if (cxt->membuf_ro && (cxt->msaved).arena)
Safefree((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
object.
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