Skip Menu |
Report information
Id: 118139
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors:
Cc:
AdminCc:

Operating System: Linux
PatchStatus: (no value)
Severity: medium
Type: library
Perl Version: 5.18.0
Fixed In: (no value)



Subject: Storable in DESTROY blocks
Date: Thu, 23 May 2013 10:29:38 -0500
To: perlbug [...] perl.org
From: rurban [...] cpanel.net
Download (untitled) / with headers
text/plain 4.9k
This is a bug report for perl from rurban@cpanel.net, generated with the help of perlbug 1.39 running under perl 5.18.0. ----------------------------------------------------------------- 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. ----------------------------------------------------------------- --- Flags: category=library severity=medium module=Storable --- This perlbug was built using Perl 5.17.11 - Mon Apr 8 10:30:38 CDT 2013 It is being executed now by Perl 5.18.0 - Mon May 20 17:25:09 CDT 2013. Site configuration information for perl 5.18.0: Configured by rurban at Mon May 20 17:25:09 CDT 2013. Summary of my perl5 (revision 5 version 18 subversion 0) configuration: Platform: osname=linux, osvers=3.2.0-4-amd64, archname=x86_64-linux uname='linux reini 3.2.0-4-amd64 #1 smp debian 3.2.41-2 x86_64 gnulinux ' config_args='-de -Dusedevel -Uversiononly -Dinstallman1dir=none -Dinstallman3dir=none -Dinstallsiteman1dir=none -Dinstallsiteman3dir=none -Uuseithreads -Accflags=''-msse4.2'' -Accflags=''-march=corei7'' -Dcf_email=''rurban@cpanel.net'' -Dperladmin=''rurban@cpanel.net''' hint=recommended, useposix=true, d_sigaction=define useithreads=undef, usemultiplicity=undef useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef use64bitint=define, use64bitall=define, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='cc', ccflags ='-msse4.2 -march=corei7 -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64', optimize='-O2', cppflags='-msse4.2 -march=corei7 -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion='', gccversion='4.7.2', gccosandvers='' intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16 ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=8, prototype=define Linker and Libraries: ld='cc', ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /usr/lib libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc libc=, so=so, useshrplib=false, libperl=libperl.a gnulibc_version='2.13' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E' cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector' Locally applied patches: --- @INC for perl 5.18.0: /usr/local/lib/perl5/site_perl/5.18.0/x86_64-linux /usr/local/lib/perl5/site_perl/5.18.0 /usr/local/lib/perl5/5.18.0/x86_64-linux /usr/local/lib/perl5/5.18.0 /usr/local/lib/perl5/site_perl/5.17.11 /usr/local/lib/perl5/site_perl/5.17.10 /usr/local/lib/perl5/site_perl/5.17.8 /usr/local/lib/perl5/site_perl/5.17.7 /usr/local/lib/perl5/site_perl/5.17.6 /usr/local/lib/perl5/site_perl/5.17.5 /usr/local/lib/perl5/site_perl/5.17.4 /usr/local/lib/perl5/site_perl/5.17.3 /usr/local/lib/perl5/site_perl/5.17.2 /usr/local/lib/perl5/site_perl/5.17.1 /usr/local/lib/perl5/site_perl/5.17.0 /usr/local/lib/perl5/site_perl/5.17 /usr/local/lib/perl5/site_perl/5.16.3 /usr/local/lib/perl5/site_perl/5.16.2 /usr/local/lib/perl5/site_perl/5.16.1 /usr/local/lib/perl5/site_perl/5.16.0 /usr/local/lib/perl5/site_perl/5.15.9 /usr/local/lib/perl5/site_perl/5.15.8 /usr/local/lib/perl5/site_perl/5.15.7 /usr/local/lib/perl5/site_perl/5.15.6 /usr/local/lib/perl5/site_perl/5.15.5 /usr/local/lib/perl5/site_perl/5.15.4 /usr/local/lib/perl5/site_perl/5.14.4 /usr/local/lib/perl5/site_perl/5.14.3 /usr/local/lib/perl5/site_perl/5.14.2 /usr/local/lib/perl5/site_perl/5.14.1 /usr/local/lib/perl5/site_perl/5.12.4 /usr/local/lib/perl5/site_perl/5.10.1 /usr/local/lib/perl5/site_perl/5.8.9 /usr/local/lib/perl5/site_perl/5.8.8 /usr/local/lib/perl5/site_perl/5.8.7 /usr/local/lib/perl5/site_perl/5.8.6 /usr/local/lib/perl5/site_perl/5.8.5 /usr/local/lib/perl5/site_perl/5.8.4 /usr/local/lib/perl5/site_perl/5.8.3 /usr/local/lib/perl5/site_perl/5.8.2 /usr/local/lib/perl5/site_perl/5.8.1 /usr/local/lib/perl5/site_perl/5.6.2 /usr/local/lib/perl5/site_perl . --- Environment for perl 5.18.0: HOME=/home/rurban LANG=en_US.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/home/rurban/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games PERL_BADLANG (unset) SHELL=/bin/bash
RT-Send-CC: perl5-porters [...] perl.org, ams [...] toroid.org
Download (untitled) / with headers
text/plain 330b
The attached patch fixes the DESTROY issue. This is a pretty rare use case, but deserves a CPAN release also. use Storable; BEGIN { store {}, "foo"; } package foo; sub new { return bless {} } DESTROY { open $fh, "<", "foo"; eval { Storable::pretrieve($fh); }; #SEGV unlink "foo"; } package main; foo->new(); -- Reini Urban
From fb66b956b9f00e34ebf6b41b197ba539e8c62126 Mon Sep 17 00:00:00 2001 From: Reini Urban <rurban@x-ray.at> Date: Thu, 23 May 2013 10:31:58 -0500 Subject: [PATCH] [perl #118139] Storable 2.42 - die in DESTROY --- dist/Storable/ChangeLog | 19 ++++++++++++++- dist/Storable/Storable.pm | 16 +++++++++--- dist/Storable/Storable.xs | 59 +++++++++++++++++++++++++++++++++++++-------- dist/Storable/t/destroy.t | 21 ++++++++++++++++ 4 files changed, 100 insertions(+), 15 deletions(-) create mode 100644 dist/Storable/t/destroy.t diff --git a/dist/Storable/ChangeLog b/dist/Storable/ChangeLog index 31e1b0c..ceaf128 100644 --- a/dist/Storable/ChangeLog +++ b/dist/Storable/ChangeLog @@ -1,4 +1,21 @@ -? +Thu May 23 09:59:33 CDT 2013 Reini Urban <rurban@cpanel.net> + + Protect against SEGV during global destruction when used + in DESTROY blocks [perl #118139]. + Compatibility fixes back to 5.6. + +Wed May 8 18:27:42 2013 -0600 Karl Williamson <public@khwilliamson.com> + Version 2.42 + + Remove core references to SVt_BIND (no changes) + +Sat Apr 13 17:45:10 2013 +0100 James E Keenan <jkeenan@cpan.org> + Version 2.41 + + Update INSTALLDIRS to favor installation under 'site' + (no changes) + +Fri Nov 30 23:03:09 2012 -0500 Steffen Mueller <smueller@cpan.org> Version 2.40 Security warnings section added diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm index 0cc5d16..327dcc1 100644 --- a/dist/Storable/Storable.pm +++ b/dist/Storable/Storable.pm @@ -1,5 +1,6 @@ # -# Copyright (c) 1995-2000, Raphael Manfredi +# Copyright (c) 1995-2001, Raphael Manfredi +# Copyright (c) 2002-2013 by the Perl 5 Porters # # You may redistribute only under the same terms as Perl 5, as specified # in the README file that comes with the distribution. @@ -659,6 +660,9 @@ routine from the C<Log::Agent> package, if it is available. Normal errors are reported by having store() or retrieve() return C<undef>. Such errors are usually I/O errors (or truncated stream errors at retrieval). +Storable will die not croak if used during global destruction. It is +forbidden to use Storable functions in C<DESTROY {}> blocks. + =head1 WIZARDS ONLY =head2 Hooks @@ -1120,6 +1124,9 @@ to string or floating point. In other words values that had been generated by integer operations such as logic ops and then not used in any string or arithmetic context before storing. +You may not use Storable in C<DESTROY {}> blocks, because during global +destruction the internal context is already destroyed. + =head2 64 bit data in perl 5.6.0 and 5.6.1 This section only applies to you if you have existing data written out @@ -1192,7 +1199,8 @@ Thank you to (in chronological order): Salvador Ortiz Garcia <sog@msg.com.mx> Dominic Dunlop <domo@computer.org> Erik Haugan <erik@solbors.no> - Benjamin A. Holzman <ben.holzman@grantstreet.com> + Benjamin A. Holzman <ben.holzman@grantstreet.com> + Reini Urban <rurban@cpanel.net> for their bug reports, suggestions and contributions. @@ -1205,8 +1213,8 @@ a binary incompatibility for the Storable image starting at version 0.6--older images are, of course, still properly understood). Murray Nesbitt made Storable thread-safe. Marc Lehmann added overloading and references to tied items support. Benjamin Holzman added a performance -improvement for overloaded classes; thanks to Grant Street Group for footing -the bill. +improvement for overloaded classes. Reini Urban protected against crashes +in global destruction; thanks to Grant Street Group for footing the bill. =head1 AUTHOR diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs index 08641cd..e259908 100644 --- a/dist/Storable/Storable.xs +++ b/dist/Storable/Storable.xs @@ -23,6 +23,9 @@ #define NEED_newCONSTSUB #define NEED_newSVpvn_flags #define NEED_newRV_noinc +#define NEED_PL_parser +#define NEED_sv_2pv_flags +#define NEED_SvIsCOW #include "ppport.h" /* handle old perls */ #endif @@ -81,6 +84,14 @@ # define HvTOTALKEYS(hv) HvKEYS(hv) #endif +#if !defined(PERL_CORE) && !defined(PL_dirty) && (PERL_VERSION >= 14) +# define PL_dirty (PL_phase == PERL_PHASE_DESTRUCT) +#endif + +#ifndef SvIsCOW +# define SvIsCOW(x) 0 +#endif + #ifdef DEBUGME #ifndef DASSERT @@ -348,19 +359,22 @@ typedef struct stcxt { #if defined(MULTIPLICITY) || defined(PERL_OBJECT) || defined(PERL_CAPI) #if (PATCHLEVEL <= 4) && (SUBVERSION < 68) -#define dSTCXT_SV \ - SV *perinterp_sv = perl_get_sv(MY_VERSION, 0) +#define dSTCXT_SV \ + SV *perinterp_sv = NULL; \ + if (!PL_dirty) \ + perinterp_sv = perl_get_sv(MY_VERSION, 0) #else /* >= perl5.004_68 */ -#define dSTCXT_SV \ - SV *perinterp_sv = *hv_fetch(PL_modglobal, \ - MY_VERSION, sizeof(MY_VERSION)-1, TRUE) +#define dSTCXT_SV \ + SV *perinterp_sv = NULL; \ + if (!PL_dirty) \ + perinterp_sv = *hv_fetch(PL_modglobal, MY_VERSION, sizeof(MY_VERSION)-1, TRUE) #endif /* < perl5.004_68 */ #define dSTCXT_PTR(T,name) \ T name = ((perinterp_sv && SvIOK(perinterp_sv) && SvIVX(perinterp_sv) \ ? (T)SvPVX(SvRV(INT2PTR(SV*,SvIVX(perinterp_sv)))) : (T) 0)) -#define dSTCXT \ - dSTCXT_SV; \ +#define dSTCXT \ + dSTCXT_SV; \ dSTCXT_PTR(stcxt_t *, cxt) #define INIT_STCXT \ @@ -2259,7 +2273,16 @@ sortcmp(const void *a, const void *b) static int store_hash(pTHX_ stcxt_t *cxt, HV *hv) { dVAR; - I32 len = HvTOTALKEYS(hv); + I32 len = +#if PERL_VERSION < 8 + HvKEYS(hv); +#else +# ifdef HAS_RESTRICTED_HASHES + HvTOTALKEYS(hv); +# else + HvUSEDKEYS(hv); +# endif +#endif I32 i; int ret = 0; I32 riter; @@ -3731,7 +3754,11 @@ static int do_store( TRACEME(("do_store (optype=%d, netorder=%d)", optype, network_order)); - optype |= ST_STORE; + if (PL_dirty) { /* already freed cxt */ + Perl_die(aTHX_ "Can't store with Storable during global destruction"); /* croak too instable here */ + return 0; + } + optype |= ST_STORE; /* * Workaround for CROAK leak: if they enter with a "dirty" context, @@ -6047,6 +6074,11 @@ static SV *do_retrieve( TRACEME(("do_retrieve (optype = 0x%x)", optype)); + if (PL_dirty) { /* already freed cxt */ + Perl_die(aTHX_ "Can't retrieve with Storable during global destruction"); /* croak too instable here */ + return &PL_sv_undef; + } + optype |= ST_RETRIEVE; /* @@ -6305,6 +6337,10 @@ static SV *dclone(pTHX_ SV *sv) SV *out; TRACEME(("dclone")); + if (PL_dirty) { /* already freed cxt */ + Perl_die(aTHX_ "Can't clone with Storable during global destruction"); /* croak too instable here */ + return &PL_sv_undef; + } /* * Workaround for CROAK leak: if they enter with a "dirty" context, @@ -6507,9 +6543,12 @@ last_op_in_netorder() PREINIT: bool result; PPCODE: + if (PL_dirty) { + Perl_die(aTHX_ "Can't use Storable during global destruction"); + XSRETURN(0); + } if (ix) { dSTCXT; - result = cxt->entry && (cxt->optype & ix) ? TRUE : FALSE; } else { result = !!last_op_in_netorder(aTHX); diff --git a/dist/Storable/t/destroy.t b/dist/Storable/t/destroy.t new file mode 100644 index 0000000..953e105 --- /dev/null +++ b/dist/Storable/t/destroy.t @@ -0,0 +1,21 @@ +# [perl #118139] crash in global destruction when accessing an +# already freed PL_modglobalor accessing the freed cxt. +use Test; +use Storable; +BEGIN { + plan tests => 1; + store {}, "foo"; +} +package foo; +sub new { return bless {} } +DESTROY { + open $fh, "<", "foo"; + eval { Storable::pretrieve($fh); }; + unlink "foo"; +} + +package main; +# print "# $^X\n"; +foo->new(); + +ok(1); -- 1.7.10.4
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Thu, 23 May 2013 17:45:10 +0200
To: perl5-porters [...] perl.org
From: Steffen Mueller <smueller [...] cpan.org>
Download (untitled) / with headers
text/plain 1.2k
On 05/23/2013 05:30 PM, rurban@cpanel.net (via RT) wrote: Show quoted text
> # New Ticket Created by rurban@cpanel.net > # Please include the string: [perl #118139] > # in the subject line of all future correspondence about this issue. > # <URL: https://rt.perl.org:443/rt3/Ticket/Display.html?id=118139 > > > > > This is a bug report for perl from rurban@cpanel.net, > generated with the help of perlbug 1.39 running under perl 5.18.0. > > > ----------------------------------------------------------------- > 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.
There's a more general problem with this: occasionally, C extensions need to do cleanup very late during global destruction. Usually just a free() or Safefree() to be done after all Perl code is done. Maybe adding a C level hook for this could allow Storable to work around this problem as well? Given what you write, I think maybe not that easily. --Steffen
CC: perl5-porters [...] perl.org
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Thu, 23 May 2013 17:57:30 +0200
To: Steffen Mueller <smueller [...] cpan.org>
From: Leon Timmermans <fawaka [...] gmail.com>
Download (untitled) / with headers
text/plain 590b
On Thu, May 23, 2013 at 5:45 PM, Steffen Mueller <smueller@cpan.org> wrote: Show quoted text
> There's a more general problem with this: occasionally, C extensions need to > do cleanup very late during global destruction. Usually just a free() or > Safefree() to be done after all Perl code is done. Maybe adding a C level > hook for this could allow Storable to work around this problem as well? > Given what you write, I think maybe not that easily.
That would not solve this problem. This problem is not in a DESTROY from Storable, but from other code calling Storable from they DESTROY methods. Leon
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 676b
On Thu May 23 08:36:26 2013, rurban wrote: Show quoted text
> The attached patch fixes the DESTROY issue. > This is a pretty rare use case, but deserves a CPAN release also. > > use Storable; > BEGIN { store {}, "foo"; } > package foo; > sub new { return bless {} } > DESTROY { open $fh, "<", "foo"; > eval { Storable::pretrieve($fh); }; #SEGV > unlink "foo"; > } > package main; > foo->new();
I"m not sure how to document the easy fix for the user in this use-case: Call the objects DESTROY method before global destruction. E.g. by setting foo->new() to a lexical, my $x = foo->new(); or by using it in a block, and it will be destroyed when going out of scope. -- Reini Urban
CC: bugs-bitbucket [...] rt.perl.org
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Thu, 23 May 2013 18:07:16 +0200
To: perl5-porters [...] perl.org
From: Leon Timmermans <fawaka [...] gmail.com>
Download (untitled) / with headers
text/plain 654b
On Thu, May 23, 2013 at 5:30 PM, rurban@cpanel.net <perlbug-followup@perl.org> wrote: Show quoted text
> 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. Leon
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 2.7k
On Thu May 23 09:08:29 2013, LeonT wrote: Show quoted text
> 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 Show quoted text
> > 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.
gdb --args /usr/local/bin/perl5.18.0d -Mblib t/destroy.t Program received signal SIGSEGV, Segmentation fault. 0x00007ffff7e395d0 in do_retrieve (my_perl=0x816010, f=0x831b68, in=0x0, optype=0) at Storable.xs:5997 5997 dSTCXT; (gdb) bt #0 0x00007ffff7e395d0 in do_retrieve (my_perl=0x816010, f=0x831b68, in=0x0, optype=0) at Storable.xs:5997 #1 0x00007ffff7e3a8e0 in pretrieve (my_perl=0x816010, f=0x831b68) at Storable.xs:6227 #2 0x00007ffff7e3ba4b in XS_Storable_pretrieve (my_perl=0x816010, cv=0xa83ae8) at Storable.xs:6436 #3 0x00000000005aa708 in Perl_pp_entersub (my_perl=0x816010) at pp_hot.c:2875 #4 0x000000000054120e in Perl_runops_debug (my_perl=0x816010) at dump.c:2213 #5 0x000000000045e067 in Perl_call_sv (my_perl=0x816010, sv=0x944410, flags=45) at perl.c:2766 #6 0x00000000005e1eae in S_curse (my_perl=0x816010, sv=0x819268, check_refcnt=true) at sv.c:6482 #7 0x00000000005df139 in Perl_sv_clear (my_perl=0x816010, orig_sv=0x819268) at sv.c:6117 #8 0x00000000005e2595 in Perl_sv_free2 (my_perl=0x816010, sv=0x819268, rc=1) at sv.c:6583 #9 0x00000000005ad71f in S_SvREFCNT_dec_NN (my_perl=0x816010, sv=0x819268) at inline.h:84 #10 0x00000000005ae1f7 in do_clean_objs (my_perl=0x816010, ref=0x9f8158) at sv.c:480 #11 0x00000000005addc3 in S_visit (my_perl=0x816010, f=0x5ade78 <do_clean_objs>, flags=2048, mask=2048) at sv.c:422 #12 0x00000000005af48a in Perl_sv_clean_objs (my_perl=0x816010) at sv.c:577 #13 0x0000000000456d70 in perl_destruct (my_perl=0x816010) at perl.c:766 #14 0x000000000041dcee in main (argc=3, argv=0x7fffffffe698, env=0x7fffffffe6b8) at perlmain.c:125 $ make Storable.i $ grep dSTCXT Storable.i #define dSTCXT_SV SV *perinterp_sv = *hv_fetch(PL_modglobal, MY_VERSION, sizeof(MY_VERSION)-1, TRUE) #define dSTCXT_PTR(T,name) T name = ((perinterp_sv && SvIOK(perinterp_sv) && SvIVX(perinterp_sv) ? (T)SvPVX(SvRV(INT2PTR(SV*,SvIVX(perinterp_sv)))) : (T) 0)) #define dSTCXT dSTCXT_SV; dSTCXT_PTR(stcxt_t *, cxt) But my testcase is too optimized to reproduce the bug. It fails with the last line changed to: - foo->new(); + $x = foo->new(); -- Reini Urban
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 348b
Attached is a revised patch. Improved the doc and revise the testcase to produce the SEGV. The problem is not the DESTROY block per se, but DESTROY being called during global destruction. Show quoted text
> But my testcase is too optimized to reproduce the bug. > It fails with the last line changed to: > > - foo->new(); > + $x = foo->new();
-- Reini Urban
From 8caa4ba424adf6f70de04f96f27d92d7ab84f820 Mon Sep 17 00:00:00 2001 From: Reini Urban <rurban@x-ray.at> Date: Thu, 23 May 2013 10:31:58 -0500 Subject: [PATCH] [perl #118139] Storable 2.42 - die in global destruction Protect against SEGV during global destruction, e.g. when used in DESTROY blocks. Compatibility fixes back to 5.6. --- dist/Storable/ChangeLog | 19 ++++++++++++++- dist/Storable/Storable.pm | 18 +++++++++++--- dist/Storable/Storable.xs | 59 +++++++++++++++++++++++++++++++++++++-------- dist/Storable/t/destroy.t | 21 ++++++++++++++++ 4 files changed, 102 insertions(+), 15 deletions(-) create mode 100644 dist/Storable/t/destroy.t diff --git a/dist/Storable/ChangeLog b/dist/Storable/ChangeLog index 31e1b0c..ceaf128 100644 --- a/dist/Storable/ChangeLog +++ b/dist/Storable/ChangeLog @@ -1,4 +1,21 @@ -? +Thu May 23 09:59:33 CDT 2013 Reini Urban <rurban@cpanel.net> + + Protect against SEGV during global destruction when used + in DESTROY blocks [perl #118139]. + Compatibility fixes back to 5.6. + +Wed May 8 18:27:42 2013 -0600 Karl Williamson <public@khwilliamson.com> + Version 2.42 + + Remove core references to SVt_BIND (no changes) + +Sat Apr 13 17:45:10 2013 +0100 James E Keenan <jkeenan@cpan.org> + Version 2.41 + + Update INSTALLDIRS to favor installation under 'site' + (no changes) + +Fri Nov 30 23:03:09 2012 -0500 Steffen Mueller <smueller@cpan.org> Version 2.40 Security warnings section added diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm index 0cc5d16..c203eb9 100644 --- a/dist/Storable/Storable.pm +++ b/dist/Storable/Storable.pm @@ -1,5 +1,6 @@ # -# Copyright (c) 1995-2000, Raphael Manfredi +# Copyright (c) 1995-2001, Raphael Manfredi +# Copyright (c) 2002-2013 by the Perl 5 Porters # # You may redistribute only under the same terms as Perl 5, as specified # in the README file that comes with the distribution. @@ -659,6 +660,10 @@ routine from the C<Log::Agent> package, if it is available. Normal errors are reported by having store() or retrieve() return C<undef>. Such errors are usually I/O errors (or truncated stream errors at retrieval). +Storable will die not croak if used during global destruction. It is +dangerous to use Storable functions in C<DESTROY {}> blocks, esp. when +the DESTROY method is being called during global destruction and not earlier. + =head1 WIZARDS ONLY =head2 Hooks @@ -1120,6 +1125,10 @@ to string or floating point. In other words values that had been generated by integer operations such as logic ops and then not used in any string or arithmetic context before storing. +You should not use Storable in C<DESTROY {}> blocks, or explcitly call the +DESTROY method at run-time (e.g. set the object to undef or make it a lexical), +because during global destruction it will fail. + =head2 64 bit data in perl 5.6.0 and 5.6.1 This section only applies to you if you have existing data written out @@ -1192,7 +1201,8 @@ Thank you to (in chronological order): Salvador Ortiz Garcia <sog@msg.com.mx> Dominic Dunlop <domo@computer.org> Erik Haugan <erik@solbors.no> - Benjamin A. Holzman <ben.holzman@grantstreet.com> + Benjamin A. Holzman <ben.holzman@grantstreet.com> + Reini Urban <rurban@cpanel.net> for their bug reports, suggestions and contributions. @@ -1205,8 +1215,8 @@ a binary incompatibility for the Storable image starting at version 0.6--older images are, of course, still properly understood). Murray Nesbitt made Storable thread-safe. Marc Lehmann added overloading and references to tied items support. Benjamin Holzman added a performance -improvement for overloaded classes; thanks to Grant Street Group for footing -the bill. +improvement for overloaded classes. Reini Urban protected against crashes +in global destruction; thanks to Grant Street Group for footing the bill. =head1 AUTHOR diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs index 08641cd..e259908 100644 --- a/dist/Storable/Storable.xs +++ b/dist/Storable/Storable.xs @@ -23,6 +23,9 @@ #define NEED_newCONSTSUB #define NEED_newSVpvn_flags #define NEED_newRV_noinc +#define NEED_PL_parser +#define NEED_sv_2pv_flags +#define NEED_SvIsCOW #include "ppport.h" /* handle old perls */ #endif @@ -81,6 +84,14 @@ # define HvTOTALKEYS(hv) HvKEYS(hv) #endif +#if !defined(PERL_CORE) && !defined(PL_dirty) && (PERL_VERSION >= 14) +# define PL_dirty (PL_phase == PERL_PHASE_DESTRUCT) +#endif + +#ifndef SvIsCOW +# define SvIsCOW(x) 0 +#endif + #ifdef DEBUGME #ifndef DASSERT @@ -348,19 +359,22 @@ typedef struct stcxt { #if defined(MULTIPLICITY) || defined(PERL_OBJECT) || defined(PERL_CAPI) #if (PATCHLEVEL <= 4) && (SUBVERSION < 68) -#define dSTCXT_SV \ - SV *perinterp_sv = perl_get_sv(MY_VERSION, 0) +#define dSTCXT_SV \ + SV *perinterp_sv = NULL; \ + if (!PL_dirty) \ + perinterp_sv = perl_get_sv(MY_VERSION, 0) #else /* >= perl5.004_68 */ -#define dSTCXT_SV \ - SV *perinterp_sv = *hv_fetch(PL_modglobal, \ - MY_VERSION, sizeof(MY_VERSION)-1, TRUE) +#define dSTCXT_SV \ + SV *perinterp_sv = NULL; \ + if (!PL_dirty) \ + perinterp_sv = *hv_fetch(PL_modglobal, MY_VERSION, sizeof(MY_VERSION)-1, TRUE) #endif /* < perl5.004_68 */ #define dSTCXT_PTR(T,name) \ T name = ((perinterp_sv && SvIOK(perinterp_sv) && SvIVX(perinterp_sv) \ ? (T)SvPVX(SvRV(INT2PTR(SV*,SvIVX(perinterp_sv)))) : (T) 0)) -#define dSTCXT \ - dSTCXT_SV; \ +#define dSTCXT \ + dSTCXT_SV; \ dSTCXT_PTR(stcxt_t *, cxt) #define INIT_STCXT \ @@ -2259,7 +2273,16 @@ sortcmp(const void *a, const void *b) static int store_hash(pTHX_ stcxt_t *cxt, HV *hv) { dVAR; - I32 len = HvTOTALKEYS(hv); + I32 len = +#if PERL_VERSION < 8 + HvKEYS(hv); +#else +# ifdef HAS_RESTRICTED_HASHES + HvTOTALKEYS(hv); +# else + HvUSEDKEYS(hv); +# endif +#endif I32 i; int ret = 0; I32 riter; @@ -3731,7 +3754,11 @@ static int do_store( TRACEME(("do_store (optype=%d, netorder=%d)", optype, network_order)); - optype |= ST_STORE; + if (PL_dirty) { /* already freed cxt */ + Perl_die(aTHX_ "Can't store with Storable during global destruction"); /* croak too instable here */ + return 0; + } + optype |= ST_STORE; /* * Workaround for CROAK leak: if they enter with a "dirty" context, @@ -6047,6 +6074,11 @@ static SV *do_retrieve( TRACEME(("do_retrieve (optype = 0x%x)", optype)); + if (PL_dirty) { /* already freed cxt */ + Perl_die(aTHX_ "Can't retrieve with Storable during global destruction"); /* croak too instable here */ + return &PL_sv_undef; + } + optype |= ST_RETRIEVE; /* @@ -6305,6 +6337,10 @@ static SV *dclone(pTHX_ SV *sv) SV *out; TRACEME(("dclone")); + if (PL_dirty) { /* already freed cxt */ + Perl_die(aTHX_ "Can't clone with Storable during global destruction"); /* croak too instable here */ + return &PL_sv_undef; + } /* * Workaround for CROAK leak: if they enter with a "dirty" context, @@ -6507,9 +6543,12 @@ last_op_in_netorder() PREINIT: bool result; PPCODE: + if (PL_dirty) { + Perl_die(aTHX_ "Can't use Storable during global destruction"); + XSRETURN(0); + } if (ix) { dSTCXT; - result = cxt->entry && (cxt->optype & ix) ? TRUE : FALSE; } else { result = !!last_op_in_netorder(aTHX); diff --git a/dist/Storable/t/destroy.t b/dist/Storable/t/destroy.t new file mode 100644 index 0000000..c28ba25 --- /dev/null +++ b/dist/Storable/t/destroy.t @@ -0,0 +1,21 @@ +# [perl #118139] crash in global destruction when accessing an +# already freed PL_modglobal or accessing the freed cxt. +use Test; +use Storable; +BEGIN { + plan tests => 1; + store {}, "foo"; +} +package foo; +sub new { return bless {} } +DESTROY { + open $fh, "<", "foo"; + eval { Storable::pretrieve($fh); }; + unlink "foo"; +} + +package main; +# print "# $^X\n"; +$x = foo->new(); + +ok(1); -- 1.7.10.4
CC: perl5-porters [...] perl.org
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Thu, 23 May 2013 18:57:30 +0200
To: Leon Timmermans <fawaka [...] gmail.com>
From: Steffen Mueller <smueller [...] cpan.org>
Download (untitled) / with headers
text/plain 850b
On 05/23/2013 05:57 PM, Leon Timmermans wrote: Show quoted text
> On Thu, May 23, 2013 at 5:45 PM, Steffen Mueller <smueller@cpan.org> wrote:
>> There's a more general problem with this: occasionally, C extensions need to >> do cleanup very late during global destruction. Usually just a free() or >> Safefree() to be done after all Perl code is done. Maybe adding a C level >> hook for this could allow Storable to work around this problem as well? >> Given what you write, I think maybe not that easily.
> > That would not solve this problem. This problem is not in a DESTROY > from Storable, but from other code calling Storable from they DESTROY > methods.
Which would be fine if the Storable guts were held on to long enough to actually be called from DESTROYs. But since the "guts" here is perl guts, not storable guts, that likely doesn't work. --Steffen
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Thu, 23 May 2013 14:41:18 -0500
To: perlbug-followup [...] perl.org
From: Reini Urban <reini [...] cpanel.net>
Download (untitled) / with headers
text/plain 962b
On 05/23/2013 10:58 AM, Leon Timmermans via RT wrote: Show quoted text
> On Thu, May 23, 2013 at 5:45 PM, Steffen Mueller <smueller@cpan.org> wrote:
>> There's a more general problem with this: occasionally, C extensions need to >> do cleanup very late during global destruction. Usually just a free() or >> Safefree() to be done after all Perl code is done. Maybe adding a C level >> hook for this could allow Storable to work around this problem as well? >> Given what you write, I think maybe not that easily.
> > That would not solve this problem. This problem is not in a DESTROY > from Storable, but from other code calling Storable from they DESTROY > methods.
DESTROY in Storable in part of the problem. We have to die in Storable if called during destruction, because Storable has its own DESTROY block which frees ctx, and you are not allowed to access that then. -- Reini Working towards a true Modern Perl. Slim, functional, unbloated, compile-time optimizable
CC: perl5-porters [...] perl.org, bugs-bitbucket [...] rt.perl.org
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Tue, 28 May 2013 16:28:50 +0100
To: Leon Timmermans <fawaka [...] gmail.com>
From: Nicholas Clark <nick [...] ccl4.org>
Download (untitled) / with headers
text/plain 3.6k
On Thu, May 23, 2013 at 06:07:16PM +0200, Leon Timmermans wrote: Show quoted text
> 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
CC: perl5-porters [...] perl.org, bugs-bitbucket [...] rt.perl.org
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Tue, 28 May 2013 17:17:02 +0100
To: Leon Timmermans <fawaka [...] gmail.com>
From: Nicholas Clark <nick [...] ccl4.org>
Download (untitled) / with headers
text/plain 1.9k
On Tue, May 28, 2013 at 04:28:50PM +0100, Nicholas Clark wrote: Show quoted text
> 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
Show quoted text
> 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.
Right. As best I can tell the dance between ->msaved and ->membuf is a bit daft. The code looks like it can be refactored so that it instead has an "out" buffer that is dynamically allocated and owned by the context, and an "in" buffer that is just a pointer to the SV that is being read from. Secondly, and more usefully, keybuf and membuf aren't actually needed at the same time. keybuf is only used as scratch space to read in hash keys. The code is also sufficiently abstracted between memory and file descriptor reads that it "READ"s into keybuf from the input source, either way. However, for the case of the input source being memory, the hash key is already nicely lined up in memory. So by busting the abstraction a bit, and peeking direct, one could completely avoid the need to have both keybuf and membuf. Which means only one dynamically allocated storage pool per context. Which gives several options to avoid needing that DESTROY. I'm not sure how much sanity it will cost to refactor this. Nicholas Clark
CC: perl5-porters [...] perl.org, bugs-bitbucket [...] rt.perl.org
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Tue, 28 May 2013 18:23:26 +0200
To: Nicholas Clark <nick [...] ccl4.org>
From: Leon Timmermans <fawaka [...] gmail.com>
Download (untitled) / with headers
text/plain 272b
On Tue, May 28, 2013 at 6:17 PM, Nicholas Clark <nick@ccl4.org> wrote: Show quoted text
> I'm not sure how much sanity it will cost to refactor this.
One fix that doesn't require too much refactoring may be to move it from a DESTROY method to free magic. Those are run much later. Leon
CC: perl5-porters [...] perl.org, bugs-bitbucket [...] rt.perl.org
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Tue, 28 May 2013 17:28:06 +0100
To: Leon Timmermans <fawaka [...] gmail.com>
From: Nicholas Clark <nick [...] ccl4.org>
Download (untitled) / with headers
text/plain 627b
On Tue, May 28, 2013 at 06:23:26PM +0200, Leon Timmermans wrote: Show quoted text
> On Tue, May 28, 2013 at 6:17 PM, Nicholas Clark <nick@ccl4.org> wrote:
> > I'm not sure how much sanity it will cost to refactor this.
> > One fix that doesn't require too much refactoring may be to move it > from a DESTROY method to free magic. Those are run much later.
Yes, that feels like a good start. I wonder if doing that is enough to make Storable reliable during global destruction. The cause of the bug seems to be solely that the thing that its context points to is getting wiped during object destruction, rather than after it. Nicholas Clark
CC: perlbug-followup [...] perl.org
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Tue, 28 May 2013 20:40:14 +0200
To: Reini Urban <reini [...] cpanel.net>
From: Steffen Mueller <smueller [...] cpan.org>
Download (untitled) / with headers
text/plain 2.1k
On 05/23/2013 09:41 PM, Reini Urban wrote: Show quoted text
> On 05/23/2013 10:58 AM, Leon Timmermans via RT wrote:
>> On Thu, May 23, 2013 at 5:45 PM, Steffen Mueller <smueller@cpan.org> >> wrote:
>>> There's a more general problem with this: occasionally, C extensions >>> need to >>> do cleanup very late during global destruction. Usually just a free() or >>> Safefree() to be done after all Perl code is done. Maybe adding a C >>> level >>> hook for this could allow Storable to work around this problem as well? >>> Given what you write, I think maybe not that easily.
>> >> That would not solve this problem. This problem is not in a DESTROY >> from Storable, but from other code calling Storable from they DESTROY >> methods.
> > DESTROY in Storable in part of the problem. > We have to die in Storable if called during destruction, because > Storable has its own DESTROY block which frees ctx, and you are not > allowed to access that then.
*nods* Ideally, I think using a scheme like we have for Sereal might be the ideal solution, but I doubt any of us will volunteer to hack that into Storable: The "context" object (encoder or decoder for Sereal) are C structs wrapped as Perl objects. You can either explicitly construct one, or, if you use the (slower) functional interface, one gets created on the fly. These on-the-fly structs have a flag set that indicates wherever they'd normally be cleared, they're instead free'd. The volatile part of the encoder/decoder state is cleared by pushing a callback on the save stack. Until cleared, the state is marked dirty. To avoid concurrent use issues (or calling back into Sereal during exception handling before it's cleaned up), Sereal will detect when you're trying to use an encoder/decoder that's already dirty and then helpfully create a clone of the static parts (the settings) without the volatile state. These clones will always also have the "free instead of clear" flag set. Sereal doesn't have state that fundamentally needs clearing at interpreter exit. Presumably, Storable has that mostly to allow the functional interface to be faster? If so, as Vincent recently pointed out, call_atexit() could help? --Steffen
CC: perl5-porters [...] perl.org
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Tue, 28 May 2013 22:15:19 +0100
To: Leon Timmermans <fawaka [...] gmail.com>
From: Nicholas Clark <nick [...] ccl4.org>
On Tue, May 28, 2013 at 05:28:06PM +0100, Nicholas Clark wrote: Show quoted text
> On Tue, May 28, 2013 at 06:23:26PM +0200, Leon Timmermans wrote:
> > On Tue, May 28, 2013 at 6:17 PM, Nicholas Clark <nick@ccl4.org> wrote:
> > > I'm not sure how much sanity it will cost to refactor this.
> > > > One fix that doesn't require too much refactoring may be to move it > > from a DESTROY method to free magic. Those are run much later.
> > Yes, that feels like a good start. I wonder if doing that is enough to > make Storable reliable during global destruction. The cause of the bug seems > to be solely that the thing that its context points to is getting wiped > during object destruction, rather than after it.
I think it just might be. It's done on smoke-me/nicholas/Storable with Reini's test case added. Test case fails without the change, passes with it. A superficial test with valgrind suggests that nothing is leaking. I've only pushed this to see what breaks. There's still the ChangeLog to update and I'm not sure what else. Nicholas Clark
CC: perl5-porters [...] perl.org
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Wed, 29 May 2013 12:05:22 +0100
To: Reini Urban via RT <perlbug-followup [...] perl.org>
From: Nicholas Clark <nick [...] ccl4.org>
On Thu, May 23, 2013 at 09:42:29AM -0700, Reini Urban via RT wrote: Show quoted text
> Attached is a revised patch.
Show quoted text
> @@ -2259,7 +2273,16 @@ sortcmp(const void *a, const void *b) > static int store_hash(pTHX_ stcxt_t *cxt, HV *hv) > { > dVAR; > - I32 len = HvTOTALKEYS(hv); > + I32 len = > +#if PERL_VERSION < 8 > + HvKEYS(hv); > +#else > +# ifdef HAS_RESTRICTED_HASHES > + HvTOTALKEYS(hv); > +# else > + HvUSEDKEYS(hv); > +# endif > +#endif > I32 i; > int ret = 0; > I32 riter;
I don't think that change does anything. For starters, HAS_RESTRICTED_HASHES will always be true if PERL_VERSION >= 8, because it's defined earlier by: #ifdef HvPLACEHOLDERS #define HAS_RESTRICTED_HASHES #else and HvPLACEHOLDERS was added by commit 8aacddc1ea3837f8 back in 5.7.2 So that simplifies to - I32 len = HvTOTALKEYS(hv); + I32 len = +#if PERL_VERSION < 8 + HvKEYS(hv); +#else + HvTOTALKEYS(hv); +#endif In turn, Storable.xs already does this for compatibility with 5.6.x and earlier: #ifndef HvTOTALKEYS # define HvTOTALKEYS(hv) HvKEYS(hv) #endif and as HvTOTALKEYS was also first introduced by commit 8aacddc1ea3837f8, for 5.6 and earlier it's not defied, so Storable.xs makes that fallback. Hence for those versions, HvTOTALKEYS(hv) is equivalent to HvKEYS(). Which means that the original 1 line is identical to the changed version. - I32 len = HvTOTALKEYS(hv); + I32 len = +#if PERL_VERSION < 8 + HvKEYS(hv); +#else + HvTOTALKEYS(hv); +#endif Here's the difference in pre-processor output for 5.6.2, 5.8.0 and 5.8.9 with and without that patch hunk: --- Storable.i.562 2013-05-29 12:50:56.000000000 +0200 +++ Storable.i.562.patched 2013-05-29 12:50:34.000000000 +0200 @@ -7945,7 +7945,16 @@ static int store_hash( stcxt_t *cxt, HV *hv) { extern int Perl___notused; - I32 len = ((XPVHV*) (hv)->sv_any)->xhv_keys; + I32 len = + + ((XPVHV*) (hv)->sv_any)->xhv_keys; + + + + + + + I32 i; int ret = 0; I32 riter; @@ -7985,7 +7994,7 @@ --- Storable.i.580 2013-05-29 12:51:14.000000000 +0200 +++ Storable.i.580.patched 2013-05-29 12:50:08.000000000 +0200 @@ -8185,7 +8185,16 @@ static int store_hash( stcxt_t *cxt, HV *hv) { extern int Perl___notused __attribute__((unused)); - I32 len = (((XPVHV*) (hv)->sv_any)->xhv_keys); + I32 len = + + + + + (((XPVHV*) (hv)->sv_any)->xhv_keys); + + + + I32 i; int ret = 0; I32 riter; --- Storable.i.589 2013-05-29 12:51:36.000000000 +0200 +++ Storable.i.589.patched 2013-05-29 12:49:43.000000000 +0200 @@ -9126,7 +9126,16 @@ static int store_hash( stcxt_t *cxt, HV *hv) { extern int Perl___notused __attribute__((unused)); - I32 len = (((XPVHV*) (hv)->sv_any)->xhv_keys); + I32 len = + + + + + (((XPVHV*) (hv)->sv_any)->xhv_keys); + + + + I32 i; int ret = 0; I32 riter; Only whitespace differences. I could build blead's current Storable.xs on 5.6.1 and 5.6.2 without any changes. The XS is actually fine all the way back to 5.004 So I'm not sure why any additional 5.6.2 changes are needed. Nicholas Clark
Subject: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Wed, 29 May 2013 19:22:17 +0200
To: perl5-porters [...] perl.org
From: Salvador Fandino <sfandino [...] yahoo.com>
Download (untitled) / with headers
text/plain 3.8k
On 05/23/2013 05:30 PM, rurban @ cpanel . net wrote: Show quoted text
> # New Ticket Created by rurban@cpanel.net > # Please include the string: [perl #118139] > # in the subject line of all future correspondence about this issue. > # <URL: https://rt.perl.org:443/rt3/Ticket/Display.html?id=118139 > > > > > This is a bug report for perl from rurban@cpanel.net, > generated with the help of perlbug 1.39 running under perl 5.18.0. > > > ----------------------------------------------------------------- > 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.
I have been working for some time in refactoring Storable. See https://github.com/salva/perl5/tree/improve_storable And a wrapped version for CPAN distribution: https://github.com/salva/p5-Storable It is still a work in progress and still has some bugs (more notably it doesn't work on 32 bit platforms where perl has been compiled with use64bitint set). Initially my plan was to send the patch to p5p once those bugs have been solved but as this bug report is here and people is already trying to solve it, I think is a good time to tell the world about my work on Storable :-) You can see the commits for a detailed history of the changes. But briefly they are as follows: - Move the context object into the C stack, so no more DESTROYing it! - Break the context into a two structures store_cxt and retrieve_cxt. - Use a perl SV as the output buffer when storing to memory (the old structure has a O(N^2) performance. - Convert all the IO macros into static C functions and let the compiler optimize them when required (last time I looked the .so was 30% smaller) - use Perl_croak to fail instead of returning 1 (for store) or NULL (for retrieve) and propagate it all the way up. - mortalize everything when required so that croak'ing doesn't leak memory. - several memory leaks fixed. - ptr_tables back-ported to old perls so that conditional code can be removed. - lots of premature optimizations removed - lots of code de-duplication I think the most important change is moving the context object into the C stack because it was and absolute nightmare: It seems that initially, probably to simplify the code, Storable used some global variables to keep the context while recursing (note that the context is reset in every call so no data is shared between successive calls to store or retrieve functions). Then, perl got support for threads and the context was moved to thread storage and the funny thing is that, probably for performance reasons, also passed from call to call when recursing! Then, Storable got the hooks STORABLE_freeze and STORABLE_thaw, so in order to make it reentrant, the context was moved into some kind of perl-thread stack. Then lots of work-arounds were added to solve common errors and memory leaks. I believe it is the worst case of code rot I have ever seen!!! With my changes, the context is created when store or freeze functions are called on the C stack and passed from function to function when recurring. The temporal structures are mortalized so that they are freed when the code comes back to Perl-land. I might have introduced new bugs but IMO, now, the code is pretty much simpler and bugs should also be much easier to fix than before. If you agree that having that into blead is a good idea, I think that the way to follow is to release it to CPAN as a development version first so that it can be tested (I have been testing it myself with all the latest perls on the 5.6 5.8, 5.10, 5.12, 5.14, 5.16, 5.18 and 5.19 series with and without threads)
CC: perl5-porters [...] perl.org
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Wed, 29 May 2013 19:39:50 +0200
To: Salvador Fandino <sfandino [...] yahoo.com>
From: Leon Timmermans <fawaka [...] gmail.com>
Download (untitled) / with headers
text/plain 1.4k
On Wed, May 29, 2013 at 7:22 PM, Salvador Fandino <sfandino@yahoo.com> wrote: Show quoted text
> I have been working for some time in refactoring Storable. See > > https://github.com/salva/perl5/tree/improve_storable > > And a wrapped version for CPAN distribution: > > https://github.com/salva/p5-Storable > > It is still a work in progress and still has some bugs (more notably it > doesn't work on 32 bit platforms where perl has been compiled with > use64bitint set). > > Initially my plan was to send the patch to p5p once those bugs have been > solved but as this bug report is here and people is already trying to > solve it, I think is a good time to tell the world about my work on > Storable :-) > > With my changes, the context is created when store or freeze functions > are called on the C stack and passed from function to function when > recurring. The temporal structures are mortalized so that they are freed > when the code comes back to Perl-land. > > I might have introduced new bugs but IMO, now, the code is pretty much > simpler and bugs should also be much easier to fix than before.
Sounds good. Show quoted text
> If you agree that having that into blead is a good idea, I think that > the way to follow is to release it to CPAN as a development version > first so that it can be tested (I have been testing it myself with all > the latest perls on the 5.6 5.8, 5.10, 5.12, 5.14, 5.16, 5.18 and 5.19 > series with and without threads)
A CPAN smoke may be helpful :-) Leon
CC: perl5-porters [...] perl.org
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Tue, 4 Jun 2013 12:29:03 +0100
To: Salvador Fandino <sfandino [...] yahoo.com>, Leon Timmermans <fawaka [...] gmail.com>
From: Nicholas Clark <nick [...] ccl4.org>
Download (untitled) / with headers
text/plain 13.9k
On Wed, May 29, 2013 at 07:22:17PM +0200, Salvador Fandino wrote: Show quoted text
> Initially my plan was to send the patch to p5p once those bugs have been > solved but as this bug report is here and people is already trying to > solve it, I think is a good time to tell the world about my work on > Storable :-)
Yes. Thanks for tracking perl5-porters to make sure that duplication of work didn't happen (at least, not for more than a day) I've skimmed your changes and wow, you have been busy. Show quoted text
> - Break the context into a two structures store_cxt and retrieve_cxt.
Good. I had wondered how possible this was. Show quoted text
> - Use a perl SV as the output buffer when storing to memory (the old > structure has a O(N^2) performance.
I'd seen the existing implementation, and thought that using an SV had the potential to simplify a chunk of the resource management code. I hadn't realised that the old structure was O(bad) Show quoted text
> I think the most important change is moving the context object into the > C stack because it was and absolute nightmare: > > It seems that initially, probably to simplify the code, Storable used > some global variables to keep the context while recursing (note that the > context is reset in every call so no data is shared between successive > calls to store or retrieve functions).
I had guessed this based on evidence such as this: #define kbuf (cxt->keybuf).arena #define ksiz (cxt->keybuf).asiz I was tempted to eliminate such #defines by doing search and replace on their use points, as (within code that one controls completely) the short term gains of just using a macro to avoid touching code later on don't seem to outweigh the obfuscation costs of doing so. But as you've been refactoring, this would just conflict. Show quoted text
> Then, perl got support for threads and the context was moved to thread > storage and the funny thing is that, probably for performance reasons, > also passed from call to call when recursing!
This seems to have been the case. The context structure was first added in this release: Tue Sep 14 22:13:28 MEST 1999 Raphael Manfredi <Raphael_Manfredi@pobox.com> Integrated "thread-safe" patch from Murray Nesbitt. Note that this may not be very efficient for threaded code, see comment in the code. Try to avoid compilation warning on 64-bit CPUs. Can't test it, since I don't have access to such machines. but with a per-function dPERINTERP That got replaced with a cxt parameter in this revision: Sun Jul 30 12:59:17 MEST 2000 Raphael Manfredi <Raphael_Manfredi@pobox.com> First revision of Storable 0.7. The serializing format is new, known as version 2.0. It is fully backward compatible with 0.6. Earlier formats are deprecated and have not even been tested: next version will drop pre-0.6 format. Changes since 0.6@11: - Moved interface to the "beta" status. Some tiny parts are still subject to change, but nothing important enough to warrant an "alpha" status any longer. - Slightly reduced the size of the Storable image by factorizing object class names and removing final object storage notification due to a redesign of the blessed object storing. - Classes can now redefine how they wish their instances to be serialized and/or deep cloned. Serializing hooks are written in Perl code. - The engine is now fully re-entrant. Show quoted text
> Then, Storable got the hooks STORABLE_freeze and STORABLE_thaw, so in > order to make it reentrant, the context was moved into some kind of > perl-thread stack.
The hooks were also added in that revision. Show quoted text
> I believe it is the worst case of code rot I have ever seen!!!
Please don't temp people to find you stronger examples. But thanks for your analysis of what went wrong. Show quoted text
> With my changes, the context is created when store or freeze functions > are called on the C stack and passed from function to function when > recurring. The temporal structures are mortalized so that they are freed > when the code comes back to Perl-land. > > I might have introduced new bugs but IMO, now, the code is pretty much > simpler and bugs should also be much easier to fix than before.
Yes, this seems like a much more sane solution. Show quoted text
> If you agree that having that into blead is a good idea, I think that > the way to follow is to release it to CPAN as a development version > first so that it can be tested (I have been testing it myself with all > the latest perls on the 5.6 5.8, 5.10, 5.12, 5.14, 5.16, 5.18 and 5.19 > series with and without threads)
I think it would be good, once it's been thoroughly tested. You've said that you've got an outstanding known bug of the 64 bit IV case for 32 bit platforms. I'd agree with Leon here: On Wed, May 29, 2013 at 07:39:50PM +0200, Leon Timmermans wrote: Show quoted text
> A CPAN smoke may be helpful :-)
I think that the way to go is probably 1) fix what you know to be broken 2) we put it in blead as a smoke-me branch and see what the various platform testers make of it. Fix those bugs 3) Put out a dev release, see what the CPAN testers make of it on various platforms and various versions. Fix any bugs 4) Once we think that there are no bugs (or we've reclassified all "bugs" as "known changes" and think that they aren't worrying), run a CPAN smoke to try to find any surprises Then release it for real. In that I'm vary wary of a large-scale refactoring of something rather low in the dependency chain, with the potential for data loss. I'd be a lot more confident if we've smoked CPAN, and nothing surprising happens. But that's relatively expensive, so do it as a final validation step after getting the cheaper tests clean first. That will take some time. Given that the changes made in the branch smoke-me/nicholas/rt-118139 fix the immediate bug, and make the code no worse than it was before, I think the right thing to do is to merge this to blead, and make a CPAN release. The changes to the XS are small, and I think will be an easy conflict to resolve. However, the changes in the branch smoke-me/nicholas/Storable start to be more significant, and conflict with what you've been doing (without solving the real problem, as you have), so I think the best thing to do is to abandon them, instead of merging them. Except, that is, for the last change, which I think you've got (attached, but I think it will conflict as-is). This code isn't just redundant, it's technically undefined behaviour: #if (PATCHLEVEL <= 6) #if defined(USE_ITHREADS) #define STORE_HASH_SORT \ ENTER; { \ PerlInterpreter *orig_perl = PERL_GET_CONTEXT; \ SAVESPTR(orig_perl); \ PERL_SET_CONTEXT(aTHX); \ qsort((char *) AvARRAY(av), len, sizeof(SV *), sortcmp); \ } LEAVE; #else /* ! USE_ITHREADS */ It was added in a refactoring which was discussed on this list in 2004. A couple of messages whose neighbours in the thread give some context: http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2004-02/msg00758.html http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2004-03/msg00136.html Whilst I participated in that thread back then, I wasn't aware of the mechanism of ithreads, and hence why the advice given and code it resulted in was wrong. Aha, experience (and hindsight). From the commit message on the attached patch: For 5.6.x and earlier, the interpreter context saving/restoring is not needed prior to calling qsort(), as within the thread there is only one interpreter in play. Moreover, the SAVESPTR(orig_perl); in the removed code is not just pointless, it's technically undefined behaviour. All that the SAVESPTR() macro will do is ensure that the value of *(&orig_perl) is restored at scope exit. It won't actually restore the interpreter context - that would need a call to PERL_SET_CONTEXT with the value of origperl. The undefined behaviour is because the LEAVE; which causes the scope pop action happens outside the block, hence orig_perl has gone out of scope, and the address &orig_perl (saved on the scope stack, written to by LEAVE), now points to invalid memory. It is, however, quite hard to even get gcc's ASAN to trap this. This code is likely unreachable anyway. It's only compiled in for 5.6.x and earlier with USE_ITHREADS. ithreads was not useful until 5.8.0. However, it takes quite a bit of effort to prove anything into viewing the existing code as undefined behaviour. I guess that the nested block that the macro creates is folded into the surrounding block, hence the automatic variables within the nested block aren't seen as going out of scope until later. I had to make the code into a static function (and hack the pre-processor to force compilation on post-5.6) like this: @@ -1032,16 +1032,22 @@ static const char byteorderstr_56[] = {BYTEORDER_BYTES_56, 0}; * sortsv is not available ( <= 5.6.1 ). */ -#if (PATCHLEVEL <= 6) +static int sortcmp(const void *a, const void *b); #if defined(USE_ITHREADS) +static sorted(pTHX_ AV *av, STRLEN len) +{ + PerlInterpreter *orig_perl = PERL_GET_CONTEXT; + SAVESPTR(orig_perl); + PERL_SET_CONTEXT(aTHX); + qsort((char *) AvARRAY(av), len, sizeof(SV *), sortcmp); +} + + #define STORE_HASH_SORT \ ENTER; { \ - PerlInterpreter *orig_perl = PERL_GET_CONTEXT; \ - SAVESPTR(orig_perl); \ - PERL_SET_CONTEXT(aTHX); \ - qsort((char *) AvARRAY(av), len, sizeof(SV *), sortcmp); \ + sorted(aTHX_ av, len); \ } LEAVE; #else /* ! USE_ITHREADS */ @@ -1051,13 +1057,6 @@ static const char byteorderstr_56[] = {BYTEORDER_BYTES_56, 0}; #endif /* USE_ITHREADS */ -#else /* PATCHLEVEL > 6 */ - -#define STORE_HASH_SORT \ - sortsv(AvARRAY(av), len, Perl_sv_cmp); - -#endif /* PATCHLEVEL <= 6 */ - static int store(pTHX_ stcxt_t *cxt, SV *sv); static SV *retrieve(pTHX_ stcxt_t *cxt, const char *cname); [plus a couple more to ensure that static sortcmp was compiled] with which I can finally provoke my test program into exploding: ./perl -Ilib -MStorable -e '++$Storable::canonical; Storable::freeze({1..4})' ================================================================= ==3557== ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffc15ef370 at pc 0x7578d7 bp 0x7fffc15eede0 sp 0x7fffc15eedd8 WRITE of size 8 at 0x7fffc15ef370 thread T0 #0 0x7578d6 (/home/nick/Perl/perl/perl+0x7578d6) #1 0x750fbe (/home/nick/Perl/perl/perl+0x750fbe) #2 0x7fdc07ac2e61 (/home/nick/Perl/perl/lib/auto/Storable/Storable.so+0x16e61) #3 0x7fdc07adbdd4 (/home/nick/Perl/perl/lib/auto/Storable/Storable.so+0x2fdd4) #4 0x7fdc07adc88c (/home/nick/Perl/perl/lib/auto/Storable/Storable.so+0x3088c) #5 0x7fdc07afcdc7 (/home/nick/Perl/perl/lib/auto/Storable/Storable.so+0x50dc7) #6 0x68cbfe (/home/nick/Perl/perl/perl+0x68cbfe) #7 0x668793 (/home/nick/Perl/perl/perl+0x668793) #8 0x47d810 (/home/nick/Perl/perl/perl+0x47d810) #9 0x47c535 (/home/nick/Perl/perl/perl+0x47c535) #10 0x41a9ad (/home/nick/Perl/perl/perl+0x41a9ad) #11 0x7fdc0844bea4 (/lib/x86_64-linux-gnu/libc-2.17.so+0x21ea4) #12 0x41a748 (/home/nick/Perl/perl/perl+0x41a748) Address 0x7fffc15ef370 is located at offset 208 in frame <Perl_leave_scope> of T0's stack: This frame has 3 object(s): [32, 40) 'arg0' [96, 104) 'arg1' [160, 168) 'arg2' HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext (longjmp and C++ exceptions *are* supported) Shadow bytes around the buggy address: 0x1000782b5e10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1000782b5e20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1000782b5e30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1000782b5e40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1000782b5e50: 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4 f2 f2 f2 f2 =>0x1000782b5e60: 00 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 f3 f3[f3]f3 0x1000782b5e70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1000782b5e80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1000782b5e90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1000782b5ea0: 00 00 f1 f1 f1 f1 01 f4 f4 f4 f2 f2 f2 f2 04 f4 0x1000782b5eb0: f4 f4 f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2 04 f4 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap righ redzone: fb Freed Heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 ASan internal: fe ==3557== ABORTING (sadly this isn't in the glorious Technicolor that Address Sanitizer delivers for its terminal output) gdb shows that the address of orig_perl is the problem address which ASAN is complaining about, and the backtrace from the fault point shows that it's at the scope exit. Breakpoint 1, sorted (my_perl=0x605c0000ee80, av=0x60620001b6e8, len=2) at Storable.xs:1041 1041 PerlInterpreter *orig_perl = PERL_GET_CONTEXT; (gdb) p &orig_perl $1 = (PerlInterpreter **) 0x7fffffffdf50 (gdb) c Continuing. Breakpoint 2, __asan_report_error (pc=7698647, bp=140737488345536, sp=140737488345528, addr=140737488346960, is_write=true, access_size=8) at /home/nick/src/gcc-4.8.0/libsanitizer/asan/asan_report.cc:628 628 /home/nick/src/gcc-4.8.0/libsanitizer/asan/asan_report.cc: No such file or directory. (gdb) up #1 0x00007ffff4e5d967 in __asan::__asan_report_store8 (addr=<optimized out>) at /home/nick/src/gcc-4.8.0/libsanitizer/asan/asan_rtl.cc:234 234 /home/nick/src/gcc-4.8.0/libsanitizer/asan/asan_rtl.cc: No such file or directory. (gdb) up #2 0x00000000007578d7 in Perl_leave_scope (my_perl=0x605c0000ee80, base=22) at scope.c:942 942 *(SV**)(ARG0_PTR)= ARG1_SV; (gdb) p arg0.any_ptr $2 = (void *) 0x7fffffffdf50 (gdb) up #3 0x0000000000750fbf in Perl_pop_scope (my_perl=0x605c0000ee80) at scope.c:110 110 LEAVE_SCOPE(oldsave); The code is redundant and buggy. It should go. Nicholas Clark

