Misleading error message from Storable #7436
Comments
From rick@bort.caThis is a bug report for perl from rick@bort.ca, % perl -MStorable=thaw -e 'thaw("a")' So far so good. % perl -MStorable=thaw -e 'thaw("aa")' It would be nice if the second had the same error as the first. Flags: Site configuration information for perl v5.8.3: Configured by Debian Project at Sun Feb 15 17:22:09 EST 2004. Summary of my perl5 (revision 5.0 version 8 subversion 3) configuration: Locally applied patches: @INC for perl v5.8.3: Environment for perl v5.8.3: |
From @jkeenanOn Sat Jul 24 08:20:35 2004, rickdelaney wrote:
I can confirm that, notwithstanding minor changes in the error messages, $ perl -MStorable=thaw -e 'thaw("aa")' Thank you very much. |
|
The RT System itself - Status changed from 'new' to 'open' |
From @doyI'm inclined to call this not a bug. How is perl supposed to |
From rick@bort.caOn Jul 02 2012, Jesse Luehrs via RT wrote:
I think that my original problem was that I was doing thaw($x) where $x I don't know the structure of the Storable binary image but I would -- |
From @dcollinsnStorable.xs: /* a is decimal 97 is ascii. The first two bytes are interpreted as major.minor version, but the first byte is first rightshifted - apparently the least significant bit is used to encode something else. So, "aa" actually survives partway through magic_check(), when Storable discovers that 48.97 is much too high a version. "a" is the unusual case, because there isn't a second byte to read: if (version_major > 1) And there is no character to be had, so it returns from magic_check without setting any other diagnostics. In any event - there is no magic number that we can possibly check for thaw() from memory. The first two bytes are the version number. There are a number of different ways we can get weird error messages. So we should really just give a better error message in the cases that are most likely to be accidentally stumbled on. I think that's the "version more recent" error as well as the "byte order not compatible" error (which you can get by passing in "\004\012\000xxxx"). Any other error message requires a crafted string like "\004\012\01012345678xxxx", which seems unlikely to happen by accident. Will upload the patch momentarily. |
From @dcollinsnAs promised - I propose this patch which changes the text of two error messages to propose that the data might also be "corrupt or not a Storable binary image". Tests are updated with the new error text. Porting/cmp_version is "failing", but I'm not sure what to do with that information, or if that's for the pumpking to $VERSION++. |
From @dcollinsn0001-RT-30807-Clearer-error-for-thaw-text.patchFrom e01cf874b04d1704a9b5bb40a7e3f476d83f5a52 Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Wed, 6 Jul 2016 23:26:32 -0400
Subject: [PATCH] [RT #30807] Clearer error for thaw('text')
---
dist/Storable/Storable.xs | 6 +++---
dist/Storable/t/croak.t | 2 +-
dist/Storable/t/malice.t | 10 +++++-----
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/dist/Storable/Storable.xs b/dist/Storable/Storable.xs
index 83cd001..3be1e07 100644
--- a/dist/Storable/Storable.xs
+++ b/dist/Storable/Storable.xs
@@ -6006,7 +6006,7 @@ static SV *magic_check(pTHX_ stcxt_t *cxt)
croak_now = 0; /* Don't croak yet. */
}
if (croak_now) {
- CROAK(("Storable binary image v%d.%d more recent than I am (v%d.%d)",
+ CROAK(("Stored data corrupt, not a Storable binary image, or binary image version v%d.%d more recent than I am (v%d.%d)",
version_major, version_minor,
STORABLE_BIN_MAJOR, STORABLE_BIN_MINOR));
}
@@ -6040,12 +6040,12 @@ static SV *magic_check(pTHX_ stcxt_t *cxt)
if (SvTRUE(perl_get_sv("Storable::interwork_56_64bit", GV_ADD))) {
if ((c != (sizeof (byteorderstr_56) - 1))
|| memNE(buf, byteorderstr_56, c))
- CROAK(("Byte order is not compatible"));
+ CROAK(("Stored data corrupt, not a Storable binary image, or byte order is not compatible"));
} else
#endif
{
if ((c != (sizeof (byteorderstr) - 1)) || memNE(buf, byteorderstr, c))
- CROAK(("Byte order is not compatible"));
+ CROAK(("Stored data corrupt, not a Storable binary image, or byte order is not compatible"));
}
current = buf + c;
diff --git a/dist/Storable/t/croak.t b/dist/Storable/t/croak.t
index ecd2bf8..366ff52 100644
--- a/dist/Storable/t/croak.t
+++ b/dist/Storable/t/croak.t
@@ -28,7 +28,7 @@ print "1..2\n";
for my $test (1,2) {
eval {thaw "\xFF\xFF"};
- if ($@ =~ /Storable binary image v127.255 more recent than I am \(v2\.\d+\)/)
+ if ($@ =~ /Stored data corrupt, not a Storable binary image, or binary image version v127.255 more recent than I am \(v2\.\d+\)/)
{
print "ok $test\n";
} else {
diff --git a/dist/Storable/t/malice.t b/dist/Storable/t/malice.t
index 867a0d7..ace6543 100644
--- a/dist/Storable/t/malice.t
+++ b/dist/Storable/t/malice.t
@@ -165,7 +165,7 @@ sub test_things {
local $Storable::accept_future_minor = 0;
test_corrupt ($copy, $sub,
- "/^Storable binary image v$header->{major}\.$minor6 more recent than I am \\(v$header->{major}\.$minor\\)/",
+ "/^Stored data corrupt, not a Storable binary image, or binary image version v$header->{major}\.$minor6 more recent than I am \\(v$header->{major}\.$minor\\)/",
"higher minor");
}
@@ -173,14 +173,14 @@ sub test_things {
my $major1 = $header->{major} + 1;
substr ($copy, $file_magic, 1) = chr 2*$major1;
test_corrupt ($copy, $sub,
- "/^Storable binary image v$major1\.$header->{minor} more recent than I am \\(v$header->{major}\.$minor\\)/",
+ "/^Stored data corrupt, not a Storable binary image, or binary image version v$major1\.$header->{minor} more recent than I am \\(v$header->{major}\.$minor\\)/",
"higher major");
# Continue messing with the previous copy
my $minor1 = $header->{minor} - 1;
substr ($copy, $file_magic + 1, 1) = chr $minor1;
test_corrupt ($copy, $sub,
- "/^Storable binary image v$major1\.$minor1 more recent than I am \\(v$header->{major}\.$minor\\)/",
+ "/^Stored data corrupt, not a Storable binary image, or binary image version v$major1\.$minor1 more recent than I am \\(v$header->{major}\.$minor\\)/",
"higher major, lower minor");
my $where;
@@ -191,7 +191,7 @@ sub test_things {
substr ($copy, $file_magic + 3, length $header->{byteorder})
= reverse $header->{byteorder};
- test_corrupt ($copy, $sub, "/^Byte order is not compatible/",
+ test_corrupt ($copy, $sub, "/^Stored data corrupt, not a Storable binary image, or byte order is not compatible/",
"byte order");
$where = $file_magic + 3 + length $header->{byteorder};
foreach (['intsize', "Integer"],
@@ -235,7 +235,7 @@ sub test_things {
# local $Storable::DEBUGME = 1;
local $Storable::accept_future_minor = 0;
test_corrupt ($copy, $sub,
- "/^Storable binary image v$header->{major}\.$minor6 more recent than I am \\(v$header->{major}\.$minor\\)/",
+ "/^Stored data corrupt, not a Storable binary image, or binary image version v$header->{major}\.$minor6 more recent than I am \\(v$header->{major}\.$minor\\)/",
"higher minor");
}
}
--
2.8.1
|
From @jkeenanOn Wed Jul 06 20:51:35 2016, dcollinsn@gmail.com wrote:
If you modify a *.xs file in a distribution, you also need to increment $VERSION in the corresponding *.pm file. See second patch attached. -- |
From @jkeenan30807-0002-Increment-Storable-VERSION-to-2.57.patchFrom 5f7fabc05a5e4119cc07f1b4eb592ac8f7a99d5d Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Thu, 7 Jul 2016 08:19:58 -0400
Subject: [PATCH] Increment Storable $VERSION to 2.57.
For RT #30807; addition to patch submitted by dcollins.
---
dist/Storable/Storable.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dist/Storable/Storable.pm b/dist/Storable/Storable.pm
index c8f6db1..e928401 100644
--- a/dist/Storable/Storable.pm
+++ b/dist/Storable/Storable.pm
@@ -22,7 +22,7 @@ package Storable; @ISA = qw(Exporter);
use vars qw($canonical $forgive_me $VERSION);
-$VERSION = '2.56';
+$VERSION = '2.57';
BEGIN {
if (eval { local $SIG{__DIE__}; require Log::Agent; 1 }) {
--
1.9.1
|
@DCOLLINS back in 2016 you proposed changes to Storable's error messages (see 1st patch in this gh-issue). Do you wish to pursue these? Thank you very much. |
Migrated from rt.perl.org#30807 (status was 'open')
Searchable as RT30807$
The text was updated successfully, but these errors were encountered: