Merge lp:~percona-toolkit-dev/percona-toolkit/fix-917770-pt-config-diff-uninit-value-crash into lp:percona-toolkit/2.1

[Brian Fraser]

[Daniel Nichter (daniel-nichter)]

We need a reproducible test case. I wrote that regex and I know that if a group fails to match the corresponding $N will be undef not empty string, so pt-config-diff should be resilient to undef values.

review: Needs Resubmitting

[Brian Fraser (fraserbn)]

New fix deals with the actual root cause, and adds two safety checks against undefs. Plus a test case : D

[Daniel Nichter (daniel-nichter)]

64 + # We were passed an undef for the value, or we're dealing with an empty
65 + # line
66 + if ( !defined($item) ) {
67 + $item = '';
68 + }
69 if ( $item ) {

Are the new lines necessary? Line 69 will be false if $item is either undef or '', so setting it to '' doesn't seem to do anything except introduce the possibility of no-name items. I think this is better handled around

+ map { $_ =~ m/^--([^=]+)(?:=(.*))?$/ ? ($1, $2) : () }

and its non-"--" variant in another call. Granted, those split/grep/map are a poor place: in the actual sub call. So perhaps it should be refactored into sub which can match for the item=val and if the item part is undef then warn about the failure to parse that line and skip it. Doing that, however, will mean that we also need to skip section headers like "[client]" in addition to comment and blank lines.

Maybe the refactored sub can take a pattern arg, m/^--([^=]+)(?:=(.*))?$/ or m/^([^=]+)(?:=(.*))?$/, because skipping blank, header, and section lines is common to both inputs. If the code to handle EOL comments is ever implemented, it could be put in this sub, too.

[lp:~percona-toolkit-dev/percona-toolkit/fix-917770-pt-config-diff-uninit-value-crash]

469. By Brian Fraser on 2012-12-04

Refactor _parse_varvals.

Now it takes two arguments: A regexp and a string to match against.
_parse_varvals itself was split in three: _preprocess_varvals,
_parse_varvals, and _process_val.

This also modifies the three places that call _parse_varvals; For
two, no real changes were needed, but parse_mysqld() required a fix
to deal with the two final lines of mysqld --help --verbose:

   To see what values a running MySQL server is using, type
   'mysqladmin variables' instead of 'mysqld --verbose --help'.

[Daniel Nichter (daniel-nichter)]

Looks good. The refactored code is much saner.

review: Approve

[Brian Fraser (fraserbn)]

review: Approve

=== modified file 'bin/pt-config-diff'
--- bin/pt-config-diff	2012-11-19 18:47:13 +0000
+++ bin/pt-config-diff	2012-12-04 08:13:22 +0000
@@ -2145,15 +2145,16 @@
       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 @@
    my ($output) = @args{@required_args};
 
    my ($config, $dupes) = _parse_varvals(
-      map { $_ =~ m/^--([^=]+)(?:=(.*))?$/ } split("\n", $output)
+      qr/^--([^=]+)(?:=(.*))?$/,
+      $output,
    );
 
    return $config, $dupes;
@@ -2186,84 +2188,94 @@
    die "Failed to parse the [mysqld] section" unless $mysqld_section;
 
    my ($config, $dupes) = _parse_varvals(
-      map  { $_ =~ m/^([^=]+)(?:=(.*))?$/ }
-      grep { $_ !~ m/^\s*#/ }  # no # comment lines
-      split("\n", $mysqld_section)
+      qr/^([^=]+)(?:=(.*))?$/,
+      $mysqld_section,
    );
 
    return $config, $dupes;
 }
 
+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);
+      
+      $var =~ tr/-/_/;
+
+      if ( !defined $val ) {
+         $val = '';
+      }
+      
+      for my $item ($var, $val) {
+         $item =~ s/^\s+//;
+         $item =~ s/\s+$//;
+      }
+
+      push @{$vars{$var} ||= []}, $val
+   }
+
+   return \%vars;
+}
+
 sub _parse_varvals {
-   my ( @varvals ) = @_;
+   my ( $vars ) = _preprocess_varvals(@_);
 
    my %config;
 
-   my $duplicate_var = 0;
    my %duplicates;
 
-   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 ) {
-         $var = $item;
-
-         $var =~ s/-/_/g;
-
-         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 {
-            $val =~ s/
-                        \A        # Start of value
-                        (['"])    # Opening quote
-                        (.*)      # Value
-                        \1        # Closing quote
-                        [\n\r]*\z # End of value
-                    /$2/x;
-            if ( my ($num, $factor) = $val =~ m/(\d+)([KMGT])b?$/i ) {
-               my %factor_for = (
-                  k => 1_024,
-                  m => 1_048_576,
-                  g => 1_073_741_824,
-                  t => 1_099_511_627_776,
-               );
-               $val = $num * $factor_for{lc $factor};
-            }
-            elsif ( $val =~ m/No default/ ) {
-               $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
-      }
+   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
+               (.*)      # Value
+               \1        # Closing quote
+               [\n\r]*\z # End of value
+            /$2/x;
+   if ( my ($num, $factor) = $val =~ m/(\d+)([KMGT])b?$/i ) {
+      my %factor_for = (
+         k => 1_024,
+         m => 1_048_576,
+         g => 1_073_741_824,
+         t => 1_099_511_627_776,
+      );
+      $val = $num * $factor_for{lc $factor};
+   }
+   elsif ( $val =~ m/No default/ ) {
+      $val = '';
+   }
+   return $val;
+}
+
 sub _mimic_show_variables {
    my ( %args ) = @_;
    my @required_args = qw(vars format);

=== modified file 'lib/MySQLConfig.pm'
--- lib/MySQLConfig.pm	2012-07-30 14:50:42 +0000
+++ lib/MySQLConfig.pm	2012-12-04 08:13:22 +0000
@@ -259,18 +259,28 @@
    #   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.
-   my $varvals = substr($output, (pos $output) + 1, length $output);
+   # Grab the varval list.
+   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 @@
 
    # Parse the "--var=val" lines.
    my ($config, $dupes) = _parse_varvals(
-      map { $_ =~ m/^--([^=]+)(?:=(.*))?$/ } split("\n", $output)
+      qr/^--([^=]+)(?:=(.*))?$/,
+      $output,
    );
 
    return $config, $dupes;
@@ -309,107 +320,116 @@
 
    # Parse the "var=val" lines.
    my ($config, $dupes) = _parse_varvals(
-      map  { $_ =~ m/^([^=]+)(?:=(.*))?$/ }
-      grep { $_ !~ m/^\s*#/ }  # no # comment lines
-      split("\n", $mysqld_section)
+      qr/^([^=]+)(?:=(.*))?$/,
+      $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
-# at index 1 then repeat for the next var-val pair.  
+# vars.
 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.
-   my $var;  # current variable (e.g. datadir)
-   my $val;  # value for current variable
-   ITEM:
-   foreach my $item ( @varvals ) {
-      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;
-
+   while ( my ($var, $vals) = each %$vars ) {
+      my $val = _process_val( pop @$vals );
+      # If the variable has duplicates, then @$vals will contain
+      # the rest of the values
+      if ( @$vals && !$can_be_duplicate{$var} ) {
          # 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
-         # 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
-         }
-      }
-      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
-         # blank strings, not NULL/undef.
-         if ( !defined $val ) {
-            $val = '';
-         }
-         else {
-            $val =~ s/
-                        \A        # Start of value
-                        (['"])    # Opening quote
-                        (.*)      # Value
-                        \1        # Closing quote
-                        [\n\r]*\z # End of value
-                    /$2/x;
-            if ( my ($num, $factor) = $val =~ m/(\d+)([KMGT])b?$/i ) {
-               # value is a size like 1k, 16M, etc.
-               my %factor_for = (
-                  k => 1_024,
-                  m => 1_048_576,
-                  g => 1_073_741_824,
-                  t => 1_099_511_627_776,
-               );
-               $val = $num * $factor_for{lc $factor};
-            }
-            elsif ( $val =~ m/No default/ ) {
-               $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
-      }
+         # but only the last is used).
+         PTDEBUG && _d("Duplicate var:", $var);
+         foreach my $current_val ( map { _process_val($_) } @$vals ) {
+            push @{$duplicates{$var} ||= []}, $current_val;
+         }
+      }
+
+      PTDEBUG && _d("Var:", $var, "val:", $val);
+
+      # Save this var-val.
+      $config{$var} = $val;
    }
 
    return \%config, \%duplicates;
 }
 
+sub _process_val {
+   my ($val) = @_;
+   $val =~ s/
+               \A        # Start of value
+               (['"])    # Opening quote
+               (.*)      # Value
+               \1        # Closing quote
+               [\n\r]*\z # End of value
+            /$2/x;
+   if ( my ($num, $factor) = $val =~ m/(\d+)([KMGT])b?$/i ) {
+      # value is a size like 1k, 16M, etc.
+      my %factor_for = (
+         k => 1_024,
+         m => 1_048_576,
+         g => 1_073_741_824,
+         t => 1_099_511_627_776,
+      );
+      $val = $num * $factor_for{lc $factor};
+   }
+   elsif ( $val =~ m/No default/ ) {
+      $val = '';
+   }
+   return $val;
+}
+
 # Sub: _mimic_show_variables
 #   Make the variables' values mimic SHOW VARIABLES.  Different output formats
 #   list values differently.  To make comparisons easier, outputs are made to

=== modified file 't/lib/MySQLConfig.t'
--- t/lib/MySQLConfig.t	2012-06-03 17:54:32 +0000
+++ t/lib/MySQLConfig.t	2012-12-04 08:13:22 +0000
@@ -9,7 +9,7 @@
 use strict;
 use warnings FATAL => 'all';
 use English qw(-no_match_vars);
-use Test::More tests => 30;
+use Test::More;
 
 use MySQLConfig;
 use DSNParser;
@@ -828,6 +828,30 @@
 }
 
 # #############################################################################
+# 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 @@
    '_d() works'
 );
 ok($sb->ok(), "Sandbox servers") or BAIL_OUT(__FILE__ . " broke the sandbox");
+done_testing;
 exit;

=== added file 't/pt-config-diff/samples/bug_917770.cnf'
--- t/pt-config-diff/samples/bug_917770.cnf	1970-01-01 00:00:00 +0000
+++ t/pt-config-diff/samples/bug_917770.cnf	2012-12-04 08:13:22 +0000
@@ -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
+ 
+ 
\ No newline at end of file