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
Comments
From @rurbanThis is a bug report for perl from rurban@cpanel.net, Storable segfaults when being used in DESTROY blocks during global Since this Storable fix needs to be released on CPAN also, I had to Flags: This perlbug was built using Perl 5.17.11 - Mon Apr 8 10:30:38 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: Locally applied patches: @INC for perl 5.18.0: Environment for perl 5.18.0: |
From @rurbanThe attached patch fixes the DESTROY issue. use Storable; -- |
From @rurban0001-perl-118139-Storable-2.42-die-in-DESTROY.patchFrom 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
|
The RT System itself - Status changed from 'new' to 'open' |
From @tseeOn 05/23/2013 05:30 PM, rurban@cpanel.net (via RT) wrote:
There's a more general problem with this: occasionally, C extensions --Steffen |
From @LeontOn Thu, May 23, 2013 at 5:45 PM, Steffen Mueller <smueller@cpan.org> wrote:
That would not solve this problem. This problem is not in a DESTROY Leon |
From @rurbanOn Thu May 23 08:36:26 2013, rurban wrote:
I"m not sure how to document the easy fix for the user E.g. by setting foo->new() to a lexical, -- |
From @LeontOn Thu, May 23, 2013 at 5:30 PM, rurban@cpanel.net
This doesn't make sense to me. perl deliberately destroys all objects Leon |
From @rurbanOn Thu May 23 09:08:29 2013, LeonT wrote:
gdb --args /usr/local/bin/perl5.18.0d -Mblib t/destroy.t Program received signal SIGSEGV, Segmentation fault. $ make Storable.i But my testcase is too optimized to reproduce the bug. - foo->new(); -- |
From @rurbanAttached is a revised patch. The problem is not the DESTROY block per se, but DESTROY being called
-- |
From @rurban0001-perl-118139-Storable-2.42-die-in-global-destruction.patchFrom 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
|
From @tseeOn 05/23/2013 05:57 PM, Leon Timmermans wrote:
Which would be fine if the Storable guts were held on to long enough to --Steffen |
From @rurbanOn 05/23/2013 10:58 AM, Leon Timmermans via RT wrote:
DESTROY in Storable in part of the problem. Working towards a true Modern Perl. |
From @nwc10On Thu, May 23, 2013 at 06:07:16PM +0200, Leon Timmermans wrote:
It's the internal context. 1..1 etc. The XS blesses the internal context object into class Storable::Cxt, and MODULE = Storable PACKAGE = Storable::Cxt void To de-obfuscate the above code you need to know this: #define kbuf (cxt->keybuf).arena Storable isn't helping itself by making a *reference* to the blessed context I've not fully worked this out, but I think that it's only using bless to However, I also *think* that Reini's patch is better than what we have now, Also, I'd be tempted to split the patch into the 5.6 fixups and the bug fix. Nicholas Clark |
From @nwc10On Tue, May 28, 2013 at 04:28:50PM +0100, Nicholas Clark wrote:
Right. As best I can tell the dance between ->msaved and ->membuf is a bit Secondly, and more usefully, keybuf and membuf aren't actually needed at I'm not sure how much sanity it will cost to refactor this. Nicholas Clark |
From @LeontOn Tue, May 28, 2013 at 6:17 PM, Nicholas Clark <nick@ccl4.org> wrote:
One fix that doesn't require too much refactoring may be to move it Leon |
From @nwc10On Tue, May 28, 2013 at 06:23:26PM +0200, Leon Timmermans wrote:
Yes, that feels like a good start. I wonder if doing that is enough to Nicholas Clark |
From @tseeOn 05/23/2013 09:41 PM, Reini Urban wrote:
*nods* Ideally, I think using a scheme like we have for Sereal might be the The "context" object (encoder or decoder for Sereal) are C structs The volatile part of the encoder/decoder state is cleared by pushing a To avoid concurrent use issues (or calling back into Sereal during Sereal doesn't have state that fundamentally needs clearing at --Steffen |
From @nwc10On Tue, May 28, 2013 at 05:28:06PM +0100, Nicholas Clark wrote:
I think it just might be. It's done on smoke-me/nicholas/Storable I've only pushed this to see what breaks. There's still the ChangeLog to Nicholas Clark |
From @nwc10On Thu, May 23, 2013 at 09:42:29AM -0700, Reini Urban via RT wrote:
I don't think that change does anything. For starters, HAS_RESTRICTED_HASHES #ifdef HvPLACEHOLDERS and HvPLACEHOLDERS was added by commit 8aacddc back in 5.7.2 So that simplifies to - I32 len = HvTOTALKEYS(hv); In turn, Storable.xs already does this for compatibility with 5.6.x and #ifndef HvTOTALKEYS and as HvTOTALKEYS was also first introduced by commit 8aacddc, Hence for those versions, HvTOTALKEYS(hv) is equivalent to HvKEYS(). Which - I32 len = HvTOTALKEYS(hv); Here's the difference in pre-processor output for 5.6.2, 5.8.0 and 5.8.9 with 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;
I could build blead's current Storable.xs on 5.6.1 and 5.6.2 without any Nicholas Clark |
From @salvaOn 05/23/2013 05:30 PM, rurban @ cpanel . net 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 Initially my plan was to send the patch to p5p once those bugs have been You can see the commits for a detailed history of the changes. But - 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 - Convert all the IO macros into static C functions and let the compiler - use Perl_croak to fail instead of returning 1 (for store) or NULL (for - 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 - lots of premature optimizations removed - lots of code de-duplication I think the most important change is moving the context object into the It seems that initially, probably to simplify the code, Storable used Then, perl got support for threads and the context was moved to thread Then, Storable got the hooks STORABLE_freeze and STORABLE_thaw, so in Then lots of work-arounds were added to solve common errors and memory 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 I might have introduced new bugs but IMO, now, the code is pretty much If you agree that having that into blead is a good idea, I think that |
From @LeontOn Wed, May 29, 2013 at 7:22 PM, Salvador Fandino <sfandino@yahoo.com> wrote:
Sounds good.
A CPAN smoke may be helpful :-) Leon |
From @nwc10On Wed, May 29, 2013 at 07:22:17PM +0200, Salvador Fandino wrote:
Yes. Thanks for tracking perl5-porters to make sure that duplication of work I've skimmed your changes and wow, you have been busy.
Good. I had wondered how possible this was.
I'd seen the existing implementation, and thought that using an SV had the
I had guessed this based on evidence such as this: #define kbuf (cxt->keybuf).arena I was tempted to eliminate such #defines by doing search and replace on their
This seems to have been the case. The context structure was first added in Tue Sep 14 22:13:28 MEST 1999 Raphael Manfredi <Raphael_Manfredi@pobox.com> Integrated "thread-safe" patch from Murray Nesbitt. Try to avoid compilation warning on 64-bit CPUs. Can't test it, 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 Changes since 0.6@11: - Moved interface to the "beta" status. Some tiny parts are still - Slightly reduced the size of the Storable image by factorizing - Classes can now redefine how they wish their instances to be serialized - The engine is now fully re-entrant.
The hooks were also added in that revision.
Please don't temp people to find you stronger examples. But thanks for your analysis of what went wrong.
Yes, this seems like a much more sane solution.
I think it would be good, once it's been thoroughly tested. You've said that On Wed, May 29, 2013 at 07:39:50PM +0200, Leon Timmermans wrote:
I think that the way to go is probably 1) fix what you know to be broken Then release it for real. In that I'm vary wary of a large-scale refactoring of something rather low That will take some time. Given that the changes made in the branch However, the changes in the branch smoke-me/nicholas/Storable start to be This code isn't just redundant, it's technically undefined behaviour: #if (PATCHLEVEL <= 6) #if defined(USE_ITHREADS) #define STORE_HASH_SORT \ #else /* ! USE_ITHREADS */ It was added in a refactoring which was discussed on this list in 2004. http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2004-02/msg00758.html Whilst I participated in that thread back then, I wasn't aware of the From the commit message on the attached patch: For 5.6.x and earlier, the interpreter context saving/restoring is not needed Moreover, the SAVESPTR(orig_perl); in the removed code is not just pointless, This code is likely unreachable anyway. It's only compiled in for 5.6.x and However, it takes quite a bit of effort to prove anything into viewing the @@ -1032,16 +1032,22 @@ static const char byteorderstr_56[] = {BYTEORDER_BYTES_56, 0}; -#if (PATCHLEVEL <= 6) #if defined(USE_ITHREADS) +static sorted(pTHX_ AV *av, STRLEN len) #else /* ! USE_ITHREADS */ #endif /* USE_ITHREADS */ -#else /* PATCHLEVEL > 6 */ [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 (sadly this isn't in the glorious Technicolor that Address Sanitizer gdb shows that the address of orig_perl is the problem address which Breakpoint 1, sorted (my_perl=0x605c0000ee80, av=0x60620001b6e8, len=2) Breakpoint 2, __asan_report_error (pc=7698647, bp=140737488345536, The code is redundant and buggy. It should go. Nicholas Clark |
From @nwc100001-In-Storable.xs-simplify-and-inline-the-macro-STORE_H.patchFrom 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
|
From @rjbs* Salvador Fandino <sfandino@yahoo.com> [2013-05-29T13:22:17]
Wow, no kidding! It looks like you've gotten some feedback on this, please let -- |
From @salva----- Original Message -----
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? |
From @tseeOn 06/09/2013 08:13 PM, Salvador Fandino wrote:
No. 5.005 has jumped the shark. Frankly, I consider 5.6.X to be so old that supporting it is purely a As far as I am concerned, even 5.8.X support becomes optional about a --Steffen |
From @ribasushiOn Sun, Jun 09, 2013 at 11:13:02AM -0700, Salvador Fandino wrote:
Weird... I had no notable problems configuring/compiling/bootstraping I hit another roadblock however - I am not sure how to properly build a rabbit@Ahasver:~/devel/p5-storable-imp$ git rev-parse HEAD 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 $]' rabbit@Ahasver:~/devel/storable-standalone$ perl Makefile.PL rabbit@Ahasver:~/devel/storable-standalone$ make Please advise - I will test on all perls I have once I get the ball Cheers |
From @salva----- Original Message -----
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. |
From @salva----- Original Message -----
Witch OS/distribution are you using?
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 |
From @ribasushiOn Mon, Jun 10, 2013 at 01:43:38AM -0700, Salvador Fandino wrote:
Various debians (stable/oldstable/sid/whatever)
Hmmm... Do you think you can put together a command log (shell history) Cheers |
From @salva----- Original Message -----
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 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 |
From @ribasushiOn Mon, Jun 10, 2013 at 02:01:54AM -0700, Salvador Fandino wrote:
I am an idiot. I went for https://github.com/salva/perl5/tree/improve_storable Ignore the noise, testing... |
From @ribasushiOn Sun, Jun 09, 2013 at 11:13:02AM -0700, Salvador Fandino wrote:
Amusingly enough everything works on my 5.6.1 and 5.6.2, as they are 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 |
From @tseeOn 06/10/2013 10:26 AM, Salvador Fandino wrote:
Yes. And they're paid to do so[1]. In the end, though, if you're doing the work, you get to choose to --Steffen [1] Yes, so is RedHat, but at least they're giving back to Perl some. |
From @salvaHi, ----- Original Message -----
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. |
From @rurbanOn 06/12/2013 11:28 AM, Salvador Fandiño via RT wrote:
I tracked the cpan history and latest blead and your patches Can you fix the ChangeLog entry? But I'm still working on some pointer-sign problems in your changes. -- Working towards a true Modern Perl. |
From @nwc10On Wed, Jun 12, 2013 at 02:29:38PM -0500, Reini Urban wrote:
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 This will tell us what the various systems set up to smoke blead make of it,
No, blead is upstream.
Yes, this is troubling me. In particular, the HP-UX hints file seems to I also discover that there is at least one bug still present. This the $ valgrind ./perl -T -Ilib cpan/Tie-RefHash/t/storable.t Perl built with -UDEBUGGING -Doptimize=-Os -Dusethreads (this is probably key - other permutations I happened to try didn't have Full -V is: Summary of my perl5 (revision 5 version 19 subversion 1) configuration: Characteristics of this binary (from libperl): I don't have any more time to look at this tonight, hence the dump of the Nicholas Clark |
From @salvaOn 06/12/2013 09:29 PM, Reini Urban wrote:
The Linux hints file says: # gcc -O3 (and higher) can cause code produced from Storable.xs that "is_stored" and "last_op_in_netorder" have been completely rewritten, On the other hand, it seems I overlooked the HP-UX hints. |
From @salva----- Original Message -----
yes, I overlooked the HP-UX hints file.
I can reproduce it, going to investigate what's happening. |
From @salvaOn 06/13/2013 10:52 AM, Salvador Fandino wrote:
The SvMAGICAL test guarding the mg_find call was looking into the wrong SV. |
From @salva----- Original Message -----
And the patch... |
From @salvaStorable_mg_find_crashes-1.patchFrom 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
|
From @nwc10On Thu, Jun 13, 2013 at 03:13:18AM -0700, Salvador Fandino wrote:
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 In Storable.xs, don't attempt return the return value of a void function. Inline Patchdiff --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 |
From @salva----- Original Message -----
Another set of patches with minor fixes for some of the warnings appearing on the smoke reports: - implicit conversions from const char * to char * |
From @salvaStorable_minor_improvements_1.patchFrom 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
|
From ams@toroid.orgAt 2013-05-23 08:36:27 -0700, perlbug-followup@perl.org wrote:
Thanks, applied. Will release to CPAN shortly. -- ams |
From @tonycozOn Thu Jun 13 05:37:22 2013, salva wrote:
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 I don't see any unused variable warnings from building Storable currently. Tony |
From @rurbanOn 08/06/2013 01:37 AM, Tony Cook via RT wrote:
No, these were against Salvadore's branch -- Working towards a true Modern Perl. |
From @tonycozOn Sat, 13 Jul 2013 06:35:56 -0700, amenonsen wrote:
Closing. Tony |
@tonycoz - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#118139 (status was 'resolved')
Searchable as RT118139$
The text was updated successfully, but these errors were encountered: