From 4a5809293b286f39d1508e69e760e4b00d5da7eb Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Fri, 19 Apr 2024 04:48:32 -0700 Subject: [PATCH] [confmap] Return error when decoding negative values into uints (#9169) **Description:** 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. **Link to tracking Issue:** Fixes #9060 **Testing:** Added unit tests for confmap functionality, functional tests in memory limiter processor (the original component this issue was filed against) --- .chloggen/fix_negative_uint_config.yaml | 25 +++++++ confmap/confmap.go | 14 ++++ confmap/confmap_test.go | 69 +++++++++++++++++++ internal/memorylimiter/config_test.go | 10 +++ .../negative_unsigned_limits_config.yaml | 13 ++++ 5 files changed, 131 insertions(+) create mode 100755 .chloggen/fix_negative_uint_config.yaml create mode 100644 internal/memorylimiter/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 9e602a62689..f3e0471e1d6 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -164,6 +164,7 @@ func decodeConfig(m *Conf, result any, errorUnused bool) error { // we unmarshal the embedded structs if present to merge with the result: unmarshalerEmbeddedStructsHookFunc(), zeroSliceHookFunc(), + negativeUintHookFunc(), ), } decoder, err := mapstructure.NewDecoder(dc) @@ -418,6 +419,19 @@ func zeroSliceHookFunc() mapstructure.DecodeHookFuncValue { } } +// 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. +// TODO: This should be removed as a part of https://github.com/open-telemetry/opentelemetry-collector/issues/9532 +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 an unsigned integer", from.Int()) + } + return from.Interface(), nil + } +} + type moduleFactory[T any, S any] interface { Create(s S) T } diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index c334b7d18b9..a1ea94e1fe4 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -5,6 +5,7 @@ package confmap import ( "errors" + "fmt" "os" "path/filepath" "strings" @@ -213,6 +214,74 @@ 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) { + 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) { + stringMap := map[string]any{ + "uint_test": tt.testValue, + } + conf := NewFromStringMap(stringMap) + cfg := &UintConfig{} + err := conf.Unmarshal(cfg) + + assert.NoError(t, err) + assert.Equal(t, cfg.UintTest, uint32(tt.testValue)) + }) + } +} + +func TestUint64Unmarshaler(t *testing.T) { + negativeInt := -1000 + testValue := uint64(negativeInt) + + type Uint64Config struct { + UintTest uint64 `mapstructure:"uint_test"` + } + stringMap := map[string]any{ + "uint_test": testValue, + } + + conf := NewFromStringMap(stringMap) + cfg := &Uint64Config{} + err := conf.Unmarshal(cfg) + + assert.NoError(t, err) + assert.Equal(t, cfg.UintTest, 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 an unsigned integer", testValue)) +} + func TestMapKeyStringToMapKeyTextUnmarshalerHookFuncDuplicateID(t *testing.T) { stringMap := map[string]any{ "bool": true, diff --git a/internal/memorylimiter/config_test.go b/internal/memorylimiter/config_test.go index ca7a243f861..29db1dc6def 100644 --- a/internal/memorylimiter/config_test.go +++ b/internal/memorylimiter/config_test.go @@ -93,3 +93,13 @@ 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) + cfg := &Config{} + err = component.UnmarshalConfig(cm, cfg) + require.Error(t, err) + require.Contains(t, err.Error(), "error decoding 'limit_mib': cannot convert negative value -2000 to an unsigned integer") + require.Contains(t, err.Error(), "error decoding 'spike_limit_mib': cannot convert negative value -2300 to an unsigned integer") +} diff --git a/internal/memorylimiter/testdata/negative_unsigned_limits_config.yaml b/internal/memorylimiter/testdata/negative_unsigned_limits_config.yaml new file mode 100644 index 00000000000..3a2ef3a5b99 --- /dev/null +++ b/internal/memorylimiter/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