From 76a010abeed489237b2659f578ca7a7be046372e Mon Sep 17 00:00:00 2001 From: Brian Fraser Date: Fri, 30 Nov 2012 16:17:45 -0300 Subject: [PATCH 1/2] Fix for 917770: Use of uninitialized value in substitution (s///) at pt-config-diff line 1996 This turned out to be two bugs mangled into one. First, _parse_varvals can deal with (var, undef), but not with (undef). This is a problem because two of the trhee spots that call _parse_varvals can return undef because of this: map { $_ =~ m/^([^=]+)(?:=(.*))?$/ } grep { $_ !~ m/^\s*#/ } # no # comment lines split("\n", $mysqld_section) The problem is twofold. First, we are not skipping empty or whitespace-only lines. That means that the map will fail, and pass an undef to _parse_varvals. So this ended up in a triple fix: Make _parse_varvals deal with a sole undef, skip empty/whitespace lines, and change that map to map { $_ =~ m/^([^=]+)(?:=(.*))?$/ ? ($1, $2) : () } so even if the regex fails in the future, no sole undef will be passed down the chain. --- bin/pt-config-diff | 10 ++++++-- lib/MySQLConfig.pm | 12 ++++++++-- t/lib/MySQLConfig.t | 27 ++++++++++++++++++++- t/pt-config-diff/samples/bug_917770.cnf | 31 +++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 5 deletions(-) create mode 100644 t/pt-config-diff/samples/bug_917770.cnf diff --git a/bin/pt-config-diff b/bin/pt-config-diff index bb96108f..9b200985 100755 --- a/bin/pt-config-diff +++ b/bin/pt-config-diff @@ -2168,7 +2168,9 @@ sub parse_my_print_defaults { my ($output) = @args{@required_args}; my ($config, $dupes) = _parse_varvals( - map { $_ =~ m/^--([^=]+)(?:=(.*))?$/ } split("\n", $output) + map { $_ =~ m/^--([^=]+)(?:=(.*))?$/ ? ($1, $2) : () } + grep { $_ !~ m/^\s*$/ } # no empty lines + split("\n", $output) ); return $config, $dupes; @@ -2186,7 +2188,8 @@ sub parse_option_file { die "Failed to parse the [mysqld] section" unless $mysqld_section; my ($config, $dupes) = _parse_varvals( - map { $_ =~ m/^([^=]+)(?:=(.*))?$/ } + map { $_ =~ m/^([^=]+)(?:=(.*))?$/ ? ($1, $2) : () } + grep { $_ !~ m/^\s*$/ } # no empty lines grep { $_ !~ m/^\s*#/ } # no # comment lines split("\n", $mysqld_section) ); @@ -2206,6 +2209,9 @@ sub _parse_varvals { my $val; # value for current variable ITEM: foreach my $item ( @varvals ) { + if ( !defined($item) ) { + $item = ''; + } if ( $item ) { $item =~ s/^\s+//; $item =~ s/\s+$//; diff --git a/lib/MySQLConfig.pm b/lib/MySQLConfig.pm index 703cada5..46f9af32 100644 --- a/lib/MySQLConfig.pm +++ b/lib/MySQLConfig.pm @@ -288,7 +288,9 @@ sub parse_my_print_defaults { # Parse the "--var=val" lines. my ($config, $dupes) = _parse_varvals( - map { $_ =~ m/^--([^=]+)(?:=(.*))?$/ } split("\n", $output) + map { $_ =~ m/^--([^=]+)(?:=(.*))?$/ ? ($1, $2) : () } + grep { $_ !~ m/^\s*$/ } # no empty lines + split("\n", $output) ); return $config, $dupes; @@ -309,7 +311,8 @@ sub parse_option_file { # Parse the "var=val" lines. my ($config, $dupes) = _parse_varvals( - map { $_ =~ m/^([^=]+)(?:=(.*))?$/ } + map { $_ =~ m/^([^=]+)(?:=(.*))?$/ ? ($1, $2) : () } + grep { $_ !~ m/^\s*$/ } # no empty lines grep { $_ !~ m/^\s*#/ } # no # comment lines split("\n", $mysqld_section) ); @@ -336,6 +339,11 @@ sub _parse_varvals { my $val; # value for current variable ITEM: foreach my $item ( @varvals ) { + # We were passed an undef for the value, or we're dealing with an empty + # line + if ( !defined($item) ) { + $item = ''; + } if ( $item ) { # Strip leading and trailing whitespace. $item =~ s/^\s+//; diff --git a/t/lib/MySQLConfig.t b/t/lib/MySQLConfig.t index 86cde81c..3e9b7b62 100644 --- a/t/lib/MySQLConfig.t +++ b/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; diff --git a/t/pt-config-diff/samples/bug_917770.cnf b/t/pt-config-diff/samples/bug_917770.cnf new file mode 100644 index 00000000..65a78028 --- /dev/null +++ b/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 + + \ No newline at end of file From a7a0da3bc9b0223bb8d97e1b697b2a61c6917890 Mon Sep 17 00:00:00 2001 From: Brian Fraser Date: Tue, 4 Dec 2012 05:02:56 -0300 Subject: [PATCH 2/2] 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'. --- bin/pt-config-diff | 136 ++++++++++++++++---------------- lib/MySQLConfig.pm | 190 ++++++++++++++++++++++++--------------------- 2 files changed, 172 insertions(+), 154 deletions(-) diff --git a/bin/pt-config-diff b/bin/pt-config-diff index 9b200985..f9fe1566 100755 --- a/bin/pt-config-diff +++ b/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,9 +2169,8 @@ sub parse_my_print_defaults { my ($output) = @args{@required_args}; my ($config, $dupes) = _parse_varvals( - map { $_ =~ m/^--([^=]+)(?:=(.*))?$/ ? ($1, $2) : () } - grep { $_ !~ m/^\s*$/ } # no empty lines - split("\n", $output) + qr/^--([^=]+)(?:=(.*))?$/, + $output, ); return $config, $dupes; @@ -2188,88 +2188,94 @@ sub parse_option_file { die "Failed to parse the [mysqld] section" unless $mysqld_section; my ($config, $dupes) = _parse_varvals( - map { $_ =~ m/^([^=]+)(?:=(.*))?$/ ? ($1, $2) : () } - grep { $_ !~ m/^\s*$/ } # no empty lines - grep { $_ !~ m/^\s*#/ } # no # comment lines - split("\n", $mysqld_section) + qr/^([^=]+)(?:=(.*))?$/, + $mysqld_section, ); return $config, $dupes; } -sub _parse_varvals { - my ( @varvals ) = @_; +sub _preprocess_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; - my %duplicates; - - my $var; # current variable (e.g. datadir) - my $val; # value for current variable - ITEM: - foreach my $item ( @varvals ) { - if ( !defined($item) ) { - $item = ''; + if ( $line !~ $re ) { + PTDEBUG && _d("Line <", $line, "> didn't match $re"); + next LINE; } - if ( $item ) { + + my ($var, $val) = ($1, $2); + + $var =~ tr/-/_/; + + if ( !defined $val ) { + $val = ''; + } + + for my $item ($var, $val) { $item =~ s/^\s+//; $item =~ s/\s+$//; } - if ( !$var ) { - $var = $item; + push @{$vars{$var} ||= []}, $val + } - $var =~ s/-/_/g; + return \%vars; +} - if ( exists $config{$var} && !$can_be_duplicate{$var} ) { - PTDEBUG && _d("Duplicate var:", $var); - $duplicate_var = 1; # flag on, save all the var's values +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; } } - 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 = ''; - } - } + PTDEBUG && _d("Var:", $var, "val:", $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 - } + $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); diff --git a/lib/MySQLConfig.pm b/lib/MySQLConfig.pm index 46f9af32..ac9cd668 100644 --- a/lib/MySQLConfig.pm +++ b/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. - 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,9 +298,8 @@ sub parse_my_print_defaults { # Parse the "--var=val" lines. my ($config, $dupes) = _parse_varvals( - map { $_ =~ m/^--([^=]+)(?:=(.*))?$/ ? ($1, $2) : () } - grep { $_ !~ m/^\s*$/ } # no empty lines - split("\n", $output) + qr/^--([^=]+)(?:=(.*))?$/, + $output, ); return $config, $dupes; @@ -311,113 +320,116 @@ sub parse_option_file { # Parse the "var=val" lines. my ($config, $dupes) = _parse_varvals( - map { $_ =~ m/^([^=]+)(?:=(.*))?$/ ? ($1, $2) : () } - grep { $_ !~ m/^\s*$/ } # no empty lines - 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 ) { - # We were passed an undef for the value, or we're dealing with an empty - # line - if ( !defined($item) ) { - $item = ''; - } - 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 + # but only the last is used). + PTDEBUG && _d("Duplicate var:", $var); + 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 - # 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 = ''; - } - } + PTDEBUG && _d("Var:", $var, "val:", $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 - } + # 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