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 9817694dd63..f3661dbb9ce 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -168,6 +168,7 @@ func decodeConfig(m *Conf, result any, errorUnused bool) error { mapstructure.TextUnmarshallerHookFunc(), unmarshalerHookFunc(result), zeroSliceHookFunc(), + negativeUintHookFunc(), ), } decoder, err := mapstructure.NewDecoder(dc) @@ -378,3 +379,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 7cfab3fb4cb..7a95faead09 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