Merged fix-917770-pt-config-diff-uninit-value-crash

bin/pt-config-diff

@@ -2145,15 +2145,16 @@ sub parse_mysqld {
       PTDEBUG && _d("mysqld help output doesn't list option files");
    }
 
-   if ( $output !~ m/^-+ -+$/mg ) {
+   if ( $output !~ m/^-+ -+$(.+?)(?:\n\n.+)?\z/sm ) {
       PTDEBUG && _d("mysqld help output doesn't list vars and vals");
       return;
    }
 
-   my $varvals = substr($output, (pos $output) + 1, length $output);
+   my $varvals = $1;
 
    my ($config, undef) = _parse_varvals(
-      $varvals =~ m/\G^(\S+)(.*)\n/mg
+      qr/^(\S+)(.*)$/,
+      $varvals,
    );
 
    return $config, \@opt_files;
@@ -2168,7 +2169,8 @@ sub parse_my_print_defaults {
    my ($output) = @args{@required_args};
 
    my ($config, $dupes) = _parse_varvals(
-      map { $_ =~ m/^--([^=]+)(?:=(.*))?$/ } split("\n", $output)
+      qr/^--([^=]+)(?:=(.*))?$/,
+      $output,
    );
 
    return $config, $dupes;
@@ -2186,49 +2188,72 @@ sub parse_option_file {
    die "Failed to parse the [mysqld] section" unless $mysqld_section;
 
    my ($config, $dupes) = _parse_varvals(
-      map  { $_ =~ m/^([^=]+)(?:=(.*))?$/ }
+      qr/^([^=]+)(?:=(.*))?$/,
-      grep { $_ !~ m/^\s*#/ }  # no # comment lines
+      $mysqld_section,
-      split("\n", $mysqld_section)
    );
 
    return $config, $dupes;
 }
 
-sub _parse_varvals {
+sub _preprocess_varvals {
-   my ( @varvals ) = @_;
+   my ($re, $to_parse) = @_;
 
-   my %config;
+   my %vars;
+   LINE:
+   foreach my $line ( split /\n/, $to_parse ) {
+      next LINE if $line =~ m/^\s*$/;  # no empty lines
+      next LINE if $line =~ m/^\s*#/;  # no # comment lines
 
-   my $duplicate_var = 0;
+      if ( $line !~ $re ) {
-   my %duplicates;
+         PTDEBUG && _d("Line <", $line, "> didn't match $re");
-
+         next LINE;
-   my $var;  # current variable (e.g. datadir)
-   my $val;  # value for current variable
-   ITEM:
-   foreach my $item ( @varvals ) {
-      if ( $item ) {
-         $item =~ s/^\s+//;
-         $item =~ s/\s+$//;
       }
 
-      if ( !$var ) {
+      my ($var, $val) = ($1, $2);
-         $var = $item;
       
-         $var =~ s/-/_/g;
+      $var =~ tr/-/_/;
-
-         if ( exists $config{$var} && !$can_be_duplicate{$var} ) {
-            PTDEBUG && _d("Duplicate var:", $var);
-            $duplicate_var = 1;  # flag on, save all the var's values
-         }
-      }
-      else {
-         my $val = $item;
-         PTDEBUG && _d("Var:", $var, "val:", $val);
 
       if ( !defined $val ) {
          $val = '';
       }
-         else {
+      
+      for my $item ($var, $val) {
+         $item =~ s/^\s+//;
+         $item =~ s/\s+$//;
+      }
+
+      push @{$vars{$var} ||= []}, $val
+   }
+
+   return \%vars;
+}
+
+sub _parse_varvals {
+   my ( $vars ) = _preprocess_varvals(@_);
+
+   my %config;
+
+   my %duplicates;
+
+   while ( my ($var, $vals) = each %$vars ) {
+      my $val = _process_val( pop @$vals );
+      if ( @$vals && !$can_be_duplicate{$var} ) {
+         PTDEBUG && _d("Duplicate var:", $var);
+         foreach my $current_val ( map { _process_val($_) } @$vals ) {
+            push @{$duplicates{$var} ||= []}, $current_val;
+         }
+      }
+
+      PTDEBUG && _d("Var:", $var, "val:", $val);
+
+      $config{$var} = $val;
+   }
+
+   return \%config, \%duplicates;
+}
+
+sub _process_val {
+   my ($val) = @_;
    $val =~ s/
                \A        # Start of value
                (['"])    # Opening quote
@@ -2248,20 +2273,7 @@ sub _parse_varvals {
    elsif ( $val =~ m/No default/ ) {
       $val = '';
    }
-         }
+   return $val;
-
-         if ( $duplicate_var ) {
-            push @{$duplicates{$var}}, $config{$var};
-            $duplicate_var = 0;  # flag off for next var
-         }
-
-         $config{$var} = $val;
-
-         $var = undef;  # next item should be a var
-      }
-   }
-
-   return \%config, \%duplicates;
 }
 
 sub _mimic_show_variables {

lib/MySQLConfig.pm

@@ -259,18 +259,28 @@ sub parse_mysqld {
    #   help                              TRUE
    #   abort-slave-event-count           0
    # So we search for that line of hypens.
-   if ( $output !~ m/^-+ -+$/mg ) {
+   #
+   # It also ends with something like
+   #
+   #   wait_timeout                      28800
+   #   
+   #   To see what values a running MySQL server is using, type
+   #   'mysqladmin variables' instead of 'mysqld --verbose --help'.
+   #
+   # So try to find it by locating a double newline, and strip it away
+   if ( $output !~ m/^-+ -+$(.+?)(?:\n\n.+)?\z/sm ) {
       PTDEBUG && _d("mysqld help output doesn't list vars and vals");
       return;
    }
 
-   # Cut off everything before the list of vars and vals.
+   # Grab the varval list.
-   my $varvals = substr($output, (pos $output) + 1, length $output);
+   my $varvals = $1;
 
    # Parse the "var  val" lines.  2nd retval is duplicates but there
    # shouldn't be any with mysqld.
    my ($config, undef) = _parse_varvals(
-      $varvals =~ m/\G^(\S+)(.*)\n/mg
+      qr/^(\S+)(.*)$/,
+      $varvals,
    );
 
    return $config, \@opt_files;
@@ -288,7 +298,8 @@ sub parse_my_print_defaults {
 
    # Parse the "--var=val" lines.
    my ($config, $dupes) = _parse_varvals(
-      map { $_ =~ m/^--([^=]+)(?:=(.*))?$/ } split("\n", $output)
+      qr/^--([^=]+)(?:=(.*))?$/,
+      $output,
    );
 
    return $config, $dupes;
@@ -309,68 +320,93 @@ sub parse_option_file {
 
    # Parse the "var=val" lines.
    my ($config, $dupes) = _parse_varvals(
-      map  { $_ =~ m/^([^=]+)(?:=(.*))?$/ }
+      qr/^([^=]+)(?:=(.*))?$/,
-      grep { $_ !~ m/^\s*#/ }  # no # comment lines
+      $mysqld_section,
-      split("\n", $mysqld_section)
    );
 
    return $config, $dupes;
 }
 
-# Parses a list of variables and their values ("varvals"), returns two
+# Called by _parse_varvals(), takes two arguments: a regex, and
+# a string to match against. The string will be split in lines,
+# and each line will be matched against the regex.
+# The regex must return to captures, although the second doesn't
+# have to match anything.
+# Returns a hashref of arrayrefs ala
+# { port => [ 12345, 12346 ], key_buffer_size => [ "16M" ] }
+sub _preprocess_varvals {
+   my ($re, $to_parse) = @_;
+
+   my %vars;
+   LINE:
+   foreach my $line ( split /\n/, $to_parse ) {
+      next LINE if $line =~ m/^\s*$/;  # no empty lines
+      next LINE if $line =~ m/^\s*#/;  # no # comment lines
+
+      if ( $line !~ $re ) {
+         PTDEBUG && _d("Line <", $line, "> didn't match $re");
+         next LINE;
+      }
+
+      my ($var, $val) = ($1, $2);
+      
+      # Variable names are usually specified like "log-bin"
+      # but in SHOW VARIABLES they're all like "log_bin".
+      $var =~ tr/-/_/;
+
+      if ( !defined $val ) {
+         $val = '';
+      }
+      
+      # Strip leading and trailing whitespace.
+      for my $item ($var, $val) {
+         $item =~ s/^\s+//;
+         $item =~ s/\s+$//;
+      }
+
+      push @{$vars{$var} ||= []}, $val
+   }
+
+   return \%vars;
+}
+
+# Parses a string of variables and their values ("varvals"), returns two
 # hashrefs: one with normalized variable=>value, the other with duplicate
-# vars.  The varvals list should start with a var at index 0 and its value
+# vars.
-# at index 1 then repeat for the next var-val pair.  
 sub _parse_varvals {
-   my ( @varvals ) = @_;
+   my ( $vars ) = _preprocess_varvals(@_);
 
    # Config built from parsing the given varvals.
    my %config;
 
    # Discover duplicate vars.  
-   my $duplicate_var = 0;
    my %duplicates;
 
-   # Keep track if item is var or val because each needs special modifications.
+   while ( my ($var, $vals) = each %$vars ) {
-   my $var;  # current variable (e.g. datadir)
+      my $val = _process_val( pop @$vals );
-   my $val;  # value for current variable
+      # If the variable has duplicates, then @$vals will contain
-   ITEM:
+      # the rest of the values
-   foreach my $item ( @varvals ) {
+      if ( @$vals && !$can_be_duplicate{$var} ) {
-      if ( $item ) {
-         # Strip leading and trailing whitespace.
-         $item =~ s/^\s+//;
-         $item =~ s/\s+$//;
-      }
-
-      if ( !$var ) {
-         # No var means this item is (should be) the next var in the list.
-         $var = $item;
-
-         # Variable names are usually specified like "log-bin"
-         # but in SHOW VARIABLES they're all like "log_bin".
-         $var =~ s/-/_/g;
-
          # The var is a duplicate (in the bad sense, i.e. where user is
          # probably unaware that there's two different values for this var
-         # but only the last is used) if we've seen it already and it cannot
+         # but only the last is used).
-         # be duplicated.  We don't have its value yet (next loop iter),
-         # so we set a flag to indicate that we should save the duplicate value.
-         if ( exists $config{$var} && !$can_be_duplicate{$var} ) {
          PTDEBUG && _d("Duplicate var:", $var);
-            $duplicate_var = 1;  # flag on, save all the var's values
+         foreach my $current_val ( map { _process_val($_) } @$vals ) {
+            push @{$duplicates{$var} ||= []}, $current_val;
          }
       }
-      else {
+
-         # $var is set so this item should be its value.
-         my $val = $item;
       PTDEBUG && _d("Var:", $var, "val:", $val);
 
-         # Avoid crashing on undef comparison.  Also, SHOW VARIABLES uses
+      # Save this var-val.
-         # blank strings, not NULL/undef.
+      $config{$var} = $val;
-         if ( !defined $val ) {
-            $val = '';
    }
-         else {
+
+   return \%config, \%duplicates;
+}
+
+sub _process_val {
+   my ($val) = @_;
    $val =~ s/
                \A        # Start of value
                (['"])    # Opening quote
@@ -391,23 +427,7 @@ sub _parse_varvals {
    elsif ( $val =~ m/No default/ ) {
       $val = '';
    }
-         }
+   return $val;
-
-         if ( $duplicate_var ) {
-            # Save the var's last value before we overwrite it with this
-            # current value.
-            push @{$duplicates{$var}}, $config{$var};
-            $duplicate_var = 0;  # flag off for next var
-         }
-
-         # Save this var-val.
-         $config{$var} = $val;
-
-         $var = undef;  # next item should be a var
-      }
-   }
-
-   return \%config, \%duplicates;
 }
 
 # Sub: _mimic_show_variables

t/lib/MySQLConfig.t

@@ -9,7 +9,7 @@ BEGIN {
 use strict;
 use warnings FATAL => 'all';
 use English qw(-no_match_vars);
-use Test::More tests => 30;
+use Test::More;
 
 use MySQLConfig;
 use DSNParser;
@@ -827,6 +827,30 @@ SKIP: {
    );
 }
 
+# #############################################################################
+# Use of uninitialized value in substitution (s///) at pt-config-diff line 1996
+# https://bugs.launchpad.net/percona-toolkit/+bug/917770
+# #############################################################################
+
+$config = eval {
+   new MySQLConfig(
+      file                => "$trunk/t/pt-config-diff/samples/bug_917770.cnf",
+      TextResultSetParser => $trp,
+   );
+};
+
+is(
+   $EVAL_ERROR,
+   '',
+   "Bug 917770: Lives ok on lines with just spaces"
+);
+
+is(
+   $config->format(),
+   'option_file',
+   "Detect option_file type"
+);
+
 # #############################################################################
 # Done.
 # #############################################################################
@@ -841,4 +865,5 @@ like(
    '_d() works'
 );
 ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox");
+done_testing;
 exit;

t/pt-config-diff/samples/bug_917770.cnf

@@ -0,0 +1,31 @@
+[client]
+user     = msandbox
+password = msandbox
+port     = PORT
+socket   = /tmp/PORT/mysql_sandboxPORT.sock
+
+[mysqld]
+port                       = PORTa
+socket                     = /tmp/PORT/mysql_sandboxPORT.sock
+pid-file                   = /tmp/PORT/data/mysql_sandboxPORT.pid
+basedir                    = PERCONA_TOOLKIT_SANDBOX
+datadir                    = /tmp/PORT/data
+key_buffer_size            = 16M
+innodb_buffer_pool_size    = 16M
+innodb_data_home_dir       = /tmp/PORT/data
+innodb_log_group_home_dir  = /tmp/PORT/data
+innodb_data_file_path      = ibdata1:10M:autoextend
+innodb_log_file_size       = 5M
+log-bin                    = mysql-bin
+relay_log                  = mysql-relay-bin
+log_slave_updates
+server-id                  = PORT
+report-host                = 127.0.0.1
+report-port                = PORT
+log-error                  = /tmp/PORT/data/mysqld.log
+innodb_lock_wait_timeout   = 3
+general_log
+general_log_file           = genlog
+# These next two lines have a space in them, and trigger the bug
+ 
+