Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storable in DESTROY blocks #12987

Closed
p5pRT opened this issue May 23, 2013 · 51 comments
Closed

Storable in DESTROY blocks #12987

p5pRT opened this issue May 23, 2013 · 51 comments

Comments

@p5pRT
Copy link

p5pRT commented May 23, 2013

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

Searchable as RT118139$

@p5pRT
Copy link
Author

p5pRT commented May 23, 2013

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
Copy link
Author

p5pRT commented May 23, 2013

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
Copy link
Author

p5pRT commented May 23, 2013

From @rurban

0001-perl-118139-Storable-2.42-die-in-DESTROY.patch
From fb66b956b9f00e34ebf6b41b197ba539e8c62126 Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Thu, 23 May 2013 10:31:58 -0500
Subject: [PATCH] [perl #118139] Storable 2.42 - die in DESTROY

---
 dist/Storable/ChangeLog   |   19 ++++++++++++++-
 dist/Storable/Storable.pm |   16 +++++++++---
 dist/Storable/Storable.xs |   59 +++++++++++++++++++++++++++++++++++++--------
 dist/Storable/t/destroy.t |   21 ++++++++++++++++
 4 files changed, 100 insertions(+), 15 deletions(-)
 create mode 100644 dist/Storable/t/destroy.t

diff --git a/dist/Storable/ChangeLog b/dist/Storable/ChangeLog
index 31e1b0c..ceaf128 100644
--- a/dist/Storable/ChangeLog
+++ b/dist/Storable/ChangeLog
@@ -1,4 +1,21 @@
-?
+Thu May 23 09:59:33 CDT 2013    Reini Urban <rurban@cpanel.net>
+
+        Protect against SEGV during global destruction when used
+        in DESTROY blocks [perl #118139].
+        Compatibility fixes back to 5.6.
+
+Wed May 8 18:27:42 2013 -0600 Karl Williamson <public@khwilliamson.com>
+    Version 2.42
+
+        Remove core references to SVt_BIND (no changes)
+
+Sat Apr 13 17:45:10 2013 +0100 James E Keenan <jkeenan@cpan.org>
+    Version 2.41
+
+        Update INSTALLDIRS to favor installation under 'site'
+        (no changes)
+
+Fri Nov 30 23:03:09 2012 -0500 Steffen Mueller <smueller@cpan.org>
     Version 2.40
 
 	Security warnings section added
diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm
index 0cc5d16..327dcc1 100644
--- a/dist/Storable/Storable.pm
+++ b/dist/Storable/Storable.pm
@@ -1,5 +1,6 @@
 #
-#  Copyright (c) 1995-2000, Raphael Manfredi
+#  Copyright (c) 1995-2001, Raphael Manfredi
+#  Copyright (c) 2002-2013 by the Perl 5 Porters
 #  
 #  You may redistribute only under the same terms as Perl 5, as specified
 #  in the README file that comes with the distribution.
@@ -659,6 +660,9 @@ routine from the C<Log::Agent> package, if it is available.
 Normal errors are reported by having store() or retrieve() return C<undef>.
 Such errors are usually I/O errors (or truncated stream errors at retrieval).
 
+Storable will die not croak if used during global destruction. It is
+forbidden to use Storable functions in C<DESTROY {}> blocks.
+
 =head1 WIZARDS ONLY
 
 =head2 Hooks
@@ -1120,6 +1124,9 @@ to string or floating point.  In other words values that had been generated
 by integer operations such as logic ops and then not used in any string or
 arithmetic context before storing.
 
+You may not use Storable in C<DESTROY {}> blocks, because during global
+destruction the internal context is already destroyed.
+
 =head2 64 bit data in perl 5.6.0 and 5.6.1
 
 This section only applies to you if you have existing data written out
@@ -1192,7 +1199,8 @@ Thank you to (in chronological order):
 	Salvador Ortiz Garcia <sog@msg.com.mx>
 	Dominic Dunlop <domo@computer.org>
 	Erik Haugan <erik@solbors.no>
-    Benjamin A. Holzman <ben.holzman@grantstreet.com>
+	Benjamin A. Holzman <ben.holzman@grantstreet.com>
+	Reini Urban <rurban@cpanel.net>
 
 for their bug reports, suggestions and contributions.
 
@@ -1205,8 +1213,8 @@ a binary incompatibility for the Storable image starting at version
 0.6--older images are, of course, still properly understood).
 Murray Nesbitt made Storable thread-safe.  Marc Lehmann added overloading
 and references to tied items support.  Benjamin Holzman added a performance
-improvement for overloaded classes; thanks to Grant Street Group for footing
-the bill.
+improvement for overloaded classes. Reini Urban protected against crashes
+in global destruction; thanks to Grant Street Group for footing the bill.
 
 =head1 AUTHOR
 
diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index 08641cd..e259908 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -23,6 +23,9 @@
 #define NEED_newCONSTSUB
 #define NEED_newSVpvn_flags
 #define NEED_newRV_noinc
+#define NEED_PL_parser
+#define NEED_sv_2pv_flags
+#define NEED_SvIsCOW
 #include "ppport.h"             /* handle old perls */
 #endif
 
@@ -81,6 +84,14 @@
 #  define HvTOTALKEYS(hv)	HvKEYS(hv)
 #endif
 
+#if !defined(PERL_CORE) && !defined(PL_dirty) && (PERL_VERSION >= 14)
+#  define PL_dirty (PL_phase == PERL_PHASE_DESTRUCT)
+#endif
+
+#ifndef SvIsCOW
+#  define SvIsCOW(x) 0
+#endif
+
 #ifdef DEBUGME
 
 #ifndef DASSERT
@@ -348,19 +359,22 @@ typedef struct stcxt {
 #if defined(MULTIPLICITY) || defined(PERL_OBJECT) || defined(PERL_CAPI)
 
 #if (PATCHLEVEL <= 4) && (SUBVERSION < 68)
-#define dSTCXT_SV 									\
-	SV *perinterp_sv = perl_get_sv(MY_VERSION, 0)
+#define dSTCXT_SV                                                     \
+        SV *perinterp_sv = NULL;                                      \
+	if (!PL_dirty)                                                \
+	  perinterp_sv = perl_get_sv(MY_VERSION, 0)
 #else	/* >= perl5.004_68 */
-#define dSTCXT_SV									\
-	SV *perinterp_sv = *hv_fetch(PL_modglobal,		\
-		MY_VERSION, sizeof(MY_VERSION)-1, TRUE)
+#define dSTCXT_SV                                                     \
+        SV *perinterp_sv = NULL;                                      \
+	if (!PL_dirty)                                                \
+          perinterp_sv = *hv_fetch(PL_modglobal, MY_VERSION, sizeof(MY_VERSION)-1, TRUE)
 #endif	/* < perl5.004_68 */
 
 #define dSTCXT_PTR(T,name)							\
 	T name = ((perinterp_sv && SvIOK(perinterp_sv) && SvIVX(perinterp_sv)	\
 				? (T)SvPVX(SvRV(INT2PTR(SV*,SvIVX(perinterp_sv)))) : (T) 0))
-#define dSTCXT										\
-	dSTCXT_SV;										\
+#define dSTCXT                                                          \
+        dSTCXT_SV;                                                      \
 	dSTCXT_PTR(stcxt_t *, cxt)
 
 #define INIT_STCXT							\
@@ -2259,7 +2273,16 @@ sortcmp(const void *a, const void *b)
 static int store_hash(pTHX_ stcxt_t *cxt, HV *hv)
 {
 	dVAR;
-	I32 len = HvTOTALKEYS(hv);
+	I32 len =
+#if PERL_VERSION < 8
+             HvKEYS(hv);
+#else
+#  ifdef HAS_RESTRICTED_HASHES
+             HvTOTALKEYS(hv);
+#  else
+             HvUSEDKEYS(hv);
+#  endif
+#endif
 	I32 i;
 	int ret = 0;
 	I32 riter;
@@ -3731,7 +3754,11 @@ static int do_store(
 	TRACEME(("do_store (optype=%d, netorder=%d)",
 		optype, network_order));
 
-	optype |= ST_STORE;
+        if (PL_dirty) { /* already freed cxt */
+          Perl_die(aTHX_ "Can't store with Storable during global destruction");  /* croak too instable here */
+          return 0;
+        }
+        optype |= ST_STORE;
 
 	/*
 	 * Workaround for CROAK leak: if they enter with a "dirty" context,
@@ -6047,6 +6074,11 @@ static SV *do_retrieve(
 
 	TRACEME(("do_retrieve (optype = 0x%x)", optype));
 
+        if (PL_dirty) { /* already freed cxt */
+          Perl_die(aTHX_ "Can't retrieve with Storable during global destruction");  /* croak too instable here */
+          return &PL_sv_undef;
+        }
+
 	optype |= ST_RETRIEVE;
 
 	/*
@@ -6305,6 +6337,10 @@ static SV *dclone(pTHX_ SV *sv)
 	SV *out;
 
 	TRACEME(("dclone"));
+        if (PL_dirty) { /* already freed cxt */
+          Perl_die(aTHX_ "Can't clone with Storable during global destruction");  /* croak too instable here */
+          return &PL_sv_undef;
+        }
 
 	/*
 	 * Workaround for CROAK leak: if they enter with a "dirty" context,
@@ -6507,9 +6543,12 @@ last_op_in_netorder()
  PREINIT:
   bool result;
  PPCODE:
+  if (PL_dirty) {
+    Perl_die(aTHX_ "Can't use Storable during global destruction");
+    XSRETURN(0);
+  }
   if (ix) {
    dSTCXT;
-
    result = cxt->entry && (cxt->optype & ix) ? TRUE : FALSE;
   } else {
    result = !!last_op_in_netorder(aTHX);
diff --git a/dist/Storable/t/destroy.t b/dist/Storable/t/destroy.t
new file mode 100644
index 0000000..953e105
--- /dev/null
+++ b/dist/Storable/t/destroy.t
@@ -0,0 +1,21 @@
+# [perl #118139] crash in global destruction when accessing an
+# already freed PL_modglobalor accessing the freed cxt.
+use Test;
+use Storable;
+BEGIN {
+  plan tests => 1;
+  store {}, "foo";
+}
+package foo;
+sub new { return bless {} }
+DESTROY {
+  open $fh, "<", "foo";
+  eval { Storable::pretrieve($fh); };
+  unlink "foo";
+}
+
+package main;
+# print "# $^X\n";
+foo->new();
+
+ok(1);
-- 
1.7.10.4

@p5pRT
Copy link
Author

p5pRT commented May 23, 2013

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

@p5pRT
Copy link
Author

p5pRT commented May 23, 2013

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
Copy link
Author

p5pRT commented May 23, 2013

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
Copy link
Author

p5pRT commented May 23, 2013

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
Copy link
Author

p5pRT commented May 23, 2013

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
Copy link
Author

p5pRT commented May 23, 2013

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
Copy link
Author

p5pRT commented May 23, 2013

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
Copy link
Author

p5pRT commented May 23, 2013

From @rurban

0001-perl-118139-Storable-2.42-die-in-global-destruction.patch
From 8caa4ba424adf6f70de04f96f27d92d7ab84f820 Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Thu, 23 May 2013 10:31:58 -0500
Subject: [PATCH] [perl #118139] Storable 2.42 - die in global destruction

Protect against SEGV during global destruction, e.g. when used
in DESTROY blocks.
Compatibility fixes back to 5.6.
---
 dist/Storable/ChangeLog   |   19 ++++++++++++++-
 dist/Storable/Storable.pm |   18 +++++++++++---
 dist/Storable/Storable.xs |   59 +++++++++++++++++++++++++++++++++++++--------
 dist/Storable/t/destroy.t |   21 ++++++++++++++++
 4 files changed, 102 insertions(+), 15 deletions(-)
 create mode 100644 dist/Storable/t/destroy.t

diff --git a/dist/Storable/ChangeLog b/dist/Storable/ChangeLog
index 31e1b0c..ceaf128 100644
--- a/dist/Storable/ChangeLog
+++ b/dist/Storable/ChangeLog
@@ -1,4 +1,21 @@
-?
+Thu May 23 09:59:33 CDT 2013    Reini Urban <rurban@cpanel.net>
+
+        Protect against SEGV during global destruction when used
+        in DESTROY blocks [perl #118139].
+        Compatibility fixes back to 5.6.
+
+Wed May 8 18:27:42 2013 -0600 Karl Williamson <public@khwilliamson.com>
+    Version 2.42
+
+        Remove core references to SVt_BIND (no changes)
+
+Sat Apr 13 17:45:10 2013 +0100 James E Keenan <jkeenan@cpan.org>
+    Version 2.41
+
+        Update INSTALLDIRS to favor installation under 'site'
+        (no changes)
+
+Fri Nov 30 23:03:09 2012 -0500 Steffen Mueller <smueller@cpan.org>
     Version 2.40
 
 	Security warnings section added
diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm
index 0cc5d16..c203eb9 100644
--- a/dist/Storable/Storable.pm
+++ b/dist/Storable/Storable.pm
@@ -1,5 +1,6 @@
 #
-#  Copyright (c) 1995-2000, Raphael Manfredi
+#  Copyright (c) 1995-2001, Raphael Manfredi
+#  Copyright (c) 2002-2013 by the Perl 5 Porters
 #  
 #  You may redistribute only under the same terms as Perl 5, as specified
 #  in the README file that comes with the distribution.
@@ -659,6 +660,10 @@ routine from the C<Log::Agent> package, if it is available.
 Normal errors are reported by having store() or retrieve() return C<undef>.
 Such errors are usually I/O errors (or truncated stream errors at retrieval).
 
+Storable will die not croak if used during global destruction. It is
+dangerous to use Storable functions in C<DESTROY {}> blocks, esp. when
+the DESTROY method is being called during global destruction and not earlier.
+
 =head1 WIZARDS ONLY
 
 =head2 Hooks
@@ -1120,6 +1125,10 @@ to string or floating point.  In other words values that had been generated
 by integer operations such as logic ops and then not used in any string or
 arithmetic context before storing.
 
+You should not use Storable in C<DESTROY {}> blocks, or explcitly call the
+DESTROY method at run-time (e.g. set the object to undef or make it a lexical),
+because during global destruction it will fail.
+
 =head2 64 bit data in perl 5.6.0 and 5.6.1
 
 This section only applies to you if you have existing data written out
@@ -1192,7 +1201,8 @@ Thank you to (in chronological order):
 	Salvador Ortiz Garcia <sog@msg.com.mx>
 	Dominic Dunlop <domo@computer.org>
 	Erik Haugan <erik@solbors.no>
-    Benjamin A. Holzman <ben.holzman@grantstreet.com>
+	Benjamin A. Holzman <ben.holzman@grantstreet.com>
+	Reini Urban <rurban@cpanel.net>
 
 for their bug reports, suggestions and contributions.
 
@@ -1205,8 +1215,8 @@ a binary incompatibility for the Storable image starting at version
 0.6--older images are, of course, still properly understood).
 Murray Nesbitt made Storable thread-safe.  Marc Lehmann added overloading
 and references to tied items support.  Benjamin Holzman added a performance
-improvement for overloaded classes; thanks to Grant Street Group for footing
-the bill.
+improvement for overloaded classes. Reini Urban protected against crashes
+in global destruction; thanks to Grant Street Group for footing the bill.
 
 =head1 AUTHOR
 
diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index 08641cd..e259908 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -23,6 +23,9 @@
 #define NEED_newCONSTSUB
 #define NEED_newSVpvn_flags
 #define NEED_newRV_noinc
+#define NEED_PL_parser
+#define NEED_sv_2pv_flags
+#define NEED_SvIsCOW
 #include "ppport.h"             /* handle old perls */
 #endif
 
@@ -81,6 +84,14 @@
 #  define HvTOTALKEYS(hv)	HvKEYS(hv)
 #endif
 
+#if !defined(PERL_CORE) && !defined(PL_dirty) && (PERL_VERSION >= 14)
+#  define PL_dirty (PL_phase == PERL_PHASE_DESTRUCT)
+#endif
+
+#ifndef SvIsCOW
+#  define SvIsCOW(x) 0
+#endif
+
 #ifdef DEBUGME
 
 #ifndef DASSERT
@@ -348,19 +359,22 @@ typedef struct stcxt {
 #if defined(MULTIPLICITY) || defined(PERL_OBJECT) || defined(PERL_CAPI)
 
 #if (PATCHLEVEL <= 4) && (SUBVERSION < 68)
-#define dSTCXT_SV 									\
-	SV *perinterp_sv = perl_get_sv(MY_VERSION, 0)
+#define dSTCXT_SV                                                     \
+        SV *perinterp_sv = NULL;                                      \
+	if (!PL_dirty)                                                \
+	  perinterp_sv = perl_get_sv(MY_VERSION, 0)
 #else	/* >= perl5.004_68 */
-#define dSTCXT_SV									\
-	SV *perinterp_sv = *hv_fetch(PL_modglobal,		\
-		MY_VERSION, sizeof(MY_VERSION)-1, TRUE)
+#define dSTCXT_SV                                                     \
+        SV *perinterp_sv = NULL;                                      \
+	if (!PL_dirty)                                                \
+          perinterp_sv = *hv_fetch(PL_modglobal, MY_VERSION, sizeof(MY_VERSION)-1, TRUE)
 #endif	/* < perl5.004_68 */
 
 #define dSTCXT_PTR(T,name)							\
 	T name = ((perinterp_sv && SvIOK(perinterp_sv) && SvIVX(perinterp_sv)	\
 				? (T)SvPVX(SvRV(INT2PTR(SV*,SvIVX(perinterp_sv)))) : (T) 0))
-#define dSTCXT										\
-	dSTCXT_SV;										\
+#define dSTCXT                                                          \
+        dSTCXT_SV;                                                      \
 	dSTCXT_PTR(stcxt_t *, cxt)
 
 #define INIT_STCXT							\
@@ -2259,7 +2273,16 @@ sortcmp(const void *a, const void *b)
 static int store_hash(pTHX_ stcxt_t *cxt, HV *hv)
 {
 	dVAR;
-	I32 len = HvTOTALKEYS(hv);
+	I32 len =
+#if PERL_VERSION < 8
+             HvKEYS(hv);
+#else
+#  ifdef HAS_RESTRICTED_HASHES
+             HvTOTALKEYS(hv);
+#  else
+             HvUSEDKEYS(hv);
+#  endif
+#endif
 	I32 i;
 	int ret = 0;
 	I32 riter;
@@ -3731,7 +3754,11 @@ static int do_store(
 	TRACEME(("do_store (optype=%d, netorder=%d)",
 		optype, network_order));
 
-	optype |= ST_STORE;
+        if (PL_dirty) { /* already freed cxt */
+          Perl_die(aTHX_ "Can't store with Storable during global destruction");  /* croak too instable here */
+          return 0;
+        }
+        optype |= ST_STORE;
 
 	/*
 	 * Workaround for CROAK leak: if they enter with a "dirty" context,
@@ -6047,6 +6074,11 @@ static SV *do_retrieve(
 
 	TRACEME(("do_retrieve (optype = 0x%x)", optype));
 
+        if (PL_dirty) { /* already freed cxt */
+          Perl_die(aTHX_ "Can't retrieve with Storable during global destruction");  /* croak too instable here */
+          return &PL_sv_undef;
+        }
+
 	optype |= ST_RETRIEVE;
 
 	/*
@@ -6305,6 +6337,10 @@ static SV *dclone(pTHX_ SV *sv)
 	SV *out;
 
 	TRACEME(("dclone"));
+        if (PL_dirty) { /* already freed cxt */
+          Perl_die(aTHX_ "Can't clone with Storable during global destruction");  /* croak too instable here */
+          return &PL_sv_undef;
+        }
 
 	/*
 	 * Workaround for CROAK leak: if they enter with a "dirty" context,
@@ -6507,9 +6543,12 @@ last_op_in_netorder()
  PREINIT:
   bool result;
  PPCODE:
+  if (PL_dirty) {
+    Perl_die(aTHX_ "Can't use Storable during global destruction");
+    XSRETURN(0);
+  }
   if (ix) {
    dSTCXT;
-
    result = cxt->entry && (cxt->optype & ix) ? TRUE : FALSE;
   } else {
    result = !!last_op_in_netorder(aTHX);
diff --git a/dist/Storable/t/destroy.t b/dist/Storable/t/destroy.t
new file mode 100644
index 0000000..c28ba25
--- /dev/null
+++ b/dist/Storable/t/destroy.t
@@ -0,0 +1,21 @@
+# [perl #118139] crash in global destruction when accessing an
+# already freed PL_modglobal or accessing the freed cxt.
+use Test;
+use Storable;
+BEGIN {
+  plan tests => 1;
+  store {}, "foo";
+}
+package foo;
+sub new { return bless {} }
+DESTROY {
+  open $fh, "<", "foo";
+  eval { Storable::pretrieve($fh); };
+  unlink "foo";
+}
+
+package main;
+# print "# $^X\n";
+$x = foo->new();
+
+ok(1);
-- 
1.7.10.4

@p5pRT
Copy link
Author

p5pRT commented May 23, 2013

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
Copy link
Author

p5pRT commented May 23, 2013

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
Copy link
Author

p5pRT commented May 28, 2013

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
Copy link
Author

p5pRT commented May 28, 2013

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
Copy link
Author

p5pRT commented May 28, 2013

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
Copy link
Author

p5pRT commented May 28, 2013

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
Copy link
Author

p5pRT commented May 28, 2013

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
Copy link
Author

p5pRT commented May 28, 2013

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
Copy link
Author

p5pRT commented May 29, 2013

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 8aacddc 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 8aacddc,
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
--- 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
Copy link
Author

p5pRT commented May 29, 2013

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
Copy link
Author

p5pRT commented May 29, 2013

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
Copy link
Author

p5pRT commented Jun 4, 2013

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=<optimized out>)
  at /home/nick/src/gcc-4.8.0/libsanitizer/asan/asan_rtl.cc​:234
234 /home/nick/src/gcc-4.8.0/libsanitizer/asan/asan_rtl.cc​: No such file or directory.
(gdb) up
#2 0x00000000007578d7 in Perl_leave_scope (my_perl=0x605c0000ee80, base=22)
  at scope.c​:942
942 *(SV**)(ARG0_PTR)= ARG1_SV;
(gdb) p arg0.any_ptr
$2 = (void *) 0x7fffffffdf50
(gdb) up
#3 0x0000000000750fbf in Perl_pop_scope (my_perl=0x605c0000ee80)
  at scope.c​:110
110 LEAVE_SCOPE(oldsave);

The code is redundant and buggy. It should go.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jun 4, 2013

From @nwc10

0001-In-Storable.xs-simplify-and-inline-the-macro-STORE_H.patch
From 8b070a3a71641cd494b53b52522da40fd1c7355a Mon Sep 17 00:00:00 2001
From: Nicholas Clark <nick@ccl4.org>
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
Copy link
Author

p5pRT commented Jun 7, 2013

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
Copy link
Author

p5pRT commented Jun 9, 2013

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
Copy link
Author

p5pRT commented Jun 10, 2013

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
Copy link
Author

p5pRT commented Jun 10, 2013

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
Copy link
Author

p5pRT commented Jun 10, 2013

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
Copy link
Author

p5pRT commented Jun 10, 2013

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
Copy link
Author

p5pRT commented Jun 10, 2013

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
Copy link
Author

p5pRT commented Jun 10, 2013

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
Copy link
Author

p5pRT commented Jun 10, 2013

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
Copy link
Author

p5pRT commented Jun 10, 2013

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
Copy link
Author

p5pRT commented Jun 10, 2013

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
Copy link
Author

p5pRT commented Jun 12, 2013

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
Copy link
Author

p5pRT commented Jun 12, 2013

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
Copy link
Author

p5pRT commented Jun 12, 2013

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​: 5fa0439
  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
Copy link
Author

p5pRT commented Jun 13, 2013

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
Copy link
Author

p5pRT commented Jun 13, 2013

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
Copy link
Author

p5pRT commented Jun 13, 2013

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
Copy link
Author

p5pRT commented Jun 13, 2013

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
Copy link
Author

p5pRT commented Jun 13, 2013

From @salva

Storable_mg_find_crashes-1.patch
From 79a100b6c866364d259c2ac6eb88ca70605bb891 Mon Sep 17 00:00:00 2001
From: Salvador <sfandino@yahoo.com>
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
Copy link
Author

p5pRT commented Jun 13, 2013

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 0e6b41b
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 --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index 5f57190..5a8ceea 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -1169,7 +1169,7 @@ static void store_ref(pTHX_ store_cxt_t *store_cxt, SV *sv)
 	} else
 		WRITE_MARK(is_weak ? SX_WEAKREF : SX_REF);
 
-	return store(aTHX_ store_cxt, sv);
+	store(aTHX_ store_cxt, sv);
 }
 
 /*
@@ -1772,7 +1772,7 @@ static void store_code(pTHX_ store_cxt_t *store_cxt, CV *cv)
     /*
 	 * retrieve_code does not work with perl 5.005 or less
 	 */
-	return store_other(aTHX_ retrieve_cxt, (SV*)cv);
+	store_other(aTHX_ retrieve_cxt, (SV*)cv);
 #else
 	dSP;
 	I32 len;
@@ -1786,7 +1786,8 @@ static void store_code(pTHX_ store_cxt_t *store_cxt, CV *cv)
 		(store_cxt->deparse < 0 && !(store_cxt->deparse =
 			SvTRUE(perl_get_sv("Storable::Deparse", GV_ADD)) ? 1 : 0))
 	) {
-		return store_other(aTHX_ store_cxt, (SV*)cv);
+		store_other(aTHX_ store_cxt, (SV*)cv);
+		return;
 	}
 
 	/*
@@ -2460,7 +2461,7 @@ static void store_blessed(
 	 * Now emit the <object> part.
 	 */
 
-	return SV_STORE(type)(aTHX_ store_cxt, sv);
+	SV_STORE(type)(aTHX_ store_cxt, sv);
 }
 
 /*


Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jun 13, 2013

From @salva

----- Original Message -----

From​: Nicholas Clark <nick@​ccl4.org>
To​: Salvador Fandino <sfandino@​yahoo.com>
Cc​: "perlbug-followup@​perl.org" <perlbug-followup@​perl.org>
Sent​: Thursday, June 13, 2013 12​:42 PM
Subject​: Re​: Storable refactoring, was Re​: [perl #118139] Storable in DESTROY
blocks

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

And the patch...

Thanks. Yes, this fixes the problem locally.

Another set of patches with minor fixes for some of the warnings appearing on the smoke reports​:

  - implicit conversions from const char * to char *
  - implicit conversions from void* to char *
  - several unused variables removed

@p5pRT
Copy link
Author

p5pRT commented Jun 13, 2013

From @salva

Storable_minor_improvements_1.patch
From 36970dfa82c805fe679e02ecddd1388bdcf13d3a Mon Sep 17 00:00:00 2001
From: Salvador <sfandino@yahoo.com>
Date: Thu, 13 Jun 2013 13:17:27 +0200
Subject: [PATCH 1/5] remove some warning related to implicit conversion of
 const char * to char *

---
 dist/Storable/Storable.xs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index ddfe410..6bff6eb 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -352,8 +352,8 @@ struct st_retrieve_cxt {
 #endif
 
 static void
-croak_io_error(pTHX_ SSize_t rc, retrieve_cxt_t *retrieve_cxt, char *str) {
-        char *error;
+croak_io_error(pTHX_ SSize_t rc, retrieve_cxt_t *retrieve_cxt, const char *str) {
+        const char *error;
         if (rc < 0) {
                 SV *ioe = GvSV(gv_fetchpvs("!", GV_ADDMULTI, SVt_PV));
                 error = SvPV_nolen(ioe);
@@ -367,7 +367,7 @@ croak_io_error(pTHX_ SSize_t rc, retrieve_cxt_t *retrieve_cxt, char *str) {
                            (retrieve_cxt->input_fh ? "file" : "string"),
                            error);
         else
-                Perl_croak(aTHX_ "%s: %s", str, error);
+                Perl_croak(aTHX_ "%s: %s", (char *)str, (char*)error);
 }
 
 #define READ_ERROR(bytes)                                          \
@@ -3098,7 +3098,7 @@ static SV *retrieve_hook(pTHX_ retrieve_cxt_t *retrieve_cxt, const char *cname)
                                 load_module(PERL_LOADMOD_NOIMPORT, newSVsv(class_sv), Nullsv);
                         }
                         for (is_thaw = 0; is_thaw < 2; is_thaw++) {
-                                char *method = (is_thaw ? "STORABLE_thaw" : "STORABLE_attach");
+                                const char *method = (is_thaw ? "STORABLE_thaw" : "STORABLE_attach");
                                 GV *gv = gv_fetchmethod_autoload(gv_stashsv(class_sv, GV_ADD),
                                                                  method, FALSE);
                                 if (gv && isGV(gv)) {
-- 
1.8.1.2


From 6711426aa870b679c4c4a64477deaa73e257f8e7 Mon Sep 17 00:00:00 2001
From: Salvador <sfandino@yahoo.com>
Date: Thu, 13 Jun 2013 13:17:59 +0200
Subject: [PATCH 2/5] remove implicit conversion of void* to char*

---
 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 6bff6eb..c9ea996 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -1109,7 +1109,7 @@ downgrade_restricted(pTHX) {
  */
 static int known_class(pTHX_ store_cxt_t *store_cxt, HV *pkg, I32 *classnum) {
 
-        char *tag1;
+        void *tag1;
 
 	TRACEME(("known_class (%s)", HvNAME_get(pkg)));
 
-- 
1.8.1.2


From bbfe426a5bddb2149aea66f2b2c2f0acc0a7a605 Mon Sep 17 00:00:00 2001
From: Salvador <sfandino@yahoo.com>
Date: Thu, 13 Jun 2013 13:34:13 +0200
Subject: [PATCH 3/5] remove unused variable

---
 dist/Storable/Storable.xs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index c9ea996..53e1d42 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -1395,7 +1395,6 @@ static void store_array(pTHX_ store_cxt_t *store_cxt, AV *av)
 	SV **sav;
 	I32 len = av_len(av) + 1;
 	I32 i;
-	int ret;
 
 	TRACEME(("store_array (0x%"UVxf")", PTR2UV(av)));
 
-- 
1.8.1.2


From 2e4c16195c2a094895c3de7c7048bd62d24c636d Mon Sep 17 00:00:00 2001
From: Salvador <sfandino@yahoo.com>
Date: Thu, 13 Jun 2013 13:36:26 +0200
Subject: [PATCH 4/5] remove unused variable

---
 dist/Storable/Storable.xs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index 53e1d42..98a278e 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -2037,7 +2037,6 @@ static int store_hook(pTHX_ store_cxt_t *store_cxt, SV *sv, int type, HV *pkg, S
         I32 ax;
 	char *classname;
 	STRLEN frozenlen;
-	SV *ref;
 	int count, i;
 	unsigned char flags;
 	char *frozenpv;
-- 
1.8.1.2


From 2c14714f158634521f9d5071ca288fe5fd4502cb Mon Sep 17 00:00:00 2001
From: Salvador <sfandino@yahoo.com>
Date: Thu, 13 Jun 2013 13:37:55 +0200
Subject: [PATCH 5/5] remove several unused variables

---
 dist/Storable/Storable.xs | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index 98a278e..e99e902 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -2587,7 +2587,6 @@ static int sv_type(pTHX_ SV *sv)
 static void store(pTHX_ store_cxt_t *store_cxt, SV *sv)
 {
 	void *tag1;
-	int ret;
 	int type;
 	PTR_TBL_t *pseen = store_cxt->pseen;
 
@@ -2861,7 +2860,6 @@ static SV *retrieve_idx_blessed(pTHX_ retrieve_cxt_t *retrieve_cxt, const char *
 	I32 idx;
 	const char *classname;
 	SV **sva;
-	SV *sv;
 
 	PERL_UNUSED_ARG(cname);
 	TRACEME(("retrieve_idx_blessed (#%d)", retrieve_cxt->tagnum));
@@ -3624,7 +3622,6 @@ static SV *retrieve_lutf8str(pTHX_ retrieve_cxt_t *retrieve_cxt, const char *cna
 static SV *retrieve_vstring_any(pTHX_ retrieve_cxt_t *retrieve_cxt, const char *cname, int l) {
 #ifdef SvVOK
 	SV *sv, *s;
-        MAGIC *mg;
         I32 len;
         READ_VARINT(l, len);
         READ_SVPV(s, len);
-- 
1.8.1.2

@p5pRT
Copy link
Author

p5pRT commented Jul 13, 2013

From ams@toroid.org

At 2013-05-23 08​:36​:27 -0700, perlbug-followup@​perl.org wrote​:

The attached patch fixes the DESTROY issue.
This is a pretty rare use case, but deserves a CPAN release also.

Thanks, applied. Will release to CPAN shortly.

-- ams

@p5pRT
Copy link
Author

p5pRT commented Aug 6, 2013

From @tonycoz

On Thu Jun 13 05​:37​:22 2013, salva wrote​:

----- Original Message -----

From​: Nicholas Clark <nick@​ccl4.org>
To​: Salvador Fandino <sfandino@​yahoo.com>
Cc​: "perlbug-followup@​perl.org" <perlbug-followup@​perl.org>
Sent​: Thursday, June 13, 2013 12​:42 PM
Subject​: Re​: Storable refactoring, was Re​: [perl #118139] Storable
in DESTROY
blocks

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

And the patch...

Thanks. Yes, this fixes the problem locally.

Another set of patches with minor fixes for some of the warnings
appearing on the smoke reports​:

� - implicit conversions from const char * to char *
� - implicit conversions from void* to char *
� - several unused variables removed

Was this patch intended to be against an older version of perl?

It doesn't apply to blead that I can see - in most of the chunks I don't
see matching context.

I don't see any unused variable warnings from building Storable currently.

Tony

@p5pRT
Copy link
Author

p5pRT commented Aug 6, 2013

From @rurban

On 08/06/2013 01​:37 AM, Tony Cook via RT wrote​:

On Thu Jun 13 05​:37​:22 2013, salva wrote​:

----- Original Message -----

From​: Nicholas Clark <nick@​ccl4.org>
To​: Salvador Fandino <sfandino@​yahoo.com>
Cc​: "perlbug-followup@​perl.org" <perlbug-followup@​perl.org>
Sent​: Thursday, June 13, 2013 12​:42 PM
Subject​: Re​: Storable refactoring, was Re​: [perl #118139] Storable
in DESTROY
blocks

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

And the patch...

Thanks. Yes, this fixes the problem locally.

Another set of patches with minor fixes for some of the warnings
appearing on the smoke reports​:

� - implicit conversions from const char * to char *
� - implicit conversions from void* to char *
� - several unused variables removed

Was this patch intended to be against an older version of perl?

It doesn't apply to blead that I can see - in most of the chunks I don't
see matching context.

I don't see any unused variable warnings from building Storable currently.

No, these were against Salvadore's branch

--
Reini

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

@p5pRT
Copy link
Author

p5pRT commented Dec 6, 2017

From @tonycoz

On Sat, 13 Jul 2013 06​:35​:56 -0700, amenonsen wrote​:

At 2013-05-23 08​:36​:27 -0700, perlbug-followup@​perl.org wrote​:

The attached patch fixes the DESTROY issue.
This is a pretty rare use case, but deserves a CPAN release also.

Thanks, applied. Will release to CPAN shortly.

Closing.

Tony

@p5pRT p5pRT closed this as completed Dec 6, 2017
@p5pRT
Copy link
Author

p5pRT commented Dec 6, 2017

@tonycoz - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant