From 02ddb32b8aa0c10bc9dd6569619d93ea898049e6 Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Thu, 21 Dec 2023 08:53:23 -0800 Subject: [PATCH 1/6] [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 +++++++++++++++++++ internal/memorylimiter/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 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 be5360e9d29..efffe43572f 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -163,6 +163,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) @@ -415,3 +416,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 e0468af3ac6..a7a63653087 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,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/internal/memorylimiter/config_test.go b/internal/memorylimiter/config_test.go index ca7a243f861..a81d8537aca 100644 --- a/internal/memorylimiter/config_test.go +++ b/internal/memorylimiter/config_test.go @@ -93,3 +93,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/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 From e29f9b7a08a9f2c0eec1ae378d42cb3f49f4b6bb Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Thu, 21 Dec 2023 16:07:09 -0800 Subject: [PATCH 2/6] Changes requested in PR - Add test case for uint64(-1000) conversion - Clean existing test case --- confmap/confmap_test.go | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index a7a63653087..e57f3ea5168 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -219,11 +219,6 @@ type UintConfig struct { } func TestUintUnmarshalerSuccess(t *testing.T) { - testValue := 1000 - stringMap := map[string]any{ - "uint_test": testValue, - } - tests := []struct { name string testValue int @@ -242,15 +237,36 @@ func TestUintUnmarshalerSuccess(t *testing.T) { 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(testValue)) + 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) { From e5dc9d683153531835e6e185f514963d60a9f3e8 Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Mon, 5 Feb 2024 09:17:21 -0800 Subject: [PATCH 3/6] Fix failing test --- internal/memorylimiter/config_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/memorylimiter/config_test.go b/internal/memorylimiter/config_test.go index a81d8537aca..5be5d9d6bee 100644 --- a/internal/memorylimiter/config_test.go +++ b/internal/memorylimiter/config_test.go @@ -97,8 +97,7 @@ 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() + 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 uint") From de96bcb884e100f16602abfd18da91a76be6c8c2 Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Fri, 22 Mar 2024 10:47:07 -0700 Subject: [PATCH 4/6] Change wording from "uint" to "unsigned integer" --- confmap/confmap.go | 2 +- confmap/confmap_test.go | 2 +- internal/memorylimiter/config_test.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/confmap/confmap.go b/confmap/confmap.go index efffe43572f..00a62593cd5 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -423,7 +423,7 @@ func zeroSliceHookFunc() mapstructure.DecodeHookFuncValue { 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 nil, fmt.Errorf("cannot convert negative value %v to an unsigned integer", from.Int()) } return from.Interface(), nil } diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index e57f3ea5168..2bd676df182 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -279,7 +279,7 @@ func TestUintUnmarshalerFailure(t *testing.T) { 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)) + 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) { diff --git a/internal/memorylimiter/config_test.go b/internal/memorylimiter/config_test.go index 5be5d9d6bee..29db1dc6def 100644 --- a/internal/memorylimiter/config_test.go +++ b/internal/memorylimiter/config_test.go @@ -100,6 +100,6 @@ func TestUnmarshalInvalidConfig(t *testing.T) { 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 uint") - require.Contains(t, err.Error(), "error decoding 'spike_limit_mib': cannot convert negative value -2300 to uint") + 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") } From becb1570ec0c44b7a46a7eb6131676ad87087c60 Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Thu, 18 Apr 2024 09:38:38 -0700 Subject: [PATCH 5/6] Add missing brace --- confmap/confmap.go | 1 + 1 file changed, 1 insertion(+) diff --git a/confmap/confmap.go b/confmap/confmap.go index 08e370a566f..a34967ef7e4 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -428,6 +428,7 @@ func negativeUintHookFunc() mapstructure.DecodeHookFuncValue { 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 { From 54a3d4df6897345b8d56175d60f6801bede996ab Mon Sep 17 00:00:00 2001 From: Curtis Robert Date: Thu, 18 Apr 2024 09:40:53 -0700 Subject: [PATCH 6/6] Add TODO to remove hook when unmarshalling is strict --- confmap/confmap.go | 1 + 1 file changed, 1 insertion(+) diff --git a/confmap/confmap.go b/confmap/confmap.go index a34967ef7e4..f3e0471e1d6 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -422,6 +422,7 @@ 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() {