301392 - Storable crashes checksetup with byte order error when moving databases

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect, P2)

Description

[A. Karl Kornel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax)

I recently completed a move of Bugzilla from a PowerPC machine running Darwin to
an i686 machine running Debian Linux.  The linux box ran a 64-bit-aware copy of
Perl.  When running checksetup, Storeable complained about incompatible byte
ordering at the end of the script.  I had to delete the contents of the
bz_schema table and run checksetup a few more times to make the error message go
away.

Reproducible: Always

Steps to Reproduce:
1. Install & Configure Bugzilla on a machine running Darwin / Mac OS X, then use
it for a while.
2. Dump the database to a .sql file.
3. Compile Perl 5.8.7, with 64-bit support, on a machine running Debian Linux
(on an i686 box).
4. Copy the bugzilla directory from (1) to the Debian machine.  Modify the
shebang lines of all files to refer to the copy of Perl built in step 3.
5. Create a new database on the Debian machine, and load the contents of the
.sql file from step 2.
6. Run checksetup.pl.
Actual Results:  
Checksetup failed with the error "Byte order is not compatible at
blib/lib/Storable.pm (autosplit into blib/lib/auto/Storable/thaw.al) line 366,
at Bugzilla/DB/Schema.pm line 1987"

Expected Results:  
Checksetup should not have produced any Storable-related errors of any kind.

I found some useful information in paragraph 5 of
http://perl.active-venture.com/lib/Storable.html#64-bit-data-in-perl-5.6.0-and-5.6.1
that might explain the problem.

I moved the Bugzilla database from a machine running Darwin/MacOSX to a machine
running Debian Linux (of some unknown version).

The Perl on Debian is 5.8.7 and does have 64-bit support (it's not the standard
version that comes with Debian).  I don't know about 64-bitness of the the Perl
on Darwin/MacOSX), though I can say that it's version 5.8.6.

I did manage to eliminate the error message by deleting everything in the
bz_schema table and running checksetup.pl two more times.  From that point on
there were no problems.

Comment 1

[Max Kanat-Alexander]

Strange, that shouldn't happen if both of your copies of Storable were from perl
5.8; I was under the impression that perl 5.8 handled that fine. It's true that
you can't go from perl 5.8 to perl 5.6 using Storable, but any perl 5.8 to 5.8
should be OK... seems like perhaps a Storable bug...?

Comment 2

[Joel Peshkin]

This problem also comes up with x86/Linux <-> SPARC/Solaris interworking.  The
schema should use Data::Dumper for storage.

Comment 3

[u197037]

(In reply to comment #2)
> This problem also comes up with x86/Linux <-> SPARC/Solaris interworking.  The
> schema should use Data::Dumper for storage.
> 

The problem is with stored binary bz_schema being unpacked on different CPU
architectures.
Module should use Data::Dumper to serialize/deserialize it.

To reproduce, just install DB on Sparc/PPC host and try to run checksetup.pl
from it. Then run checksetup from linux(or other x86) host with the same DB.
I've got:
checksetup.pl: Byte order is not compatible at ../../lib/Storable.pm (autosplit
into ../../lib/auto/Storable/thaw.al) line 363, at Bugzilla/DB/Schema.pm line 2062

Comment 4

[Joel Peshkin]

Actually... while we are at it.  We may as well have Data::Dumper use a
human-friendly format rather than its most compact format.

Comment 5

[u197037]

Attachment: Proposed initial patch

Max,
this patch is an outline of Joel idea for platform-independent chached DB
schema.
Please followup with your comments.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 195158 [details] [diff] [review]
Proposed initial patch

Looks basically good. I was concerned that eventually we'd have to do this. :-)

A few comments:

1) Add "local" to those $Data variables, and I'm pretty sure they should be
$Data::Dumper.

2) We should eval the Schema inside a Safe container, like we do in
Bugzilla::Config. (That way, if somebody manages to write to the DB, it doesn't
allow them to execute arbitrary code.)

Comment 7

[Max Kanat-Alexander]

Oh, and we'll also need to change the bz_schema column to a TEXT column rather
than a BLOB. So we'll have to do the conversion code at the same place we do
that... probably somewhere in bz_setup_database instead of where it is now.

Comment 8

[u197037]

Attachment: "local"ized $Data vars, dumped schema evaluated in Safe's sandbox

Max, please check if I properly used local - I'm not sure of how it exactly
work.
Also, usage of Safe does not save us from signals (FPE,etc) and infinite loops
if someone modify DB field.
See http://search.cpan.org/~rgarcia/Safe-2.11/Safe.pm#Some_Safety_Issues

Comment 9

[u197037]

Comment on attachment 195259 [details] [diff] [review]
"local"ized $Data vars, dumped schema evaluated in Safe's sandbox

Sorry, wrong patch.

Comment 10

[u197037]

Attachment: The right version

Previous comment still actual

Comment 11

[u197037]

(In reply to comment #7)
> Oh, and we'll also need to change the bz_schema column to a TEXT column rather
> than a BLOB. So we'll have to do the conversion code at the same place we do
> that... probably somewhere in bz_setup_database instead of where it is now.

Max, this part is a bit out of my knowledge level and I'm currently a bit busy
to investigate that.
Could you or somebody else take it over?
OTOH, attached patch is just happy with BLOB. Is it really needed to alter
column definition? If we leave it as it is now, the serializer/deserializer code
will be able to switch versions smoothly and transparent.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 195263 [details] [diff] [review]
The right version

>+    
>+    # Make it ok to eval
>+    local $Data::Dumper::Purity = 1;
>+    
>+    # avoid cross-refs
>+    local $Data::Dumper::Deepcopy = 1;

  Yes, you used local correctly. :-) Also add $Data::Dumper::Sortkeys = 1 so
that our Schema is always stored the same way. That will make it easier to
write comparison tools that compare just the text.

>+        # At this point, we have no backwards-compatibility
>+        # code to write, so $version is ignored.
>+        # For what $version ought to be used for, see the
>+        # "private" section of the SCHEMA_VERSION docs.

  You can just remove this comment.

>+        my $cpt = new Safe;
>+        
>+        $thawed_hash = $cpt->reval($serialized) ||
>+            die "Unable to restore cached schema: " . $@;

  Does that actually work? The $VAR1 of Data::Dumper will be in The namespace
Safe::Root0. What I'd rather do is reval the string, and then use
$cpt->varglob('VAR1') to get it out.

Comment 13

[u197037]

Attachment: Updated patch according to comment #12

Updated patch to address comment 12 issues (Sortkeys, comment and the way to
access stored $VAR1)

Comment 14

[Max Kanat-Alexander]

Comment on attachment 196197 [details] [diff] [review]
Updated patch according to comment #12

Great. :-) Looks right, works on landfill

Comment 15

[Max Kanat-Alexander]

Note that Dennis's comment adding himself to Schema.pm had a conflict, and needs
to be manually inserted when applying the patch on the tip.

Comment 16

[Frédéric Buclin]

tip:

Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.39; previous revision: 1.38
done

2.20rc2:

Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.32.2.4; previous revision: 1.32.2.3
done