From 0468d121a8c40c25403634991b58a32aa5cf8bb7 Mon Sep 17 00:00:00 2001 From: Ed Silva Date: Tue, 18 Apr 2017 10:22:52 -0700 Subject: [PATCH 1/2] fix logic for testing inclusive and exclusive ranges --- lib/Monitoring/Plugin/Range.pm | 75 +++++++++------ t/Monitoring-Plugin-01.t | 19 ++-- t/Monitoring-Plugin-Range.t | 171 +++++++++++++++++---------------- t/check_stuff.t | 6 +- 4 files changed, 150 insertions(+), 121 deletions(-) diff --git a/lib/Monitoring/Plugin/Range.pm b/lib/Monitoring/Plugin/Range.pm index 4cacfd4..8a23f0e 100644 --- a/lib/Monitoring/Plugin/Range.pm +++ b/lib/Monitoring/Plugin/Range.pm @@ -75,9 +75,9 @@ sub parse_range_string { } if ($string =~ /^($value_re)$/) { # 'x:10' or '10' $range->_set_range_end($string); + $range->end_infinity(0); $valid++; } - if ($valid && ($range->start_infinity == 1 || $range->end_infinity == 1 || $range->start <= $range->end)) { return $range; } @@ -86,36 +86,53 @@ sub parse_range_string { # Returns 1 if an alert should be raised, otherwise 0 sub check_range { - my ($self, $value) = @_; - my $false = 0; - my $true = 1; - if ($self->alert_on == INSIDE) { - $false = 1; - $true = 0; - } - if ($self->end_infinity == 0 && $self->start_infinity == 0) { - if ($self->start <= $value && $value <= $self->end) { - return $false; - } else { - return $true; - } - } elsif ($self->start_infinity == 0 && $self->end_infinity == 1) { - if ( $value >= $self->start ) { - return $false; - } else { - return $true; - } - } elsif ($self->start_infinity == 1 && $self->end_infinity == 0) { - if ($value <= $self->end) { - return $false; - } else { - return $true; - } - } else { - return $false; - } + my ( $self, $value ) = @_; + my ( $eval_string, $op ); + + # this could be a from-to or equality check + if ( $self->end_infinity == 0 && $self->start_infinity == 0 ) { + + # this could be a test for exclusion (outside the range) + # or inclusion (inside the range) + my $left_op = $self->alert_on == INSIDE ? '>=' : '<'; + my $right_op = $self->alert_on == INSIDE ? '<=' : '>'; + my $cmp = $self->alert_on == INSIDE ? '&&' : '||'; + $eval_string = sprintf( + "%s %s %s %s %s %s %s", + $value, $left_op, $self->start, $cmp, + $value, $right_op, $self->end + ); + } + else { + my $check; + + # there is a start value, testing to infinity + if ( $self->start_infinity == 0 && $self->end_infinity == 1 ) { + + # if we are checking for a value inside the range + # it will be >= the start value + $op = $self->alert_on == INSIDE ? '>=' : '<'; + $check = $self->start; + } + + # there is an end value, testing from negative infinity + elsif ( $self->start_infinity == 1 && $self->end_infinity == 0 ) { + + # if we are checking for a value inside the range + # it will be <= the start value + $op = $self->alert_on == INSIDE ? '<=' : '>' + $check = $self->end; + } + else { + return 0; + } + $eval_string = "$value $op $check"; + } + + return int( eval $eval_string ); } + # Constructor - map args to hashref for SUPER sub new { diff --git a/t/Monitoring-Plugin-01.t b/t/Monitoring-Plugin-01.t index f5882a5..5863085 100644 --- a/t/Monitoring-Plugin-01.t +++ b/t/Monitoring-Plugin-01.t @@ -30,7 +30,7 @@ $p = Monitoring::Plugin->new( shortname => "SIZE", plugin => "check_stuff", () is($p->shortname, "SIZE", "shortname is not overriden by default"); diag "warn if < 10, critical if > 25 " if $ENV{TEST_VERBOSE}; -my $t = $p->set_thresholds( warning => "10:25", critical => "~:25" ); +my $t = $p->set_thresholds( warning => "10:", critical => "~:25" ); use Data::Dumper; #diag "dumping p: ". Dumper $p; @@ -44,7 +44,7 @@ $p->add_perfdata( threshold => $t, ); -cmp_ok( $p->all_perfoutput, 'eq', "size=1kB;10:25;~:25", "Perfdata correct"); +cmp_ok( $p->all_perfoutput, 'eq', "size=1kB;10:;~:25", "Perfdata correct"); #diag "dumping perfdata: ". Dumper ($p->perfdata); $p->add_perfdata( @@ -53,7 +53,7 @@ $p->add_perfdata( threshold => $t, ); -is( $p->all_perfoutput, "size=1kB;10:25;~:25 time=3.52;10:25;~:25", "Perfdata correct when no uom specified"); +is( $p->all_perfoutput, "size=1kB;10:;~:25 time=3.52;10:;~:25", "Perfdata correct when no uom specified"); my $expected = {qw( -1 WARNING @@ -64,8 +64,13 @@ my $expected = {qw( 30 CRITICAL )}; -foreach (sort {$a<=>$b} keys %$expected) { - like $p->die( return_code => $t->get_status($_), message => "page size at http://... was ${_}kB" ), - qr/$expected->{$_}/, - "Output okay. $_ = $expected->{$_}" ; +foreach ( sort { $a <=> $b } keys %$expected ) { + like( + $p->die( + return_code => $t->get_status($_), + message => "page size at http://... was ${_}kB" + ), + qr/$expected->{$_}/, + "Output okay. $_ = $expected->{$_}" + ); } diff --git a/t/Monitoring-Plugin-Range.t b/t/Monitoring-Plugin-Range.t index 9a6e826..fac2f2c 100644 --- a/t/Monitoring-Plugin-Range.t +++ b/t/Monitoring-Plugin-Range.t @@ -31,7 +31,7 @@ foreach (qw( diag "range: 0..6 inclusive" if $ENV{TEST_VERBOSE}; -$r = Monitoring::Plugin::Range->parse_range_string("6"); +$r = Monitoring::Plugin::Range->parse_range_string("\@6"); isa_ok( $r, "Monitoring::Plugin::Range"); ok( defined $r, "'6' is valid range"); cmp_ok( $r->start, '==', 0, "Start correct"); @@ -41,27 +41,31 @@ cmp_ok( $r->end_infinity, '==', 0, "Not using positive infinity"); cmp_ok( $r, 'eq', "6", "Stringification back to original"); my $expected = { - -1 => 1, # 1 means it raises an alert because it's OUTSIDE the range - 0 => 0, # 0 means it's inside the range (no alert) - 4 => 0, - 6 => 0, - 6.1 => 1, - 79.999999 => 1, + -1 => 0, # 0 means it's OUTSIDE the range (no alert) + 0 => 1, # 1 means it's inside the range (alert) + 4 => 1, + 6 => 1, + 6.1 => 0, + 79.999999 => 0, }; sub test_expected { - my $r = shift; - my $expected = shift; - foreach (sort {$a<=>$b} keys %$expected) { - is $r->check_range($_), $expected->{$_}, - " $_ should " . ($expected->{$_} ? 'not ' : '') . "be in the range (line ".(caller)[2].")"; - } + my $r = shift; + my $expected = shift; + foreach ( sort { $a <=> $b } keys %$expected ) { + my $res = $r->check_range($_); + is $r->check_range($_), $expected->{$_}, + " $_ should " + . ( $expected->{$_} ? 'not ' : '' ) + . "be in the range (line " + . (caller)[2] . ")"; + } } test_expected( $r, $expected ); diag "range : -7..23, inclusive" if $ENV{TEST_VERBOSE}; -$r = Monitoring::Plugin::Range->parse_range_string("-7:23"); +$r = Monitoring::Plugin::Range->parse_range_string("\@-7:23"); ok( defined $r, "'-7:23' is valid range"); cmp_ok( $r->start, '==', -7, "Start correct"); cmp_ok( $r->start_infinity, '==', 0, "Not using negative infinity"); @@ -70,20 +74,20 @@ cmp_ok( $r->end_infinity, '==', 0, "Not using positive infinity"); cmp_ok( $r, 'eq', "-7:23", "Stringification back to original"); $expected = { - -23 => 1, - -7 => 0, - -1 => 0, - 0 => 0, - 4 => 0, - 23 => 0, - 23.1 => 1, - 79.999999 => 1, + -23 => 0, + -7 => 1, + -1 => 1, + 0 => 1, + 4 => 1, + 23 => 1, + 23.1 => 0, + 79.999999 => 0, }; test_expected( $r, $expected ); diag "range : 0..5.75, inclusive" if $ENV{TEST_VERBOSE}; -$r = Monitoring::Plugin::Range->parse_range_string(":5.75"); +$r = Monitoring::Plugin::Range->parse_range_string("\@:5.75"); ok( defined $r, "':5.75' is valid range"); cmp_ok( $r->start, '==', 0, "Start correct"); cmp_ok( $r->start_infinity, '==', 0, "Not using negative infinity"); @@ -91,55 +95,58 @@ cmp_ok( $r->end, '==', 5.75, "End correct"); cmp_ok( $r->end_infinity, '==', 0, "Not using positive infinity"); cmp_ok( $r, 'eq', "5.75", "Stringification to simplification"); $expected = { - -1 => 1, - 0 => 0, - 4 => 0, - 5.75 => 0, - 5.7501 => 1, - 6 => 1, - 6.1 => 1, - 79.999999 => 1, + -1 => 0, + 0 => 1, + 4 => 1, + 5.75 => 1, + 5.7501 => 0, + 6 => 0, + 6.1 => 0, + 79.999999 => 0, }; test_expected( $r, $expected ); diag "range : negative infinity .. -95.99, inclusive" if $ENV{TEST_VERBOSE}; -$r = Monitoring::Plugin::Range->parse_range_string("~:-95.99"); +$r = Monitoring::Plugin::Range->parse_range_string("\@~:-95.99"); ok( defined $r, "'~:-95.99' is valid range"); cmp_ok( $r->start_infinity, '==', 1, "Using negative infinity"); cmp_ok( $r->end, '==', -95.99, "End correct"); cmp_ok( $r->end_infinity, '==', 0, "Not using positive infinity"); cmp_ok( $r, 'eq', "~:-95.99", "Stringification back to original"); $expected = { - -1001341 => 0, - -96 => 0, - -95.999 => 0, - -95.99 => 0, - -95.989 => 1, - -95 => 1, - 0 => 1, - 5.7501 => 1, - 79.999999 => 1, + -1001341 => 1, + -96 => 1, + -95.999 => 1, + -95.99 => 1, + -95.989 => 0, + -95 => 0, + 0 => 0, + 5.7501 => 0, + 79.999999 => 0, }; test_expected( $r, $expected ); diag "range 10..infinity , inclusive" if $ENV{TEST_VERBOSE}; test_expected( $r, $expected ); -$r = Monitoring::Plugin::Range->parse_range_string("10:"); +$r = Monitoring::Plugin::Range->parse_range_string("\@10:"); +use Data::Dumper::Concise; +print 'RANGE: ', Dumper($r); + ok( defined $r, "'10:' is valid range"); cmp_ok( $r->start, '==', 10, "Start correct"); cmp_ok( $r->start_infinity, '==', 0, "Not using negative infinity"); cmp_ok( $r->end_infinity, '==', 1, "Using positive infinity"); cmp_ok( $r, 'eq', "10:", "Stringification back to original"); $expected = { - -95.999 => 1, - -1 => 1, - 0 => 1, - 9.91 => 1, - 10 => 0, - 11.1 => 0, - 123456789012346 => 0, + -95.999 => 0, + -1 => 0, + 0 => 0, + 9.91 => 0, + 10 => 1, + 11.1 => 1, + 123456789012346 => 1, }; test_expected( $r, $expected ); @@ -147,70 +154,70 @@ test_expected( $r, $expected ); diag "range 123456789012345..infinity , inclusive" if $ENV{TEST_VERBOSE}; test_expected( $r, $expected ); -$r = Monitoring::Plugin::Range->parse_range_string("123456789012345:"); +$r = Monitoring::Plugin::Range->parse_range_string("\@123456789012345:"); ok( defined $r, "'123456789012345:' is valid range"); cmp_ok( $r->start, '==', 123456789012345, "Start correct"); cmp_ok( $r->start_infinity, '==', 0, "Not using negative infinity"); cmp_ok( $r->end_infinity, '==', 1, "Using positive infinity"); cmp_ok( $r, 'eq', "123456789012345:", "Stringification back to original"); $expected = { - -95.999 => 1, - -1 => 1, - 0 => 1, + -95.999 => 0, + -1 => 0, + 0 => 0, # The fractional values needs to be quoted, otherwise the hash rounds it up to ..345 # and there is one less test run. # I think some newer versions of perl use a higher precision value for the hash key. # This doesn't appear to affect the actual plugin though - "123456789012344.91" => 1, - 123456789012345 => 0, - "123456789012345.61" => 0, - 123456789012346 => 0, + "123456789012344.91" => 0, + 123456789012345 => 1, + "123456789012345.61" => 1, + 123456789012346 => 1, }; test_expected( $r, $expected ); diag "range: <= zero " if $ENV{TEST_VERBOSE}; -$r = Monitoring::Plugin::Range->parse_range_string("~:0"); +$r = Monitoring::Plugin::Range->parse_range_string("\@~:0"); ok( defined $r, "'~:0' is valid range"); cmp_ok( $r->start_infinity, '==', 1, "Using negative infinity"); cmp_ok( $r->end, '==', 0, "End correct"); cmp_ok( $r->end_infinity, '==', 0, "Not using positive infinity"); -cmp_ok( $r->alert_on, '==', 0, "Will alert on outside of range"); +cmp_ok( $r->alert_on, '==', 1, "Will alert on inside of range"); cmp_ok( $r, 'eq', "~:0", "Stringification back to original"); -ok( $r->check_range(0.5) == 1, "0.5 - alert"); -ok( $r->check_range(-10) == 0, "-10 - no alert"); -ok( $r->check_range(0) == 0, "0 - no alert"); +ok( $r->check_range(0.5) == 0, "0.5 - no alert"); +ok( $r->check_range(-10) == 1, "-10 - alert"); +ok( $r->check_range(0) == 1, "0 - alert"); $expected = { - -123456789012344.91 => 0, - -1 => 0, - 0 => 0, - .001 => 1, - 123456789012345 => 1, + -123456789012344.91 => 1, + -1 => 1, + 0 => 1, + .001 => 0, + 123456789012345 => 0, }; test_expected( $r, $expected ); diag "range: OUTSIDE 0..657.8210567" if $ENV{TEST_VERBOSE}; -$r = Monitoring::Plugin::Range->parse_range_string('@0:657.8210567'); -ok( defined $r, '"@0:657.8210567" is a valid range'); +$r = Monitoring::Plugin::Range->parse_range_string('0:657.8210567'); +ok( defined $r, '"0:657.8210567" is a valid range'); cmp_ok( $r->start, '==', 0, "Start correct"); cmp_ok( $r->start_infinity, '==', 0, "Not using negative infinity"); cmp_ok( $r->end, '==', 657.8210567, "End correct"); cmp_ok( $r->end_infinity, '==', 0, "Not using positive infinity"); -cmp_ok( $r->alert_on, '==', 1, "Will alert on inside of range"); -cmp_ok( $r, 'eq', '@657.8210567', "Stringification to simplified version"); -ok( $r->check_range(32.88) == 1, "32.88 - alert"); -ok( $r->check_range(-2) == 0, "-2 - no alert"); -ok( $r->check_range(657.8210567) == 1, "657.8210567 - alert"); -ok( $r->check_range(0) == 1, "0 - alert"); +cmp_ok( $r->alert_on, '==', 0, "Will alert on outside of range"); +cmp_ok( $r, 'eq', '657.8210567', "Stringification to simplified version"); +ok( $r->check_range(32.88) == 0, "32.88 - no alert"); +ok( $r->check_range(-2) == 1, "-2 - no alert"); +ok( $r->check_range(657.8210567) == 0, "657.8210567 - no alert"); +ok( $r->check_range(0) == 0, "0 - no alert"); $expected = { - -134151 => 0, - -1 => 0, - 0 => 1, - .001 => 1, - 657.8210567 => 1, - 657.9 => 0, - 123456789012345 => 0, + -134151 => 1, + -1 => 1, + 0 => 0, + .001 => 0, + 657.8210567 => 0, + 657.9 => 1, + 123456789012345 => 1, }; test_expected( $r, $expected ); diff --git a/t/check_stuff.t b/t/check_stuff.t index 6a1d845..c3d778b 100755 --- a/t/check_stuff.t +++ b/t/check_stuff.t @@ -39,9 +39,9 @@ like $r, qr/UNKNOWN.+invalid/i, "UNKNOWN (warning: invalid -r) with $args"; my $expected = { - " -w 10:15 -c~:15 -r 0" => 'WARNING', - " -w 10:15 -c~:15 -r 11" => 'OK', - " -w 10:15 -c~:15 -r 15.8" => 'CRITICAL', + " -w \@10:15 -c~:15 -r 0" => 'OK', + " -w \@10:15 -c~:15 -r 11" => 'WARNING', + " -c~:15 -r 15.8" => 'CRITICAL', }; test_expected( $s, $expected ); From 1ef0b1369439095264a59d5ed1c3dae3073fc4e3 Mon Sep 17 00:00:00 2001 From: Ed Silva Date: Tue, 18 Apr 2017 11:42:15 -0700 Subject: [PATCH 2/2] reformat and untaint value --- lib/Monitoring/Plugin/Range.pm | 89 +++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 35 deletions(-) diff --git a/lib/Monitoring/Plugin/Range.pm b/lib/Monitoring/Plugin/Range.pm index 8a23f0e..d938d3c 100644 --- a/lib/Monitoring/Plugin/Range.pm +++ b/lib/Monitoring/Plugin/Range.pm @@ -48,47 +48,66 @@ sub _set_range_end { # Returns a N::P::Range object if the string is a conforms to a Monitoring Plugin range string, otherwise null sub parse_range_string { - my ($class, $string) = @_; - my $valid = 0; - my $range = $class->new( start => 0, start_infinity => 0, end => 0, end_infinity => 1, alert_on => OUTSIDE); - - $string =~ s/\s//g; # strip out any whitespace - # check for valid range definition - unless ( $string =~ /[\d~]/ && $string =~ m/^\@?($value_re|~)?(:($value_re)?)?$/ ) { - carp "invalid range definition '$string'"; - return undef; - } - - if ($string =~ s/^\@//) { - $range->alert_on(INSIDE); - } - - if ($string =~ s/^~//) { # '~:x' - $range->start_infinity(1); - } - if ( $string =~ m/^($value_re)?:/ ) { # '10:' - my $start = $1; - $range->_set_range_start($start) if defined $start; - $range->end_infinity(1); # overridden below if there's an end specified - $string =~ s/^($value_re)?://; - $valid++; - } - if ($string =~ /^($value_re)$/) { # 'x:10' or '10' - $range->_set_range_end($string); - $range->end_infinity(0); - $valid++; - } - if ($valid && ($range->start_infinity == 1 || $range->end_infinity == 1 || $range->start <= $range->end)) { - return $range; - } - return undef; + my ( $class, $string ) = @_; + my $valid = 0; + my $range = $class->new( + start => 0, start_infinity => 0, end => 0, + end_infinity => 1, alert_on => OUTSIDE + ); + + $string =~ s/\s//g; # strip out any whitespace + # check for valid range definition + unless ( $string =~ /[\d~]/ + && $string =~ m/^\@?($value_re|~)?(:($value_re)?)?$/ ) { + carp "invalid range definition '$string'"; + return undef; + } + + if ( $string =~ s/^\@// ) { + $range->alert_on(INSIDE); + } + + if ( $string =~ s/^~// ) { # '~:x' + $range->start_infinity(1); + } + if ( $string =~ m/^($value_re)?:/ ) { # '10:' + my $start = $1; + $range->_set_range_start($start) if defined $start; + $range->end_infinity(1); # overridden below if there's an end specified + $string =~ s/^($value_re)?://; + $valid++; + } + if ( $string =~ /^($value_re)$/ ) { # 'x:10' or '10' + $range->_set_range_end($string); + $range->end_infinity(0); + $valid++; + } + if ( + $valid + && ( $range->start_infinity == 1 + || $range->end_infinity == 1 + || $range->start <= $range->end ) + ) { + return $range; + } + return undef; } + # Returns 1 if an alert should be raised, otherwise 0 sub check_range { my ( $self, $value ) = @_; my ( $eval_string, $op ); + # untaint + if ($value =~ /($value_re)/) { + $value = $1; + } + else { + carp "invalid value '$value'"; + return undef; + } + # this could be a from-to or equality check if ( $self->end_infinity == 0 && $self->start_infinity == 0 ) { @@ -120,7 +139,7 @@ sub check_range { # if we are checking for a value inside the range # it will be <= the start value - $op = $self->alert_on == INSIDE ? '<=' : '>' + $op = $self->alert_on == INSIDE ? '<=' : '>'; $check = $self->end; } else {