p5pRT commented 11 years ago

Migrated from rt.perl.org#118139 (status was 'resolved')

Searchable as RT118139$

p5pRT commented 11 years ago

From @rurban

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

p5pRT commented 11 years ago

From @rurban

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

p5pRT commented 11 years ago

From @rurban

0001-perl-118139-Storable-2.42-die-in-DESTROY.patch

```diff From fb66b956b9f00e34ebf6b41b197ba539e8c62126 Mon Sep 17 00:00:00 2001 From: Reini Urban 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 + + 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 + Version 2.42 + + Remove core references to SVt_BIND (no changes) + +Sat Apr 13 17:45:10 2013 +0100 James E Keenan + Version 2.41 + + Update INSTALLDIRS to favor installation under 'site' + (no changes) + +Fri Nov 30 23:03:09 2012 -0500 Steffen Mueller 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 package, if it is available. Normal errors are reported by having store() or retrieve() return C. 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 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 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 Dominic Dunlop Erik Haugan - Benjamin A. Holzman + Benjamin A. Holzman + Reini Urban 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 ```

p5pRT commented 11 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 11 years ago

From @tsee

On 05/23/2013 05:30 PM\, rurban@cpanel.net (via RT) wrote:

# 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-archive.perl.org/perl5/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

p5pRT commented 11 years ago

From @Leont

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.

Leon

p5pRT commented 11 years ago

From @rurban

On Thu May 23 08:36:26 2013\, rurban wrote:

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

p5pRT commented 11 years ago

From @Leont

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.

Leon

p5pRT commented 11 years ago

From @rurban

On Thu May 23 09:08:29 2013\, LeonT wrote:

On Thu\, May 23\, 2013 at 5:30 PM\, rurban@cpanel.net \perlbug\-followup@perl\.org wrote:

Storable segfaults when being used in DESTROY blocks during global destruction\, when accessing an already freed PL_modglobal or the internal ctx. The best fix is to die if used in global destruction. See the new test t/destroy.t\, which crashes in all perl versions.

Since this Storable fix needs to be released on CPAN also\, I had to add some compatibility fixes\, tested back to 5.6.

This doesn't make sense to me. perl deliberately destroys all objects before destroying anything else. PL_modglobal should still exist when any DESTROY is run.

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

p5pRT commented 11 years ago

From @rurban

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.

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

p5pRT commented 11 years ago

From @rurban

0001-perl-118139-Storable-2.42-die-in-global-destruction.patch

```diff From 8caa4ba424adf6f70de04f96f27d92d7ab84f820 Mon Sep 17 00:00:00 2001 From: Reini Urban 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 + + 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 + Version 2.42 + + Remove core references to SVt_BIND (no changes) + +Sat Apr 13 17:45:10 2013 +0100 James E Keenan + Version 2.41 + + Update INSTALLDIRS to favor installation under 'site' + (no changes) + +Fri Nov 30 23:03:09 2012 -0500 Steffen Mueller 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 package, if it is available. Normal errors are reported by having store() or retrieve() return C. 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 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 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 Dominic Dunlop Erik Haugan - Benjamin A. Holzman + Benjamin A. Holzman + Reini Urban 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 ```

p5pRT commented 11 years ago

From @tsee

On 05/23/2013 05:57 PM\, Leon Timmermans 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.

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

p5pRT commented 11 years ago

From @rurban

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. -- Reini

Working towards a true Modern Perl. Slim\, functional\, unbloated\, compile-time optimizable

p5pRT commented 11 years ago

From @nwc10

On Thu\, May 23\, 2013 at 06:07:16PM +0200\, Leon Timmermans wrote:

On Thu\, May 23\, 2013 at 5:30 PM\, rurban@cpanel.net \perlbug\-followup@perl\.org wrote:

Storable segfaults when being used in DESTROY blocks during global destruction\, when accessing an already freed PL_modglobal or the internal ctx. The best fix is to die if used in global destruction. See the new test t/destroy.t\, which crashes in all perl versions.

Since this Storable fix needs to be released on CPAN also\, I had to add some compatibility fixes\, tested back to 5.6.

This doesn't make sense to me. perl deliberately destroys all objects before destroying anything else. PL_modglobal should still exist when any DESTROY is run.

It's the internal context.

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

etc.

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

MODULE = Storable PACKAGE = Storable::Cxt

void DESTROY(self) SV *self PREINIT: stcxt_t *cxt = (stcxt_t *)SvPVX(SvRV(self)); PPCODE: if (kbuf) Safefree(kbuf); if (!cxt->membuf_ro && mbase) Safefree(mbase); if (cxt->membuf_ro && (cxt->msaved).arena) Safefree((cxt->msaved).arena);

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

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

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

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

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

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

Nicholas Clark

p5pRT commented 11 years ago

From @nwc10

On Tue\, May 28\, 2013 at 04:28:50PM +0100\, Nicholas Clark wrote:

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

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

p5pRT commented 11 years ago

From @Leont

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.

Leon

p5pRT commented 11 years ago

From @nwc10

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.

Nicholas Clark

p5pRT commented 11 years ago

From @tsee

On 05/23/2013 09:41 PM\, Reini Urban wrote:

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

p5pRT commented 11 years ago

From @nwc10

On Tue\, May 28\, 2013 at 05:28:06PM +0100\, Nicholas Clark wrote:

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

p5pRT commented 11 years ago

From @nwc10

On Thu\, May 23\, 2013 at 09:42:29AM -0700\, Reini Urban via RT wrote:

Attached is a revised patch.

@@ -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:

Inline Patch

```diff --- 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

p5pRT commented 11 years ago

From @salva

On 05/23/2013 05:30 PM\, rurban @ cpanel . net wrote:

# 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-archive.perl.org/perl5/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)

p5pRT commented 11 years ago

From @Leont

On Wed\, May 29\, 2013 at 7:22 PM\, Salvador Fandino \sfandino@yahoo\.com wrote:

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.

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

p5pRT commented 11 years ago

From @nwc10

On Wed\, May 29\, 2013 at 07:22:17PM +0200\, Salvador Fandino wrote:

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.

- Break the context into a two structures store_cxt and retrieve_cxt.

Good. I had wondered how possible this was.

- 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)

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.

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.

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.

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.

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.

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:

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=\) 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

p5pRT commented 11 years ago

From @nwc10

0001-In-Storable.xs-simplify-and-inline-the-macro-STORE_H.patch

```diff From 8b070a3a71641cd494b53b52522da40fd1c7355a Mon Sep 17 00:00:00 2001 From: Nicholas Clark Date: Wed, 29 May 2013 20:39:09 +0200 Subject: [PATCH] In Storable.xs, simplify and inline the macro STORE_HASH_SORT. 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. --- dist/Storable/Storable.xs | 36 +++++------------------------------- 1 files changed, 5 insertions(+), 31 deletions(-) diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs index 3d04db8..1297d54 100644 --- a/dist/Storable/Storable.xs +++ b/dist/Storable/Storable.xs @@ -1027,36 +1027,6 @@ static const char byteorderstr_56[] = {BYTEORDER_BYTES_56, 0}; SvRV_set(ref, NULL); \ SvREFCNT_dec(ref); \ } STMT_END -/* - * sort (used in store_hash) - conditionally use qsort when - * sortsv is not available ( <= 5.6.1 ). - */ - -#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 */ - -#define STORE_HASH_SORT \ - qsort((char *) AvARRAY(av), len, sizeof(SV *), sortcmp); - -#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); @@ -2316,7 +2286,11 @@ static int store_hash(pTHX_ stcxt_t *cxt, HV *hv) av_store(av, AvFILLp(av)+1, key); /* av_push(), really */ } - STORE_HASH_SORT; +#if (PATCHLEVEL <= 6) + qsort((char *) AvARRAY(av), len, sizeof(SV *), sortcmp); +#else + sortsv(AvARRAY(av), len, Perl_sv_cmp); +#endif for (i = 0; i < len; i++) { #ifdef HAS_RESTRICTED_HASHES -- 1.7.4.3 ```

p5pRT commented 11 years ago

From @rjbs

* 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.

-- rjbs

p5pRT commented 11 years ago

From @salva

----- 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?

p5pRT commented 11 years ago

