From 2ea573e227605331a4ad3286e18d5626e00375ea Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Thu, 21 Dec 2023 08:53:23 -0800 Subject: [PATCH] [confmap] Reject negative values decoded into uints This adds a decode hook for unmarshalling negative integers into uint types. This will now return an error instead of converting negative values into large uint values. --- .chloggen/fix_negative_uint_config.yaml | 25 +++++++++ confmap/confmap.go | 13 +++++ confmap/confmap_test.go | 53 +++++++++++++++++++ .../memorylimiterprocessor/config_test.go | 11 ++++ .../negative_unsigned_limits_config.yaml | 13 +++++ 5 files changed, 115 insertions(+) create mode 100755 .chloggen/fix_negative_uint_config.yaml create mode 100644 processor/memorylimiterprocessor/testdata/negative_unsigned_limits_config.yaml diff --git a/.chloggen/fix_negative_uint_config.yaml b/.chloggen/fix_negative_uint_config.yaml new file mode 100755 index 00000000000..7d9f74de78f --- /dev/null +++ b/.chloggen/fix_negative_uint_config.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: confmap + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Fix decoding negative configuration values into uints + +# One or more tracking issues or pull requests related to the change +issues: [9060] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] \ No newline at end of file diff --git a/confmap/confmap.go b/confmap/confmap.go index 254f0813433..9f41dbcebed 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -158,6 +158,7 @@ func decodeConfig(m *Conf, result any, errorUnused bool) error { mapstructure.TextUnmarshallerHookFunc(), unmarshalerHookFunc(result), zeroSliceHookFunc(), + negativeUintHookFunc(), ), } decoder, err := mapstructure.NewDecoder(dc) @@ -368,3 +369,15 @@ func zeroSliceHookFunc() mapstructure.DecodeHookFuncValue { return from.Interface(), nil } } + +// This hook is used to solve the issue: https://github.com/open-telemetry/opentelemetry-collector/issues/9060 +// Decoding should fail when converting a negative integer to any type of unsigned integer. This prevents +// negative values being decoded as large uint values. +func negativeUintHookFunc() mapstructure.DecodeHookFuncValue { + return func(from reflect.Value, to reflect.Value) (interface{}, error) { + if from.CanInt() && from.Int() < 0 && to.CanUint() { + return nil, fmt.Errorf("cannot convert negative value %v to uint", from.Int()) + } + return from.Interface(), nil + } +} diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index 211eca429aa..fa9c404f544 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -5,6 +5,7 @@ package confmap import ( "errors" + "fmt" "os" "path/filepath" "strings" @@ -212,6 +213,58 @@ func TestMapKeyStringToMapKeyTextUnmarshalerHookFunc(t *testing.T) { assert.Equal(t, map[TestID]string{"string": "this is a string"}, cfg.Map) } +type UintConfig struct { + UintTest uint32 `mapstructure:"uint_test"` +} + +func TestUintUnmarshalerSuccess(t *testing.T) { + testValue := 1000 + stringMap := map[string]any{ + "uint_test": testValue, + } + + tests := []struct { + name string + testValue int + }{ + { + name: "Test convert 0 to uint", + testValue: 0, + }, + { + name: "Test positive uint conversion", + testValue: 1000, + }, + } + + for _, tt := range tests { + tt := tt + + t.Run(tt.name, func(t *testing.T) { + conf := NewFromStringMap(stringMap) + cfg := &UintConfig{} + err := conf.Unmarshal(cfg) + + assert.NoError(t, err) + assert.Equal(t, cfg.UintTest, uint32(testValue)) + }) + } + +} + +func TestUintUnmarshalerFailure(t *testing.T) { + testValue := -1000 + stringMap := map[string]any{ + "uint_test": testValue, + } + conf := NewFromStringMap(stringMap) + cfg := &UintConfig{} + err := conf.Unmarshal(cfg) + + assert.Error(t, err) + assert.Contains(t, err.Error(), fmt.Sprintf("* error decoding 'uint_test': cannot convert negative value %v to uint", testValue)) +} + func TestMapKeyStringToMapKeyTextUnmarshalerHookFuncDuplicateID(t *testing.T) { stringMap := map[string]any{ "bool": true, diff --git a/processor/memorylimiterprocessor/config_test.go b/processor/memorylimiterprocessor/config_test.go index d4d2e787b85..43768b73f64 100644 --- a/processor/memorylimiterprocessor/config_test.go +++ b/processor/memorylimiterprocessor/config_test.go @@ -104,3 +104,14 @@ func TestConfigValidate(t *testing.T) { }) } } + +func TestUnmarshalInvalidConfig(t *testing.T) { + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "negative_unsigned_limits_config.yaml")) + require.NoError(t, err) + factory := NewFactory() + cfg := factory.CreateDefaultConfig() + err = component.UnmarshalConfig(cm, cfg) + require.Error(t, err) + require.Contains(t, err.Error(), "error decoding 'limit_mib': cannot convert negative value -2000 to uint") + require.Contains(t, err.Error(), "error decoding 'spike_limit_mib': cannot convert negative value -2300 to uint") +} diff --git a/processor/memorylimiterprocessor/testdata/negative_unsigned_limits_config.yaml b/processor/memorylimiterprocessor/testdata/negative_unsigned_limits_config.yaml new file mode 100644 index 00000000000..3a2ef3a5b99 --- /dev/null +++ b/processor/memorylimiterprocessor/testdata/negative_unsigned_limits_config.yaml @@ -0,0 +1,13 @@ +# check_interval is the time between measurements of memory usage for the +# purposes of avoiding going over the limits. Defaults to zero, so no +# checks will be performed. Values below 1 second are not recommended since +# it can result in unnecessary CPU consumption. +check_interval: 5s + +# Maximum amount of memory, in MiB, targeted to be allocated by the process heap. +# Note that typically the total memory usage of process will be about 50MiB higher +# than this value. +limit_mib: -2000 + +# The maximum, in MiB, spike expected between the measurements of memory usage. +spike_limit_mib: -2300