Bug 938300: vers_cmp() incorrectly compares module versions

r=sgreen a=justdave

6 files changed, 113 insertions, 89 deletions

diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm
index f5320cb7f..063e2cf69 100644
--- a/Bugzilla/DB.pm
+++ b/Bugzilla/DB.pm
@@ -17,11 +17,12 @@ use parent -norequire, qw(DBI::db);
 
 use Bugzilla::Constants;
 use Bugzilla::Install::Requirements;
-use Bugzilla::Install::Util qw(vers_cmp install_string);
+use Bugzilla::Install::Util qw(install_string);
 use Bugzilla::Install::Localconfig;
 use Bugzilla::Util;
 use Bugzilla::Error;
 use Bugzilla::DB::Schema;
+use Bugzilla::Version;
 
 use List::Util qw(max);
 use Storable qw(dclone);
diff --git a/Bugzilla/Install/Requirements.pm b/Bugzilla/Install/Requirements.pm
index 06f855fc5..09ea4abe3 100644
--- a/Bugzilla/Install/Requirements.pm
+++ b/Bugzilla/Install/Requirements.pm
@@ -17,7 +17,7 @@ use 5.10.1;
 use strict;
 
 use Bugzilla::Constants;
-use Bugzilla::Install::Util qw(vers_cmp install_string bin_loc 
+use Bugzilla::Install::Util qw(install_string bin_loc
                                extension_requirement_packages);
 use List::Util qw(max);
 use Term::ANSIColor;
@@ -86,7 +86,6 @@ use constant APACHE_PATH => [qw(
 # are 'blacklisted'--that is, even if the version is high enough, Bugzilla
 # will refuse to say that it's OK to run with that version.
 sub REQUIRED_MODULES {
-    my $perl_ver = sprintf('%vd', $^V);
     my @modules = (
     {
         package => 'CGI.pm',
@@ -125,7 +124,7 @@ sub REQUIRED_MODULES {
     {
         package => 'DBI',
         module  => 'DBI',
-        version => (vers_cmp($perl_ver, '5.13.3') > -1) ? '1.614' : '1.54'
+        version => ($^V >= v5.13.3) ? '1.614' : '1.54'
     },
     # 2.24 contains several useful text virtual methods.
     {
@@ -187,7 +186,6 @@ sub REQUIRED_MODULES {
 };
 
 sub OPTIONAL_MODULES {
-    my $perl_ver = sprintf('%vd', $^V);
     my @modules = (
     {
         package => 'GD',
@@ -198,8 +196,9 @@ sub OPTIONAL_MODULES {
     {
         package => 'Chart',
         module  => 'Chart::Lines',
-        # Versions below 2.1 cannot be detected accurately.
-        version => '2.1',
+        # Versions below 2.4.1 cannot be compared accurately, see
+        # https://rt.cpan.org/Public/Bug/Display.html?id=28218.
+        version => '2.4.1',
         feature => [qw(new_charts old_charts)],
     },
     {
@@ -314,7 +313,7 @@ sub OPTIONAL_MODULES {
         # We need the 'utf8_mode' method of HTML::Parser, for HTML::Scrubber.
         package => 'HTML-Parser',
         module  => 'HTML::Parser',
-        version => (vers_cmp($perl_ver, '5.13.3') > -1) ? '3.67' : '3.40',
+        version => ($^V >= v5.13.3) ? '3.67' : '3.40',
         feature => ['html_desc'],
     },
     {
@@ -668,8 +667,8 @@ sub check_graphviz {
     return $return;
 }
 
-# This was originally clipped from the libnet Makefile.PL, adapted here to
-# use the below vers_cmp routine for accurate version checking.
+# This was originally clipped from the libnet Makefile.PL, adapted here for
+# accurate version checking.
 sub have_vers {
     my ($params, $output) = @_;
     my $module  = $params->{module};
@@ -694,15 +693,17 @@ sub have_vers {
     if ($@) {
         no strict 'refs';
         $vnum = ${"${module}::VERSION"};
-    }
-    $vnum ||= -1;
 
-    # Fix CPAN versions like 1.9304.
-    if ($module eq 'CPAN' and $vnum =~ /^(\d\.\d{2})\d{2}$/) {
-        $vnum = $1;
+        # If we come here, then the version is not a valid one.
+        # We try to sanitize it.
+        if ($vnum =~ /^((\d+)(\.\d+)*)/) {
+            $vnum = $1;
+        }
     }
+    $vnum ||= -1;
 
-    my $vok = (vers_cmp($vnum,$wanted) > -1);
+    # Must do a string comparison as $vnum may be of the form 5.10.1.
+    my $vok = ($vnum ne '-1' && version->new($vnum) >= version->new($wanted)) ? 1 : 0;
     my $blacklisted;
     if ($vok && $params->{blacklist}) {
         $blacklisted = grep($vnum =~ /$_/, @{$params->{blacklist}});
diff --git a/Bugzilla/Install/Util.pm b/Bugzilla/Install/Util.pm
index 53cc9d2ec..66ea7169e 100644
--- a/Bugzilla/Install/Util.pm
+++ b/Bugzilla/Install/Util.pm
@@ -38,7 +38,6 @@ our @EXPORT_OK = qw(
     include_languages
     success
     template_include_path
-    vers_cmp
     init_console
 );
 
@@ -476,49 +475,6 @@ sub template_include_path {
     return \@include_path;
 }
 
-# This is taken straight from Sort::Versions 1.5, which is not included
-# with perl by default.
-sub vers_cmp {
-    my ($a, $b) = @_;
-
-    # Remove leading zeroes - Bug 344661
-    $a =~ s/^0*(\d.+)/$1/;
-    $b =~ s/^0*(\d.+)/$1/;
-
-    my @A = ($a =~ /([-.]|\d+|[^-.\d]+)/g);
-    my @B = ($b =~ /([-.]|\d+|[^-.\d]+)/g);
-
-    my ($A, $B);
-    while (@A and @B) {
-        $A = shift @A;
-        $B = shift @B;
-        if ($A eq '-' and $B eq '-') {
-            next;
-        } elsif ( $A eq '-' ) {
-            return -1;
-        } elsif ( $B eq '-') {
-            return 1;
-        } elsif ($A eq '.' and $B eq '.') {
-            next;
-        } elsif ( $A eq '.' ) {
-            return -1;
-        } elsif ( $B eq '.' ) {
-            return 1;
-        } elsif ($A =~ /^\d+$/ and $B =~ /^\d+$/) {
-            if ($A =~ /^0/ || $B =~ /^0/) {
-                return $A cmp $B if $A cmp $B;
-            } else {
-                return $A <=> $B if $A <=> $B;
-            }
-        } else {
-            $A = uc $A;
-            $B = uc $B;
-            return $A cmp $B if $A cmp $B;
-        }
-    }
-    @A <=> @B;
-}
-
 sub no_checksetup_from_cgi {
     print "Content-Type: text/html; charset=UTF-8\r\n\r\n";
     print install_string('no_checksetup_from_cgi');
@@ -894,28 +850,6 @@ Used by L<Bugzilla::Template> to determine the languages' list which
 are compiled with the browser's I<Accept-Language> and the languages 
 of installed templates.
 
-=item C<vers_cmp>
-
-=over
-
-=item B<Description>
-
-This is a comparison function, like you would use in C<sort>, except that
-it compares two version numbers. So, for example, 2.10 would be greater
-than 2.2.
-
-It's based on versioncmp from L<Sort::Versions>, with some Bugzilla-specific
-fixes.
-
-=item B<Params>: C<$a> and C<$b> - The versions you want to compare.
-
-=item B<Returns>
-
-C<-1> if C<$a> is less than C<$b>, C<0> if they are equal, or C<1> if C<$a>
-is greater than C<$b>.
-
-=back
-
 =back
 
 =head1 B<Methods in need of POD>
diff --git a/Bugzilla/Version.pm b/Bugzilla/Version.pm
index 1dcd2b141..c6b178a8a 100644
--- a/Bugzilla/Version.pm
+++ b/Bugzilla/Version.pm
@@ -10,9 +10,10 @@ package Bugzilla::Version;
 use 5.10.1;
 use strict;
 
-use parent qw(Bugzilla::Object);
+use parent qw(Bugzilla::Object Exporter);
+
+@Bugzilla::Version::EXPORT = qw(vers_cmp);
 
-use Bugzilla::Install::Util qw(vers_cmp);
 use Bugzilla::Util;
 use Bugzilla::Error;
 
@@ -200,6 +201,53 @@ sub _check_product {
     return Bugzilla->user->check_can_admin_product($product->name);
 }
 
+###############################
+#####     Functions        ####
+###############################
+
+# This is taken straight from Sort::Versions 1.5, which is not included
+# with perl by default.
+sub vers_cmp {
+    my ($a, $b) = @_;
+
+    # Remove leading zeroes - Bug 344661
+    $a =~ s/^0*(\d.+)/$1/;
+    $b =~ s/^0*(\d.+)/$1/;
+
+    my @A = ($a =~ /([-.]|\d+|[^-.\d]+)/g);
+    my @B = ($b =~ /([-.]|\d+|[^-.\d]+)/g);
+
+    my ($A, $B);
+    while (@A and @B) {
+        $A = shift @A;
+        $B = shift @B;
+        if ($A eq '-' and $B eq '-') {
+            next;
+        } elsif ( $A eq '-' ) {
+            return -1;
+        } elsif ( $B eq '-') {
+            return 1;
+        } elsif ($A eq '.' and $B eq '.') {
+            next;
+        } elsif ( $A eq '.' ) {
+            return -1;
+        } elsif ( $B eq '.' ) {
+            return 1;
+        } elsif ($A =~ /^\d+$/ and $B =~ /^\d+$/) {
+            if ($A =~ /^0/ || $B =~ /^0/) {
+                return $A cmp $B if $A cmp $B;
+            } else {
+                return $A <=> $B if $A <=> $B;
+            }
+        } else {
+            $A = uc $A;
+            $B = uc $B;
+            return $A cmp $B if $A cmp $B;
+        }
+    }
+    return @A <=> @B;
+}
+
 1;
 
 __END__
@@ -243,11 +291,51 @@ below.
 
 =item C<bug_count()>
 
- Description: Returns the total of bugs that belong to the version.
+=over
+
+=item B<Description>
+
+Returns the total of bugs that belong to the version.
+
+=item B<Params>
+
+none
+
+=item B<Returns>
+
+Integer with the number of bugs.
+
+=back
+
+=back
+
+=head1 FUNCTIONS
+
+=over
+
+=item C<vers_cmp($a, $b)>
+
+=over
+
+=item B<Description>
+
+This is a comparison function, like you would use in C<sort>, except that
+it compares two version numbers. So, for example, 2.10 would be greater
+than 2.2.
+
+It's based on versioncmp from L<Sort::Versions>, with some Bugzilla-specific
+fixes.
 
- Params:      none.
+=item B<Params>
 
- Returns:     Integer with the number of bugs.
+C<$a> and C<$b> - The versions you want to compare.
+
+=item B<Returns>
+
+C<-1> if C<$a> is less than C<$b>, C<0> if they are equal, or C<1> if C<$a>
+is greater than C<$b>.
+
+=back
 
 =back
 
diff --git a/install-module.pl b/install-module.pl
index 37ea8cc41..a7359917d 100755
--- a/install-module.pl
+++ b/install-module.pl
@@ -23,7 +23,7 @@ use Bugzilla::Install::CPAN;
 
 use Bugzilla::Constants;
 use Bugzilla::Install::Requirements;
-use Bugzilla::Install::Util qw(bin_loc init_console vers_cmp);
+use Bugzilla::Install::Util qw(bin_loc init_console);
 
 use Data::Dumper;
 use Getopt::Long;
diff --git a/query.cgi b/query.cgi
index 0e921ac0c..65a7d3c3d 100755
--- a/query.cgi
+++ b/query.cgi
@@ -18,9 +18,9 @@ use Bugzilla::User;
 use Bugzilla::Util;
 use Bugzilla::Error;
 use Bugzilla::Product;
+use Bugzilla::Version;
 use Bugzilla::Keyword;
 use Bugzilla::Field;
-use Bugzilla::Install::Util qw(vers_cmp);
 use Bugzilla::Token;
 
 ###############