From b086ecee0c9dcf4d30c9703defa44e661ba0d208 Mon Sep 17 00:00:00 2001 From: Daniel Nelson Date: Tue, 11 Jul 2017 15:54:38 -0700 Subject: [PATCH] Do not allow metrics with trailing slashes (#3007) It is not possible to encode a measurement, tag, or field whose last character is a backslash due to it being an unescapable character. Because the tight coupling between line protocol and the internal metric model, prevent metrics like this from being created. Measurements with a trailing slash are not allowed and the point will be dropped. Tags and fields with a trailing a slash will be dropped from the point. --- internal/models/makemetric.go | 29 ++++++ internal/models/running_input_test.go | 123 ++++++++++++++++++++++++++ metric/metric.go | 23 ++++- metric/metric_test.go | 52 +++++++++++ 4 files changed, 226 insertions(+), 1 deletion(-) diff --git a/internal/models/makemetric.go b/internal/models/makemetric.go index 4e398043a..b2c150546 100644 --- a/internal/models/makemetric.go +++ b/internal/models/makemetric.go @@ -3,6 +3,7 @@ package models import ( "log" "math" + "strings" "time" "github.com/influxdata/telegraf" @@ -77,7 +78,27 @@ func makemetric( } } + for k, v := range tags { + if strings.HasSuffix(k, `\`) { + log.Printf("D! Measurement [%s] tag [%s] "+ + "ends with a backslash, skipping", measurement, k) + delete(tags, k) + continue + } else if strings.HasSuffix(v, `\`) { + log.Printf("D! Measurement [%s] tag [%s] has a value "+ + "ending with a backslash, skipping", measurement, k) + delete(tags, k) + continue + } + } + for k, v := range fields { + if strings.HasSuffix(k, `\`) { + log.Printf("D! Measurement [%s] field [%s] "+ + "ends with a backslash, skipping", measurement, k) + delete(fields, k) + continue + } // Validate uint64 and float64 fields // convert all int & uint types to int64 switch val := v.(type) { @@ -128,6 +149,14 @@ func makemetric( delete(fields, k) continue } + case string: + if strings.HasSuffix(val, `\`) { + log.Printf("D! Measurement [%s] field [%s] has a value "+ + "ending with a backslash, skipping", measurement, k) + delete(fields, k) + continue + } + fields[k] = v default: fields[k] = v } diff --git a/internal/models/running_input_test.go b/internal/models/running_input_test.go index 33c785c6e..1a6491e35 100644 --- a/internal/models/running_input_test.go +++ b/internal/models/running_input_test.go @@ -9,6 +9,7 @@ import ( "github.com/influxdata/telegraf" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestMakeMetricNoFields(t *testing.T) { @@ -332,6 +333,128 @@ func TestMakeMetricNameSuffix(t *testing.T) { ) } +func TestMakeMetric_TrailingSlash(t *testing.T) { + now := time.Now() + + tests := []struct { + name string + measurement string + fields map[string]interface{} + tags map[string]string + expectedNil bool + expectedMeasurement string + expectedFields map[string]interface{} + expectedTags map[string]string + }{ + { + name: "Measurement cannot have trailing slash", + measurement: `cpu\`, + fields: map[string]interface{}{ + "value": int64(42), + }, + tags: map[string]string{}, + expectedNil: true, + }, + { + name: "Field key with trailing slash dropped", + measurement: `cpu`, + fields: map[string]interface{}{ + "value": int64(42), + `bad\`: `xyzzy`, + }, + tags: map[string]string{}, + expectedMeasurement: `cpu`, + expectedFields: map[string]interface{}{ + "value": int64(42), + }, + expectedTags: map[string]string{}, + }, + { + name: "Field value with trailing slash dropped", + measurement: `cpu`, + fields: map[string]interface{}{ + "value": int64(42), + "bad": `xyzzy\`, + }, + tags: map[string]string{}, + expectedMeasurement: `cpu`, + expectedFields: map[string]interface{}{ + "value": int64(42), + }, + expectedTags: map[string]string{}, + }, + { + name: "Must have one field after dropped", + measurement: `cpu`, + fields: map[string]interface{}{ + "bad": `xyzzy\`, + }, + tags: map[string]string{}, + expectedNil: true, + }, + { + name: "Tag key with trailing slash dropped", + measurement: `cpu`, + fields: map[string]interface{}{ + "value": int64(42), + }, + tags: map[string]string{ + `host\`: "localhost", + "a": "x", + }, + expectedMeasurement: `cpu`, + expectedFields: map[string]interface{}{ + "value": int64(42), + }, + expectedTags: map[string]string{ + "a": "x", + }, + }, + { + name: "Tag value with trailing slash dropped", + measurement: `cpu`, + fields: map[string]interface{}{ + "value": int64(42), + }, + tags: map[string]string{ + `host`: `localhost\`, + "a": "x", + }, + expectedMeasurement: `cpu`, + expectedFields: map[string]interface{}{ + "value": int64(42), + }, + expectedTags: map[string]string{ + "a": "x", + }, + }, + } + + ri := NewRunningInput(&testInput{}, &InputConfig{ + Name: "TestRunningInput", + }) + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + m := ri.MakeMetric( + tc.measurement, + tc.fields, + tc.tags, + telegraf.Untyped, + now) + + if tc.expectedNil { + require.Nil(t, m) + } else { + require.NotNil(t, m) + require.Equal(t, tc.expectedMeasurement, m.Name()) + require.Equal(t, tc.expectedFields, m.Fields()) + require.Equal(t, tc.expectedTags, m.Tags()) + } + }) + } +} + type testInput struct{} func (t *testInput) Description() string { return "" } diff --git a/metric/metric.go b/metric/metric.go index fdef072c3..dd2d6cbc0 100644 --- a/metric/metric.go +++ b/metric/metric.go @@ -6,6 +6,7 @@ import ( "hash/fnv" "sort" "strconv" + "strings" "time" "github.com/influxdata/telegraf" @@ -26,6 +27,9 @@ func New( if len(name) == 0 { return nil, fmt.Errorf("Metric cannot be made with an empty name") } + if strings.HasSuffix(name, `\`) { + return nil, fmt.Errorf("Metric cannot have measurement name ending with a backslash") + } var thisType telegraf.ValueType if len(mType) > 0 { @@ -44,6 +48,13 @@ func New( // pre-allocate exact size of the tags slice taglen := 0 for k, v := range tags { + if strings.HasSuffix(k, `\`) { + return nil, fmt.Errorf("Metric cannot have tag key ending with a backslash") + } + if strings.HasSuffix(v, `\`) { + return nil, fmt.Errorf("Metric cannot have tag value ending with a backslash") + } + if len(k) == 0 || len(v) == 0 { continue } @@ -66,7 +77,17 @@ func New( // pre-allocate capacity of the fields slice fieldlen := 0 - for k, _ := range fields { + for k, v := range fields { + if strings.HasSuffix(k, `\`) { + return nil, fmt.Errorf("Metric cannot have field key ending with a backslash") + } + switch val := v.(type) { + case string: + if strings.HasSuffix(val, `\`) { + return nil, fmt.Errorf("Metric cannot have field value ending with a backslash") + } + } + // 10 bytes is completely arbitrary, but will at least prevent some // amount of allocations. There's a small possibility this will create // slightly more allocations for a metric that has many short fields. diff --git a/metric/metric_test.go b/metric/metric_test.go index c154549f3..e875a213f 100644 --- a/metric/metric_test.go +++ b/metric/metric_test.go @@ -687,3 +687,55 @@ func TestEmptyTagValueOrKey(t *testing.T) { assert.NoError(t, err) } + +func TestNewMetric_TrailingSlash(t *testing.T) { + now := time.Now() + + tests := []struct { + name string + tags map[string]string + fields map[string]interface{} + }{ + { + name: `cpu\`, + fields: map[string]interface{}{ + "value": int64(42), + }, + }, + { + name: "cpu", + fields: map[string]interface{}{ + `value\`: "x", + }, + }, + { + name: "cpu", + fields: map[string]interface{}{ + "value": `x\`, + }, + }, + { + name: "cpu", + tags: map[string]string{ + `host\`: "localhost", + }, + fields: map[string]interface{}{ + "value": int64(42), + }, + }, + { + name: "cpu", + tags: map[string]string{ + "host": `localhost\`, + }, + fields: map[string]interface{}{ + "value": int64(42), + }, + }, + } + + for _, tc := range tests { + _, err := New(tc.name, tc.tags, tc.fields, now) + assert.Error(t, err) + } +}