From 5a97e82579009fad1c1c7a906d3a1c98346e62f0 Mon Sep 17 00:00:00 2001 From: Cristian Ciutea Date: Fri, 17 Jul 2020 16:12:41 +0200 Subject: [PATCH] Fix panic caused by providing a not valid regex pattern (#47) * fix: fixed panic caused by providing a not valid regex pattern to process filter config Co-authored-by: Carlos --- pkg/metrics/sampler/matcher.go | 23 +++++++++--- pkg/metrics/sampler/matcher_test.go | 57 ++++++++++++++++++++++++----- 2 files changed, 65 insertions(+), 15 deletions(-) diff --git a/pkg/metrics/sampler/matcher.go b/pkg/metrics/sampler/matcher.go index d4cdcfc5a..961663ed1 100644 --- a/pkg/metrics/sampler/matcher.go +++ b/pkg/metrics/sampler/matcher.go @@ -5,6 +5,7 @@ package sampler import ( "fmt" + "github.com/newrelic/infrastructure-agent/pkg/log" "reflect" "regexp" "strings" @@ -20,6 +21,8 @@ var ( typesToEvaluate = map[string]bool{"ProcessSample": true} ) +var mlog = log.WithComponent("SamplerMatcher") + // IncludeSampleMatchFn func that returns whether an event/sample should be included, it satisfies // the metrics matcher (processor.MatcherChain) interface. type IncludeSampleMatchFn func(sample interface{}) bool @@ -114,7 +117,7 @@ func build(dimensionName string, expr string) ExpressionMatcher { // so this matcher basically get's ignored in the current implementation mappedAttributeName, found := attrCache[dimensionName] if !found { - return constantMatcher{false} + return constantMatcher{value: false} } eval := matcher{ @@ -123,7 +126,10 @@ func build(dimensionName string, expr string) ExpressionMatcher { if strings.HasPrefix(expr, "regex") { regex := strings.Trim(strings.TrimSpace(strings.TrimLeft(expr, "regex")), `"`) - cacheRegex(regex) + if err := cacheRegex(regex); err != nil { + mlog.WithError(err).Error(fmt.Sprintf("could not intitilize expression matcher for the provided configuration: '%s'", expr)) + return constantMatcher{value: false} + } eval.ExpectedValue = regex eval.Evaluator = regularExpressionEvaluator } else { @@ -134,11 +140,16 @@ func build(dimensionName string, expr string) ExpressionMatcher { return eval } -func cacheRegex(regex string) { +func cacheRegex(pattern string) error { //if not cached yet, cache it - if _, ok := regexCache[regex]; !ok { - regexCache[regex] = regexp.MustCompile(regex) + if _, ok := regexCache[pattern]; !ok { + regex, err := regexp.Compile(pattern) + if err != nil { + return err + } + regexCache[pattern] = regex } + return nil } // MatcherChain is a chain of evaluators @@ -260,6 +271,6 @@ func NewSampleMatchFn(enableProcessMetrics *bool, includeMetricsMatchers config. } enabled, exists := ffRetriever.GetFeatureFlag(handler.FlagFullProcess) - return exists && enabled + return exists && enabled } } diff --git a/pkg/metrics/sampler/matcher_test.go b/pkg/metrics/sampler/matcher_test.go index 1a2755f43..2aae22e84 100644 --- a/pkg/metrics/sampler/matcher_test.go +++ b/pkg/metrics/sampler/matcher_test.go @@ -26,8 +26,8 @@ import ( "github.com/stretchr/testify/assert" ) -var metricDimensionProcessName string = "process.name" -var metricDimensionProcessExecutable string = "process.executable" +var metricDimensionProcessName = "process.name" +var metricDimensionProcessExecutable = "process.executable" func Test_EvaluatorChain_WithSingleRule(t *testing.T) { @@ -103,6 +103,14 @@ func Test_EvaluatorChain_WithSingleRule(t *testing.T) { }, want: false, }, + { + name: "ProcessCmdLine_NotValidRegex", + input: &types.ProcessSample{CmdLine: "/bin/java"}, + rules: map[string][]string{ + metricDimensionProcessExecutable: {"regex *"}, + }, + want: false, + }, } for _, tc := range cases { @@ -165,6 +173,15 @@ func Test_Evaluator_WithTwoLiteralRules(t *testing.T) { }, want: true, }, + { + name: "ProcessNameAndExecutableAreMatchNotValidRegex", + input: javaProcessSample, + rules: map[string][]string{ + metricDimensionProcessExecutable: {"regex *"}, + metricDimensionProcessName: {"java"}, + }, + want: true, + }, } for _, tc := range cases { @@ -453,6 +470,27 @@ func Test_EvaluatorChain_WithMultipleRuleAttribute(t *testing.T) { }, want: []bool{true, true, false, true, true, true, false}, }, + { + /* + matchers: + process.executable: + - regex * # bad regex + - regex .* + */ + name: "ProcessExecutable_NotValidRegex", + input: []interface{}{ + types.ProcessSample{CmdLine: "/bin/java"}, + types.ProcessSample{CmdLine: "/bin/local/java"}, + }, + rules: map[string][]string{ + metricDimensionProcessExecutable: { + "regex *", + "/bin/java", + "/bin/local/java", + }, + }, + want: []bool{true, true}, + }, } for _, tc := range cases { @@ -509,11 +547,13 @@ func Test_EvaluatorChain_LogTraceMatcher(t *testing.T) { } type enabledFFRetriever struct{} + func (e *enabledFFRetriever) GetFeatureFlag(name string) (enabled bool, exists bool) { return true, true } type disabledFFRetriever struct{} + func (e *disabledFFRetriever) GetFeatureFlag(name string) (enabled bool, exists bool) { return false, true } @@ -523,7 +563,6 @@ func TestNewSampleMatchFn(t *testing.T) { falseVar := false emptyMatchers := config.IncludeMetricsMap{} - type args struct { enableProcessMetrics *bool includeMetricsMatchers config.IncludeMetricsMap @@ -588,20 +627,20 @@ func TestNewSampleMatchFn(t *testing.T) { { name: "process samples matching rules are included", args: args{ - enableProcessMetrics: &trueVar, + enableProcessMetrics: &trueVar, includeMetricsMatchers: config.IncludeMetricsMap{"process.name": []string{"regex \"foo.*\""}}, - ffRetriever: testFF.EmptyFFRetriever, - sample: &fixture.ProcessSample, + ffRetriever: testFF.EmptyFFRetriever, + sample: &fixture.ProcessSample, }, include: true, }, { name: "process samples not matching rules are not included", args: args{ - enableProcessMetrics: &trueVar, + enableProcessMetrics: &trueVar, includeMetricsMatchers: config.IncludeMetricsMap{"process.name": []string{"regex \"bar*\""}}, - ffRetriever: testFF.EmptyFFRetriever, - sample: &fixture.ProcessSample, + ffRetriever: testFF.EmptyFFRetriever, + sample: &fixture.ProcessSample, }, include: false, },