From @tsee

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.

--Steffen

p5pRT commented 11 years ago

From @ribasushi

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.

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:

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

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

p5pRT commented 11 years ago

From @salva

----- 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.

p5pRT commented 11 years ago

From @salva

----- 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

p5pRT commented 11 years ago

From @ribasushi

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...

Cheers

p5pRT commented 11 years ago

From @salva

----- 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$

p5pRT commented 11 years ago

From @ribasushi

On Mon\, Jun 10\, 2013 at 02:01:54AM -0700\, Salvador Fandino wrote:

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...

p5pRT commented 11 years ago

From @ribasushi

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.

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

p5pRT commented 11 years ago

From @tsee

On 06/10/2013 10:26 AM\, Salvador Fandino wrote:

----- 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

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.

p5pRT commented 11 years ago

From @salva

Hi\,

----- 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.

p5pRT commented 11 years ago

From @rurban

On 06/12/2013 11:28 AM\, Salvador Fandiño via RT 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.

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

p5pRT commented 11 years ago

From @nwc10

On Wed\, Jun 12\, 2013 at 02:29:38PM -0500\, Reini Urban wrote:

On 06/12/2013 11:28 AM\, Salvador Fandiño via RT wrote:

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.

Also\, is the CPAN package stored in some version control system somewhere?

No\, blead is upstream.

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

p5pRT commented 11 years ago

From @salva

On 06/12/2013 09:29 PM\, Reini Urban wrote:

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.

p5pRT commented 11 years ago

From @salva

----- 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.

p5pRT commented 11 years ago

From @salva

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.

p5pRT commented 11 years ago

From @salva

----- 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...

p5pRT commented 11 years ago

From @salva

Storable_mg_find_crashes-1.patch

```diff From 79a100b6c866364d259c2ac6eb88ca70605bb891 Mon Sep 17 00:00:00 2001 From: Salvador Date: Thu, 13 Jun 2013 11:47:52 +0200 Subject: [PATCH] a SvMAGICAL test guarding a mg_find call was been performed on the wrong object --- dist/Storable/Storable.xs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs index 5f57190..99414ea 100644 --- a/dist/Storable/Storable.xs +++ b/dist/Storable/Storable.xs @@ -2166,7 +2166,7 @@ static int store_hook(pTHX_ store_cxt_t *store_cxt, SV *sv, int type, HV *pkg, S * references. We use magic as a marker on the hook SV * that the class does not use STORABLE_attach at all */ - if (!SvMAGICAL(sv) || !mg_find(hook, PERL_MAGIC_ext)) { + if (!SvMAGICAL(hook) || !mg_find(hook, PERL_MAGIC_ext)) { GV* gv = gv_fetchmethod_autoload(pkg, "STORABLE_attach", FALSE); if (gv && isGV(gv)) CROAK(("Freeze cannot return references if %s class is using STORABLE_attach", classname)); -- 1.8.1.2 ```

p5pRT commented 11 years ago

From @nwc10

On Thu\, Jun 13\, 2013 at 03:13:18AM -0700\, Salvador Fandino wrote:

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.

Inline Patch

```diff 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

Perl / perl5

Storable in DESTROY blocks #12987

From @rurban

From @rurban

From @rurban

From @tsee

From @Leont

From @rurban

From @Leont

From @rurban

From @rurban

From @rurban

From @tsee

From @rurban

From @nwc10

From @nwc10

From @Leont

From @nwc10

From @tsee

From @nwc10

From @nwc10

From @salva

From @Leont

From @nwc10

./perl -Ilib -MStorable -e '++$Storable​::canonical; Storable​::freeze({1..4})'

From @nwc10

From @rjbs

From @salva

From @tsee

From @ribasushi

From @salva

From @salva

From @ribasushi

From @salva

From @ribasushi

From @ribasushi

From @tsee

From @salva

From @rurban

From @nwc10

From @salva

From @salva

From @salva

From @salva

From @salva

From @nwc10

From @salva

From @salva

From ams@toroid.org

From @tonycoz

From @rurban

From @tonycoz

./perl -Ilib -MStorable -e '++$Storable::canonical; Storable::freeze({1..4})'