Message body is not shown because sender requested not to inline it.

CC: perl5-porters [...] perl.org
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Fri, 7 Jun 2013 12:19:32 -0400
To: Salvador Fandino <sfandino [...] yahoo.com>
From: Ricardo Signes <perl.p5p [...] rjbs.manxome.org>
Download (untitled) / with headers
text/plain 344b
* Salvador Fandino <sfandino@yahoo.com> [2013-05-29T13:22:17] Show quoted text
> I have been working for some time in refactoring Storable. See > > https://github.com/salva/perl5/tree/improve_storable
Wow, no kidding! It looks like you've gotten some feedback on this, please let me know if you need anything specific to help things move foward. -- rjbs
Download signature.asc
application/pgp-signature 490b

Message body not shown because it is not plain text.

CC: "perl5-porters [...] perl.org" <perl5-porters [...] perl.org>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Sun, 9 Jun 2013 11:13:02 -0700 (PDT)
To: Ricardo Signes <perl.p5p [...] rjbs.manxome.org>
From: Salvador Fandino <sfandino [...] yahoo.com>
Download (untitled) / with headers
text/plain 1.5k
Show quoted text
----- Original Message -----
> From: Ricardo Signes <perl.p5p@rjbs.manxome.org> > To: Salvador Fandino <sfandino@yahoo.com> > Cc: perl5-porters@perl.org > Sent: Friday, June 7, 2013 6:19 PM > Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY
blocks
> > * Salvador Fandino <sfandino@yahoo.com> [2013-05-29T13:22:17]
>> I have been working for some time in refactoring Storable. See >> >>   https://github.com/salva/perl5/tree/improve_storable
> > Wow, no kidding!  It looks like you've gotten some feedback on this, please > let > me know if you need anything specific to help things move foward.
My biggest problem at the moment is getting old perl versions to compile and work. Specifically, 5.6.2 completely fails to compile into something usable on current Linux distributions. I have been installing virtual machines with old Linux releases, in order to have an environment where it compiles cleanly. So far, I have went back up to Ubuntu 10.04 but some tests from the perl test suite are still failing there. Then, another issue is that the current versions of several packages used by Storable (i.e. B::Deparse) doesn't support 5.6 anymore... well, the thing is that it is being a very time consuming task, though, eventually I will be able to do do it. If anybody has a working perl 5.6.x and want to compile and test the version of the module I have at GitHub (https://github.com/salva/p5-Storable), it would be very helpfull. BTW, how about 5.005? does still make sense to support such old perl version?
CC: Ricardo Signes <perl.p5p [...] rjbs.manxome.org>, "perl5-porters [...] perl.org" <perl5-porters [...] perl.org>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Mon, 10 Jun 2013 07:30:52 +0200
To: Salvador Fandino <sfandino [...] yahoo.com>
From: Steffen Mueller <smueller [...] cpan.org>
Download (untitled) / with headers
text/plain 522b
On 06/09/2013 08:13 PM, Salvador Fandino wrote: Show quoted text
> BTW, how about 5.005? does still make sense to support such old perl version?
No. 5.005 has jumped the shark. Frankly, I consider 5.6.X to be so old that supporting it is purely a courtesy to those contributors known to rely on such an outdated release (for whatever questionable reason). As far as I am concerned, even 5.8.X support becomes optional about a year after RedHat/CentOS drop support for versions of their distributions that ship with 5.8. --Steffen
CC: "perl5-porters [...] perl.org" <perl5-porters [...] perl.org>
Subject: How does one smoke dist/* (was: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks)
Date: Mon, 10 Jun 2013 16:30:02 +1000
To: Salvador Fandino <sfandino [...] yahoo.com>
From: Peter Rabbitson <rabbit-p5p [...] rabbit.us>
Download (untitled) / with headers
text/plain 2.4k
On Sun, Jun 09, 2013 at 11:13:02AM -0700, Salvador Fandino wrote: Show quoted text
> ----- Original Message ----- >
> > From: Ricardo Signes <perl.p5p@rjbs.manxome.org> > > To: Salvador Fandino <sfandino@yahoo.com> > > Cc: perl5-porters@perl.org > > Sent: Friday, June 7, 2013 6:19 PM > > Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY
> blocks
> > > > * Salvador Fandino <sfandino@yahoo.com> [2013-05-29T13:22:17]
> >> I have been working for some time in refactoring Storable. See > >> > >>   https://github.com/salva/perl5/tree/improve_storable
> > > > Wow, no kidding!  It looks like you've gotten some feedback on this, please > > let > > me know if you need anything specific to help things move foward.
> > My biggest problem at the moment is getting old perl versions to compile and work.
Weird... I had no notable problems configuring/compiling/bootstraping both 5.6.1 and 5.6.2. I hit another roadblock however - I am not sure how to properly build a dual-life dist from the perl tree. This is what I get on a more recent perl: Show quoted text
>>>>
rabbit@Ahasver:~/devel/p5-storable-imp$ git rev-parse HEAD 181247bf4fb683761428f68c808b06ef73d1fb14 rabbit@Ahasver:~/devel/p5-storable-imp$ cp -a dist/Storable/ ../storable-standalone rabbit@Ahasver:~/devel/p5-storable-imp$ cd ../storable-standalone/ rabbit@Ahasver:~/devel/storable-standalone$ perlbrew use 5.8.9 rabbit@Ahasver:~/devel/storable-standalone$ perl -e 'die $]' 5.008009 at -e line 1. rabbit@Ahasver:~/devel/storable-standalone$ perl Makefile.PL Writing Makefile for Storable Writing MYMETA.yml and MYMETA.json rabbit@Ahasver:~/devel/storable-standalone$ make cp Storable.pm blib/lib/Storable.pm /home/rabbit/perl5/perlbrew/perls/5.8.9/bin/perl /home/rabbit/perl5/perlbrew/perls/5.8.9/lib/5.8.9/ExtUtils/xsubpp -typemap /home/rabbit/perl5/perlbrew/perls/5.8.9/lib/5.8.9/ExtUtils/typemap Storable.xs > Storable.xsc && mv Storable.xsc Storable.c cc -c -D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -O2 -DVERSION=\"2.42\" -DXS_VERSION=\"2.42\" -fPIC "-I/home/rabbit/perl5/perlbrew/perls/5.8.9/lib/5.8.9/x86_64-linux-thread-multi/CORE" Storable.c Storable.xs:26:55: fatal error: ppport.h: No such file or directory compilation terminated. make: *** [Storable.o] Error 1 Show quoted text
>>>>
Please advise - I will test on all perls I have once I get the ball rolling (I have a rather large collection, both 64 and 32 - TUX's collection is even larger). Cheers
CC: Ricardo Signes <perl.p5p [...] rjbs.manxome.org>, "perl5-porters [...] perl.org" <perl5-porters [...] perl.org>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Mon, 10 Jun 2013 01:26:07 -0700 (PDT)
To: Steffen Mueller <smueller [...] cpan.org>
From: Salvador Fandino <sfandino [...] yahoo.com>
Show quoted text
----- Original Message -----
> From: Steffen Mueller <smueller@cpan.org> > To: Salvador Fandino <sfandino@yahoo.com> > Cc: Ricardo Signes <perl.p5p@rjbs.manxome.org>; "perl5-porters@perl.org" <perl5-porters@perl.org> > Sent: Monday, June 10, 2013 7:30 AM > Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY
blocks
> > On 06/09/2013 08:13 PM, Salvador Fandino wrote:
>> BTW, how about 5.005? does still make sense to support such old perl
> version? > > No. 5.005 has jumped the shark. > > Frankly, I consider 5.6.X to be so old that supporting it is purely a > courtesy to those contributors known to rely on such an outdated release > (for whatever questionable reason). > > As far as I am concerned, even 5.8.X support becomes optional about a > year after RedHat/CentOS drop support for versions of their > distributions that ship with 5.8.
All the world's not Linux! AFAIK, perl 5.8.4 is distributed as the system perl in Solaris 10 and Solaris 11 is still supporting that version too. I guess there are some other Unix vendors still supporting such old Perl versions.
CC: "perl5-porters [...] perl.org" <perl5-porters [...] perl.org>
Subject: Re: How does one smoke dist/* (was: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks)
Date: Mon, 10 Jun 2013 01:43:38 -0700 (PDT)
To: Peter Rabbitson <rabbit-p5p [...] rabbit.us>
From: Salvador Fandino <sfandino [...] yahoo.com>
Download (untitled) / with headers
text/plain 1.8k
Show quoted text
----- Original Message -----
> From: Peter Rabbitson <rabbit-p5p@rabbit.us> > To: Salvador Fandino <sfandino@yahoo.com> > Cc: "perl5-porters@perl.org" <perl5-porters@perl.org> > Sent: Monday, June 10, 2013 8:30 AM > Subject: How does one smoke dist/* (was: Storable refactoring, was Re:
[perl #118139] Storable in DESTROY blocks)
> > On Sun, Jun 09, 2013 at 11:13:02AM -0700, Salvador Fandino wrote:
>> ----- Original Message ----- >>
>> > From: Ricardo Signes <perl.p5p@rjbs.manxome.org> >> > To: Salvador Fandino <sfandino@yahoo.com> >> > Cc: perl5-porters@perl.org >> > Sent: Friday, June 7, 2013 6:19 PM >> > Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in
> DESTROY
>>   blocks
>> > >> > * Salvador Fandino <sfandino@yahoo.com> [2013-05-29T13:22:17]
>> >>  I have been working for some time in refactoring Storable. See >> >> >> >>   https://github.com/salva/perl5/tree/improve_storable
>> > >> > Wow, no kidding!  It looks like you've gotten some feedback on
> this, please
>> > let >> > me know if you need anything specific to help things move foward.
>> >> My biggest problem at the moment is getting old perl versions to compile
> and work. > > Weird... I had no notable problems configuring/compiling/bootstraping > both 5.6.1 and 5.6.2.
Witch OS/distribution are you using?
> I hit another roadblock however - I am not sure how to properly build a > dual-life dist from the perl tree. This is what I get on a more recent > perl:
I got the latest Storable version available from CPAN and used it as the base replacing Storable.pm and Storable.xs with the ones from core. I suppose the standalone version should also be under version control somewhere, but didn't bother to investigate that. BTW, in order to compile my version of Storable with old perl versions you also need the ptr_table.h file that is available from https://github.com/salva/p5-Storable
CC: "perl5-porters [...] perl.org" <perl5-porters [...] perl.org>
Subject: Re: How does one smoke dist/* (was: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks)
Date: Mon, 10 Jun 2013 18:47:35 +1000
To: Salvador Fandino <sfandino [...] yahoo.com>
From: Peter Rabbitson <rabbit-p5p [...] rabbit.us>
Download (untitled) / with headers
text/plain 1.1k
On Mon, Jun 10, 2013 at 01:43:38AM -0700, Salvador Fandino wrote: Show quoted text
> > > > Weird... I had no notable problems configuring/compiling/bootstraping > > both 5.6.1 and 5.6.2.
> > Witch OS/distribution are you using?
Various debians (stable/oldstable/sid/whatever) Show quoted text
>
> > I hit another roadblock however - I am not sure how to properly build a > > dual-life dist from the perl tree. This is what I get on a more recent > > perl:
> > I got the latest Storable version available from CPAN and used it as the base replacing Storable.pm and Storable.xs with the ones from core. > > I suppose the standalone version should also be under version control somewhere, but didn't bother to investigate that. > > BTW, in order to compile my version of Storable with old perl versions you also need the ptr_table.h file that is available from https://github.com/salva/p5-Storable
Hmmm... Do you think you can put together a command log (shell history) demosntrating how you are testing your changes against *any* other perl? I want to make sure I am testing what you are doing, otherwise it will be a lot of wasted time going back and forth... Cheers
CC: "perl5-porters [...] perl.org" <perl5-porters [...] perl.org>
Subject: Re: How does one smoke dist/* (was: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks)
Date: Mon, 10 Jun 2013 02:01:54 -0700 (PDT)
To: Peter Rabbitson <rabbit-p5p [...] rabbit.us>
From: Salvador Fandino <sfandino [...] yahoo.com>
Download (untitled) / with headers
text/plain 4.9k
Show quoted text
----- Original Message -----
> From: Peter Rabbitson <rabbit-p5p@rabbit.us> > To: Salvador Fandino <sfandino@yahoo.com> > Cc: "perl5-porters@perl.org" <perl5-porters@perl.org> > Sent: Monday, June 10, 2013 10:47 AM > Subject: Re: How does one smoke dist/* (was: Storable refactoring, was Re:
[perl #118139] Storable in DESTROY blocks)
> > On Mon, Jun 10, 2013 at 01:43:38AM -0700, Salvador Fandino wrote:
>> > >> > Weird... I had no notable problems configuring/compiling/bootstraping >> > both 5.6.1 and 5.6.2.
>> >> Witch OS/distribution are you using?
> > Various debians (stable/oldstable/sid/whatever) >
>>
>> > I hit another roadblock however - I am not sure how to properly build
> a
>> > dual-life dist from the perl tree. This is what I get on a more recent
>
>> > perl:
>> >> I got the latest Storable version available from CPAN and used it as the
> base replacing Storable.pm and Storable.xs with the ones from core.
>> >> I suppose the standalone version should also be under version control
> somewhere, but didn't bother to investigate that.
>> >> BTW, in order to compile my version of Storable with old perl versions you
> also need the ptr_table.h file that is available from > https://github.com/salva/p5-Storable > > > Hmmm... Do you think you can put together a command log (shell history) > demosntrating how you are testing your changes against *any* other perl? > I want to make sure I am testing what you are doing, otherwise it will > be a lot of wasted time going back and forth...
Clone the repository:   $ git clone git://github.com/salva/p5-Storable.git and then compile and test it in the usual way (ignore the warnings about missing files).   $ cd p5-Storable   $ perl Makefile.PL   $ make test There is also a script ('configure_and_test.sh') that can be used for testing it with perlbrew:   $ perlbrew exec ./configure_and_test.sh A sample session follows: salva@topo:~/t$ git clone git://github.com/salva/p5-Storable.git Cloning into 'p5-Storable'... remote: Counting objects: 99, done. remote: Compressing objects: 100% (89/89), done. remote: Total 99 (delta 22), reused 84 (delta 7) Receiving objects: 100% (99/99), 220.86 KiB | 216 KiB/s, done. Resolving deltas: 100% (22/22), done. salva@topo:~/t$ cd p5-Storable/ salva@topo:~/t/p5-Storable$ perl Makefile.PL Checking if your kit is complete... Warning: the following files are missing in your kit:     Storable.bs     Storable.c     Storable.o     t/compat/Test/Builder.pm     t/compat/Test/More.pm     t/compat/Test/Simple.pm Please inform the author. Writing Makefile for Storable Writing MYMETA.yml and MYMETA.json salva@topo:~/t/p5-Storable$ make test cp Storable.pm blib/lib/Storable.pm /usr/bin/perl /usr/local/share/perl/5.14.2/ExtUtils/xsubpp  -typemap /usr/share/perl/5.14/ExtUtils/typemap  Storable.xs > Storable.xsc && mv Storable.xsc Storable.c cc -c   -D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fstack-protector -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -O2 -g   -DVERSION=\"2.42\" -DXS_VERSION=\"2.42\" -fPIC "-I/usr/lib/perl/5.14/CORE"   Storable.c Running Mkbootstrap for Storable () chmod 644 Storable.bs rm -f blib/arch/auto/Storable/Storable.so cc  -shared -L/usr/local/lib -fstack-protector Storable.o  -o blib/arch/auto/Storable/Storable.so     \              \       chmod 755 blib/arch/auto/Storable/Storable.so cp Storable.bs blib/arch/auto/Storable/Storable.bs chmod 644 blib/arch/auto/Storable/Storable.bs PERL_DL_NONLAZY=1 /usr/bin/perl "-MExtUtils::Command::MM" "-e" "test_harness(0, 'blib/lib', 'blib/arch')" t/*.t t/attach_errors.t ..... ok     t/attach_singleton.t .. ok     t/blessed.t ........... ok     t/canonical.t ......... ok   t/circular_hook.t ..... ok   t/code.t .............. ok     t/compat01.t .......... skipped: Test only works for 32 bit little-ending machines t/compat06.t .......... ok   t/croak.t ............. ok   t/dclone.t ............ ok     t/downgrade.t ......... ok       t/file_magic.t ........ ok     t/forgive.t ........... ok   t/freeze.t ............ ok     t/integer.t ........... ok       t/interwork56.t ....... skipped: Your IVs are no larger than your longs t/just_plain_nasty.t .. ok     t/lock.t .............. ok   t/malice.t ............ ok       t/overload.t .......... ok     t/recurse.t ........... ok     t/restrict.t .......... ok       t/retrieve.t .......... ok     t/robust.t ............ ok   t/sig_die.t ........... ok   t/store.t ............. ok     t/threads.t ........... ok   t/tied.t .............. ok     t/tied_hook.t ......... ok     t/tied_items.t ........ ok   t/utf8.t .............. ok   t/utf8hash.t .......... ok       t/weak.t .............. ok     All tests successful. Files=33, Tests=2582,  6 wallclock secs ( 1.12 usr  0.09 sys +  3.39 cusr  0.46 csys =  5.06 CPU) Result: PASS salva@topo:~/t/p5-Storable$
CC: "perl5-porters [...] perl.org" <perl5-porters [...] perl.org>
Subject: Re: How does one smoke dist/* (was: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks)
Date: Mon, 10 Jun 2013 19:14:15 +1000
To: Salvador Fandino <sfandino [...] yahoo.com>
From: Peter Rabbitson <rabbit-p5p [...] rabbit.us>
Download (untitled) / with headers
text/plain 266b
On Mon, Jun 10, 2013 at 02:01:54AM -0700, Salvador Fandino wrote: Show quoted text
> > Clone the repository: > >   $ git clone git://github.com/salva/p5-Storable.git >
I am an idiot. I went for https://github.com/salva/perl5/tree/improve_storable Ignore the noise, testing...
CC: "perl5-porters [...] perl.org" <perl5-porters [...] perl.org>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Mon, 10 Jun 2013 19:46:24 +1000
To: Salvador Fandino <sfandino [...] yahoo.com>
From: Peter Rabbitson <rabbit-p5p [...] rabbit.us>
Download (untitled) / with headers
text/plain 1.3k
On Sun, Jun 09, 2013 at 11:13:02AM -0700, Salvador Fandino wrote: Show quoted text
> ----- Original Message ----- >
> > From: Ricardo Signes <perl.p5p@rjbs.manxome.org> > > To: Salvador Fandino <sfandino@yahoo.com> > > Cc: perl5-porters@perl.org > > Sent: Friday, June 7, 2013 6:19 PM > > Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY
> blocks
> > > > * Salvador Fandino <sfandino@yahoo.com> [2013-05-29T13:22:17]
> >> I have been working for some time in refactoring Storable. See > >> > >>   https://github.com/salva/perl5/tree/improve_storable
> > > > Wow, no kidding!  It looks like you've gotten some feedback on this, please > > let > > me know if you need anything specific to help things move foward.
> > If anybody has a working perl 5.6.x and want to compile and test
Amusingly enough everything works on my 5.6.1 and 5.6.2, as they are compiled without ithreads. Any threded 5.8 before 5.8.9 fails however. Please find the logs produced by the following command: perlbrew exec bash -c 'res=$((perl -V && perl Makefile.PL && make realclean && rm -f MANIFEST && perl Makefile.PL && make && make test)2>&1) && echo "$res" > buildlog/perl_${PERLBREW_PERL}.PASS || echo "$res" > buildlog/perl_${PERLBREW_PERL}.FAIL' In this repo: https://github.com/ribasushi/p5-storable_improvement_debug Hope this helps Cheers
CC: Ricardo Signes <perl.p5p [...] rjbs.manxome.org>, "perl5-porters [...] perl.org" <perl5-porters [...] perl.org>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Mon, 10 Jun 2013 19:56:22 +0200
To: Salvador Fandino <sfandino [...] yahoo.com>
From: Steffen Mueller <smueller [...] cpan.org>
Download (untitled) / with headers
text/plain 1.2k
On 06/10/2013 10:26 AM, Salvador Fandino wrote: Show quoted text
> ----- Original Message -----
>> From: Steffen Mueller <smueller@cpan.org> >> To: Salvador Fandino <sfandino@yahoo.com> >> Cc: Ricardo Signes <perl.p5p@rjbs.manxome.org>; "perl5-porters@perl.org" <perl5-porters@perl.org> >> Sent: Monday, June 10, 2013 7:30 AM >> Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY
> blocks
Show quoted text
>> As far as I am concerned, even 5.8.X support becomes optional about a >> year after RedHat/CentOS drop support for versions of their >> distributions that ship with 5.8.
> > All the world's not Linux! > > AFAIK, perl 5.8.4 is distributed as the system perl in Solaris 10 and Solaris 11 is still supporting that version too. > > I guess there are some other Unix vendors still supporting such old Perl versions.
Yes. And they're paid to do so[1]. In the end, though, if you're doing the work, you get to choose to support more than you have to. I have absolutely no problem with that. :) --Steffen [1] Yes, so is RedHat, but at least they're giving back to Perl some. There are some RedHat people active on this list. I'm not aware of many commercial Unix vendors speaking up here, specifically not when it comes to feeding back patches. Just one exception comes to mind.
CC: "perl5-porters [...] perl.org" <perl5-porters [...] perl.org>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Wed, 12 Jun 2013 09:28:12 -0700 (PDT)
To: Ricardo Signes <perl.p5p [...] rjbs.manxome.org>
From: Salvador Fandino <sfandino [...] yahoo.com>
Download (untitled) / with headers
text/plain 1.4k
Hi, Show quoted text
----- Original Message -----
> From: Ricardo Signes <perl.p5p@rjbs.manxome.org> > To: Salvador Fandino <sfandino@yahoo.com> > Cc: perl5-porters@perl.org > Sent: Friday, June 7, 2013 6:19 PM > Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY
blocks
> > * Salvador Fandino <sfandino@yahoo.com> [2013-05-29T13:22:17]
>> I have been working for some time in refactoring Storable. See >> >>   https://github.com/salva/perl5/tree/improve_storable
> > Wow, no kidding!  It looks like you've gotten some feedback on this, please > let > me know if you need anything specific to help things move foward.
I have applied the patch from Nicholas Clark and also solved the problems found by Peter Rabbitson. The test suite passes on any perl from 5.6.0 upward (*). I have run it on hundreds of different perls (do you know about the new perlbrew feature "install-multiple"?) At this point I think the module is ready to be released to CPAN as a development version so that it gets stressed by the CPAN Testers or anybody that want to give it a try. So, could I have comaintainership of the module assigned so that it doesn't appear as a unauthorized release? Also, is the CPAN package stored in some version control system somewhere? *) t/threads.t coredumps on perl 5.10.0-thread but the problem is not related to Storable at all,  it still happens when all the references to Storable are removed from the test script.
CC: Salvador Fandino <sfandino [...] yahoo.com>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Wed, 12 Jun 2013 14:29:38 -0500
To: perlbug-followup [...] perl.org
From: Reini Urban <reini [...] cpanel.net>
Download (untitled) / with headers
text/plain 1.6k
On 06/12/2013 11:28 AM, Salvador Fandiño via RT wrote: Show quoted text
> ----- Original Message -----
>> From: Ricardo Signes <perl.p5p@rjbs.manxome.org> >> To: Salvador Fandino <sfandino@yahoo.com> >> Cc: perl5-porters@perl.org >> Sent: Friday, June 7, 2013 6:19 PM >> Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY
> blocks
>> >> * Salvador Fandino <sfandino@yahoo.com> [2013-05-29T13:22:17]
>>> I have been working for some time in refactoring Storable. See >>> >>> https://github.com/salva/perl5/tree/improve_storable
>> >> Wow, no kidding! It looks like you've gotten some feedback on this, please >> let >> me know if you need anything specific to help things move foward.
> > > I have applied the patch from Nicholas Clark and also solved the problems found by Peter Rabbitson. The test suite passes on any perl from 5.6.0 upward (*). I have run it on hundreds of different perls (do you know about the new perlbrew feature "install-multiple"?) > > At this point I think the module is ready to be released to CPAN as a development version so that it gets stressed by the CPAN Testers or anybody that want to give it a try. > > So, could I have comaintainership of the module assigned so that it doesn't appear as a unauthorized release? > > Also, is the CPAN package stored in some version control system somewhere?
I tracked the cpan history and latest blead and your patches in git@github.com:rurban/Storable.git Can you fix the ChangeLog entry? But I'm still working on some pointer-sign problems in your changes. And you cannot just remove the hints to fix gcc -O3 optimizer problems. -- Reini Working towards a true Modern Perl. Slim, functional, unbloated, compile-time optimizable
CC: perlbug-followup [...] perl.org
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Wed, 12 Jun 2013 21:15:14 +0100
To: Salvador Fandino <sfandino [...] yahoo.com>
From: Nicholas Clark <nick [...] ccl4.org>
Download (untitled) / with headers
text/plain 8.7k
On Wed, Jun 12, 2013 at 02:29:38PM -0500, Reini Urban wrote: Show quoted text
> On 06/12/2013 11:28 AM, Salvador Fandiño via RT wrote:
Show quoted text
> > At this point I think the module is ready to be released to CPAN as a development version so that it gets stressed by the CPAN Testers or anybody that want to give it a try. > > > > So, could I have comaintainership of the module assigned so that it doesn't appear as a unauthorized release?
I don't think that it's time for a CPAN dev release yet. I've taken your branch, rebased it onto blead, updated the MANIFEST in the commits that moved or added files, grabbed Reini's fixes (but not the Changelog yet), and pushed it as a branch smoke-me/salva/Storable This will tell us what the various systems set up to smoke blead make of it, but I've already got one SEGV locally. Show quoted text
> > Also, is the CPAN package stored in some version control system somewhere?
No, blead is upstream. Show quoted text
> I tracked the cpan history and latest blead and your patches > in git@github.com:rurban/Storable.git > > Can you fix the ChangeLog entry? > > But I'm still working on some pointer-sign problems in your changes. > And you cannot just remove the hints to fix gcc -O3 optimizer problems.
Yes, this is troubling me. In particular, the HP-UX hints file seems to represent a real current issue. I may be able to test that. I also discover that there is at least one bug still present. This the branch that I pushed: $ valgrind ./perl -T -Ilib cpan/Tie-RefHash/t/storable.t ==13753== Memcheck, a memory error detector ==13753== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al. ==13753== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info ==13753== Command: ./perl -T -Ilib cpan/Tie-RefHash/t/storable.t ==13753== 1..42 ==13753== Invalid read of size 1 ==13753== at 0x47B602: Perl_mg_find (in /home/nick/Perl/perl/perl) ==13753== by 0x648DB9E: store (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648EBBF: store_ref (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648E17E: store (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648E31B: store_tied (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648E17E: store (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648ED00: dclone (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648EE4A: XS_Storable_dclone (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x48F33E: Perl_pp_entersub (in /home/nick/Perl/perl/perl) ==13753== by 0x48DCE9: Perl_runops_standard (in /home/nick/Perl/perl/perl) ==13753== by 0x432EF3: perl_run (in /home/nick/Perl/perl/perl) ==13753== by 0x41D5D1: main (in /home/nick/Perl/perl/perl) ==13753== Address 0x68d9ce2 is 2 bytes after a block of size 16 alloc'd ==13753== at 0x4C25D8C: malloc (vg_replace_malloc.c:270) ==13753== by 0x476C03: Perl_safesysmalloc (in /home/nick/Perl/perl/perl) ==13753== by 0x49A943: Perl_sv_grow (in /home/nick/Perl/perl/perl) ==13753== by 0x49E164: Perl_sv_setpvn (in /home/nick/Perl/perl/perl) ==13753== by 0x648ECF2: dclone (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648EE4A: XS_Storable_dclone (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x48F33E: Perl_pp_entersub (in /home/nick/Perl/perl/perl) ==13753== by 0x48DCE9: Perl_runops_standard (in /home/nick/Perl/perl/perl) ==13753== by 0x432EF3: perl_run (in /home/nick/Perl/perl/perl) ==13753== by 0x41D5D1: main (in /home/nick/Perl/perl/perl) ==13753== ==13753== ==13753== Process terminating with default action of signal 11 (SIGSEGV) ==13753== General Protection Fault ==13753== at 0x47B602: Perl_mg_find (in /home/nick/Perl/perl/perl) ==13753== by 0x648DB9E: store (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648EBBF: store_ref (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648E17E: store (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648E31B: store_tied (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648E17E: store (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648ED00: dclone (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x648EE4A: XS_Storable_dclone (in /home/nick/Perl/perl/lib/auto/Storable/Storable.so) ==13753== by 0x48F33E: Perl_pp_entersub (in /home/nick/Perl/perl/perl) ==13753== by 0x48DCE9: Perl_runops_standard (in /home/nick/Perl/perl/perl) ==13753== by 0x432EF3: perl_run (in /home/nick/Perl/perl/perl) ==13753== by 0x41D5D1: main (in /home/nick/Perl/perl/perl) ==13753== ==13753== HEAP SUMMARY: ==13753== in use at exit: 1,833,483 bytes in 6,382 blocks ==13753== total heap usage: 15,761 allocs, 9,379 frees, 3,411,485 bytes allocated ==13753== ==13753== LEAK SUMMARY: ==13753== definitely lost: 452 bytes in 11 blocks ==13753== indirectly lost: 2,153 bytes in 30 blocks ==13753== possibly lost: 948,889 bytes in 1,474 blocks ==13753== still reachable: 881,989 bytes in 4,867 blocks ==13753== suppressed: 0 bytes in 0 blocks ==13753== Rerun with --leak-check=full to see details of leaked memory ==13753== ==13753== For counts of detected and suppressed errors, rerun with: -v ==13753== ERROR SUMMARY: 2 errors from 1 contexts (suppressed: 4 from 4) Segmentation fault Perl built with -UDEBUGGING -Doptimize=-Os -Dusethreads (this is probably key - other permutations I happened to try didn't have problems). Full -V is: Summary of my perl5 (revision 5 version 19 subversion 1) configuration: Derived from: 5fa04395b5f903b295875988c4aa513df6315ad7 Platform: osname=linux, osvers=2.6.32-5-amd64, archname=x86_64-linux-thread-multi uname='linux gcc20 2.6.32-5-amd64 #1 smp mon jan 16 16:22:28 utc 2012 x86_64 gnulinux ' config_args='-Dusedevel=y -Dcc=ccache gcc -Dld=gcc -Dcf_email=nick@ccl4.org -Dperladmin=nick@ccl4.org -Dinc_version_list= -Dinc_version_list_init=0 -UDEBUGGING -Doptimize=-Os -Dusethreads -Uuselongdouble -Uuse64bitall -Uusemymalloc -Duseperlio -Dprefix=~/Sandpit/snap-v5.19.0-640-g5fa0439 -Uusevendorprefix -Uvendorprefix=~/Sandpit/snap-v5.19.0-640-g5fa0439 -Uuserelocatableinc -Umad -Uuseshrplib -de' hint=recommended, useposix=true, d_sigaction=define useithreads=define, usemultiplicity=define useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef use64bitint=define, use64bitall=undef, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='ccache gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64', optimize='-Os', cppflags='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion='', gccversion='4.4.5', gccosandvers='' intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16 ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=8, prototype=define Linker and Libraries: ld='gcc', ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /lib/../lib /usr/lib/../lib /lib /usr/lib /lib64 /usr/lib64 libs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc libc=/lib/libc-2.11.3.so, so=so, useshrplib=false, libperl=libperl.a gnulibc_version='2.11.3' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E' cccdlflags='-fPIC', lddlflags='-shared -Os -L/usr/local/lib -fstack-protector' Characteristics of this binary (from libperl): Compile-time options: HAS_TIMES MULTIPLICITY PERLIO_LAYERS PERL_DONT_CREATE_GVSV PERL_HASH_FUNC_ONE_AT_A_TIME_HARD PERL_IMPLICIT_CONTEXT PERL_MALLOC_WRAP PERL_NEW_COPY_ON_WRITE PERL_PRESERVE_IVUV PERL_USE_DEVEL USE_64_BIT_INT USE_ITHREADS USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_PERLIO USE_PERL_ATOF USE_REENTRANT_API Locally applied patches: uncommitted-changes Built under linux Compiled at Jun 12 2013 22:00:42 @INC: lib /home/nick/Sandpit/snap-v5.19.0-640-g5fa0439/lib/perl5/site_perl/5.19.1/x86_64-linux-thread-multi /home/nick/Sandpit/snap-v5.19.0-640-g5fa0439/lib/perl5/site_perl/5.19.1 /home/nick/Sandpit/snap-v5.19.0-640-g5fa0439/lib/perl5/5.19.1/x86_64-linux-thread-multi /home/nick/Sandpit/snap-v5.19.0-640-g5fa0439/lib/perl5/5.19.1 . I don't have any more time to look at this tonight, hence the dump of the summary into this e-mail. Nicholas Clark
CC: "perlbug-followup [...] perl.org" <perlbug-followup [...] perl.org>, perl5 porters list <perl5-porters [...] perl.org>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROYblocks
Date: Thu, 13 Jun 2013 01:17:24 -0700 (PDT)
To: Reini Urban <reini [...] cpanel.net>
From: Salvador Fandino <sfandino [...] yahoo.com>
Download (untitled) / with headers
text/plain 924b
On 06/12/2013 09:29 PM, Reini Urban wrote: Show quoted text
> And you cannot just remove the hints to fix gcc -O3 optimizer problems.
The Linux hints file says: # gcc -O3 (and higher) can cause code produced from Storable.xs that # dumps core immediately in recurse.t and retrieve.t, in is_storing() # and last_op_in_netorder(), respectively.  In both cases the cxt is # full of junk (and according to valgrind the cxt was never stack'd, # malloc'd or free'd).  Observed in Debian 3.0 x86, with gccs 2.95.4 # 20011002 and 3.3, and in Redhat 7.1 with gcc 3.3.1. The failures # happen only for unthreaded builds, threaded builds work okay. "is_stored" and "last_op_in_netorder" have been completely rewritten, and the old hairy context handling code is also gone. I don't see a reason to keep the work around unless it is proven that compiling with -O3 generates bad code yet. On the other hand, it seems I overlooked the HP-UX hints.
CC: "perlbug-followup [...] perl.org" <perlbug-followup [...] perl.org>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Thu, 13 Jun 2013 01:52:06 -0700 (PDT)
To: Nicholas Clark <nick [...] ccl4.org>
From: Salvador Fandino <sfandino [...] yahoo.com>
Download (untitled) / with headers
text/plain 985b
Show quoted text
----- Original Message -----
> From: Nicholas Clark <nick@ccl4.org> > To: Salvador Fandino <sfandino@yahoo.com> > Cc: perlbug-followup@perl.org > Sent: Wednesday, June 12, 2013 10:15 PM > Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY
blocks
> > On Wed, Jun 12, 2013 at 02:29:38PM -0500, Reini Urban wrote:
>> I tracked the cpan history and latest blead and your patches >> in git@github.com:rurban/Storable.git >> >> Can you fix the ChangeLog entry? >> >> But I'm still working on some pointer-sign problems in your changes. >> And you cannot just remove the hints to fix gcc -O3 optimizer problems.
> > Yes, this is troubling me. In particular, the HP-UX hints file seems to > represent a real current issue. I may be able to test that.
yes, I overlooked the HP-UX hints file.
> I also discover that there is at least one bug still present. This the > branch that I pushed:
I can reproduce it, going to investigate what's happening.
CC: "perlbug-followup [...] perl.org" <perlbug-followup [...] perl.org>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Thu, 13 Jun 2013 03:07:04 -0700 (PDT)
To: Nicholas Clark <nick [...] ccl4.org>
From: Salvador Fandino <sfandino [...] yahoo.com>
Download (untitled) / with headers
text/plain 293b
On 06/13/2013 10:52 AM, Salvador Fandino wrote: Show quoted text
>> I also discover that there is at least one bug still present. This the >> branch that I pushed:
> > I can reproduce it, going to investigate what's happening. >
The SvMAGICAL test guarding the mg_find call was looking into the wrong SV.
CC: "perlbug-followup [...] perl.org" <perlbug-followup [...] perl.org>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Thu, 13 Jun 2013 03:13:18 -0700 (PDT)
To: Nicholas Clark <nick [...] ccl4.org>
From: Salvador Fandino <sfandino [...] yahoo.com>
Download (untitled) / with headers
text/plain 636b
Show quoted text
----- Original Message -----
> From: Salvador Fandino <sfandino@yahoo.com> > To: Nicholas Clark <nick@ccl4.org> > Cc: "perlbug-followup@perl.org" <perlbug-followup@perl.org> > Sent: Thursday, June 13, 2013 12:07 PM > Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks > > On 06/13/2013 10:52 AM, Salvador Fandino wrote: >
>>> I also discover that there is at least one bug still present. This the >>> branch that I pushed:
>> >> I can reproduce it, going to investigate what's happening. >>
> > The SvMAGICAL test guarding the mg_find call was looking into the wrong SV.
And the patch...

Message body is not shown because sender requested not to inline it.

CC: "perlbug-followup [...] perl.org" <perlbug-followup [...] perl.org>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Thu, 13 Jun 2013 11:42:35 +0100
To: Salvador Fandino <sfandino [...] yahoo.com>
From: Nicholas Clark <nick [...] ccl4.org>
Download (untitled) / with headers
text/plain 1.7k
On Thu, Jun 13, 2013 at 03:13:18AM -0700, Salvador Fandino wrote: Show quoted text
> And the patch...
Thanks. Yes, this fixes the problem locally. I've pushed this to smoke-me/salva/Storable I'd already pushed a commit to fix a gcc-ism spotted by the HP compiler: commit 0e6b41b5683f2b07a6400d094ec39bdac28cdd8c Author: Nicholas Clark <nick@ccl4.org> Date: Thu Jun 13 10:43:07 2013 +0200 In Storable.xs, don't attempt return the return value of a void function. Sadly gcc is fine with the idea of return func_which_returns_void(); being the same as return;. Vigilant C compilers are not. diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs index 5f57190..5a8ceea 100644 --- a/dist/Storable/Storable.xs +++ b/dist/Storable/Storable.xs @@ -1169,7 +1169,7 @@ static void store_ref(pTHX_ store_cxt_t *store_cxt, SV *sv) } else WRITE_MARK(is_weak ? SX_WEAKREF : SX_REF); - return store(aTHX_ store_cxt, sv); + store(aTHX_ store_cxt, sv); } /* @@ -1772,7 +1772,7 @@ static void store_code(pTHX_ store_cxt_t *store_cxt, CV *cv) /* * retrieve_code does not work with perl 5.005 or less */ - return store_other(aTHX_ retrieve_cxt, (SV*)cv); + store_other(aTHX_ retrieve_cxt, (SV*)cv); #else dSP; I32 len; @@ -1786,7 +1786,8 @@ static void store_code(pTHX_ store_cxt_t *store_cxt, CV *cv) (store_cxt->deparse < 0 && !(store_cxt->deparse = SvTRUE(perl_get_sv("Storable::Deparse", GV_ADD)) ? 1 : 0)) ) { - return store_other(aTHX_ store_cxt, (SV*)cv); + store_other(aTHX_ store_cxt, (SV*)cv); + return; } /* @@ -2460,7 +2461,7 @@ static void store_blessed( * Now emit the <object> part. */ - return SV_STORE(type)(aTHX_ store_cxt, sv); + SV_STORE(type)(aTHX_ store_cxt, sv); } /* Nicholas Clark
CC: "perlbug-followup [...] perl.org" <perlbug-followup [...] perl.org>
Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY blocks
Date: Thu, 13 Jun 2013 05:36:19 -0700 (PDT)
To: Nicholas Clark <nick [...] ccl4.org>
From: Salvador Fandino <sfandino [...] yahoo.com>
Download (untitled) / with headers
text/plain 689b
Show quoted text
----- Original Message -----
> From: Nicholas Clark <nick@ccl4.org> > To: Salvador Fandino <sfandino@yahoo.com> > Cc: "perlbug-followup@perl.org" <perlbug-followup@perl.org> > Sent: Thursday, June 13, 2013 12:42 PM > Subject: Re: Storable refactoring, was Re: [perl #118139] Storable in DESTROY
blocks
> > On Thu, Jun 13, 2013 at 03:13:18AM -0700, Salvador Fandino wrote: >
>> And the patch...
> > Thanks. Yes, this fixes the problem locally.
Another set of patches with minor fixes for some of the warnings appearing on the smoke reports:   - implicit conversions from const char * to char *   - implicit conversions from void* to char *   - several unused variables removed

Message body is not shown because sender requested not to inline it.

CC: perl5-porters [...] perl.org
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Sat, 13 Jul 2013 19:05:00 +0530
To: Reini Urban via RT <perlbug-followup [...] perl.org>
From: Abhijit Menon-Sen <ams [...] toroid.org>
Download (untitled) / with headers
text/plain 237b
At 2013-05-23 08:36:27 -0700, perlbug-followup@perl.org wrote: Show quoted text
> > The attached patch fixes the DESTROY issue. > This is a pretty rare use case, but deserves a CPAN release also.
Thanks, applied. Will release to CPAN shortly. -- ams
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1012b
On Thu Jun 13 05:37:22 2013, salva wrote: Show quoted text
> ----- Original Message -----
> > From: Nicholas Clark <nick@ccl4.org> > > To: Salvador Fandino <sfandino@yahoo.com> > > Cc: "perlbug-followup@perl.org" <perlbug-followup@perl.org> > > Sent: Thursday, June 13, 2013 12:42 PM > > Subject: Re: Storable refactoring, was Re: [perl #118139] Storable
> in DESTROY > blocks
> > > > On Thu, Jun 13, 2013 at 03:13:18AM -0700, Salvador Fandino wrote: > >
> >> And the patch...
> > > > Thanks. Yes, this fixes the problem locally.
> > Another set of patches with minor fixes for some of the warnings > appearing on the smoke reports: > > � - implicit conversions from const char * to char * > � - implicit conversions from void* to char * > � - several unused variables removed
Was this patch intended to be against an older version of perl? It doesn't apply to blead that I can see - in most of the chunks I don't see matching context. I don't see any unused variable warnings from building Storable currently. Tony
Subject: Re: [perl #118139] Storable in DESTROY blocks
Date: Tue, 06 Aug 2013 10:02:51 -0500
To: perlbug-followup [...] perl.org
From: Reini Urban <reini [...] cpanel.net>
Download (untitled) / with headers
text/plain 1.1k
On 08/06/2013 01:37 AM, Tony Cook via RT wrote: Show quoted text
> On Thu Jun 13 05:37:22 2013, salva wrote:
>> ----- Original Message -----
>>> From: Nicholas Clark <nick@ccl4.org> >>> To: Salvador Fandino <sfandino@yahoo.com> >>> Cc: "perlbug-followup@perl.org" <perlbug-followup@perl.org> >>> Sent: Thursday, June 13, 2013 12:42 PM >>> Subject: Re: Storable refactoring, was Re: [perl #118139] Storable
>> in DESTROY >> blocks
>>> >>> On Thu, Jun 13, 2013 at 03:13:18AM -0700, Salvador Fandino wrote: >>>
>>>> And the patch...
>>> >>> Thanks. Yes, this fixes the problem locally.
>> >> Another set of patches with minor fixes for some of the warnings >> appearing on the smoke reports: >> >> � - implicit conversions from const char * to char * >> � - implicit conversions from void* to char * >> � - several unused variables removed
> > Was this patch intended to be against an older version of perl? > > It doesn't apply to blead that I can see - in most of the chunks I don't > see matching context. > > I don't see any unused variable warnings from building Storable currently.
No, these were against Salvadore's branch -- Reini Working towards a true Modern Perl. Slim, functional, unbloated, compile-time optimizable
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 308b
On Sat, 13 Jul 2013 06:35:56 -0700, amenonsen wrote: Show quoted text
> At 2013-05-23 08:36:27 -0700, perlbug-followup@perl.org wrote:
> > > > The attached patch fixes the DESTROY issue. > > This is a pretty rare use case, but deserves a CPAN release also.
> > Thanks, applied. Will release to CPAN shortly.
Closing. Tony


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org