From 9249dab5b443bcca234c5d0454b20807977d0ccd Mon Sep 17 00:00:00 2001 From: Thomas Pelletier Date: Thu, 21 Aug 2025 11:12:38 +0200 Subject: [PATCH 1/5] Handle array table into an empty slice Fix #995 --- unmarshaler.go | 46 ++++++++++++++++++++++-------- unmarshaler_test.go | 68 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 11 deletions(-) diff --git a/unmarshaler.go b/unmarshaler.go index eda6dd33..5ee2e21d 100644 --- a/unmarshaler.go +++ b/unmarshaler.go @@ -415,17 +415,41 @@ func (d *decoder) handleArrayTableCollection(key unstable.Iterator, v reflect.Va } return v, nil - case reflect.Slice: - elem := v.Index(v.Len() - 1) - x, err := d.handleArrayTable(key, elem) - if err != nil || d.skipUntilTable { - return reflect.Value{}, err - } - if x.IsValid() { - elem.Set(x) - } - - return v, err + case reflect.Slice: + // Create a new element when the slice is empty; otherwise operate on + // the last element. Call handleArrayTable once to avoid duplication. + var ( + elem reflect.Value + created bool + ) + if v.Len() == 0 { + created = true + elemType := v.Type().Elem() + if elemType.Kind() == reflect.Interface { + elem = makeMapStringInterface() + } else { + elem = reflect.New(elemType).Elem() + } + } else { + elem = v.Index(v.Len() - 1) + } + + x, err := d.handleArrayTable(key, elem) + if err != nil || d.skipUntilTable { + return reflect.Value{}, err + } + if x.IsValid() { + if created { + elem = x + } else { + elem.Set(x) + } + } + + if created { + return reflect.Append(v, elem), nil + } + return v, err case reflect.Array: idx := d.arrayIndex(false, v) if idx >= v.Len() { diff --git a/unmarshaler_test.go b/unmarshaler_test.go index fa386858..728efb6e 100644 --- a/unmarshaler_test.go +++ b/unmarshaler_test.go @@ -4040,3 +4040,71 @@ func TestIssue994_OK(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "bar from unmarshaler", d.S) } + +func TestIssue995(t *testing.T) { + // Reproduces https://github.com/pelletier/go-toml/issues/995 + // The decoder used to panic with "reflect: slice index out of range" + // when encountering a nested array table like [[rules.allowlists]] while + // the parent slice (Rules) was still empty. + + type AllowList struct { + Description string + Condition string + Commits []string + Paths []string + RegexTarget string + Regexes []string + StopWords []string + } + + type Rule struct { + ID string + Description string + Regex string + SecretGroup int + Entropy interface{} + Keywords []string + Path string + Tags []string + AllowList *AllowList + Allowlists []AllowList + } + + type GitleaksConfig struct { + Description string + Rules []Rule + Allowlist struct { + Commits []string + Paths []string + RegexTarget string + Regexes []string + StopWords []string + } + } + + doc := ` +[[allowlists]] + description = "Exception for File " + files = [ '''app/src'''] + +[[rules.allowlists]] + description = "policies" + regexes = [ + '''abc''' + ] +` + + var cfg GitleaksConfig + err := toml.Unmarshal([]byte(doc), &cfg) + assert.NoError(t, err) + + // Ensure no panic and that nested array table was created. + if len(cfg.Rules) == 0 { + t.Fatalf("expected Rules to contain at least one element after unmarshaling nested array table") + } + if len(cfg.Rules[0].Allowlists) != 1 { + t.Fatalf("expected first Rule to have exactly one allowlists entry, got %d", len(cfg.Rules[0].Allowlists)) + } + assert.Equal(t, "policies", cfg.Rules[0].Allowlists[0].Description) + assert.Equal(t, []string{"abc"}, cfg.Rules[0].Allowlists[0].Regexes) +} From bd8dbb594e85bb1a7ef6452787e754e0a0d329a3 Mon Sep 17 00:00:00 2001 From: Thomas Pelletier Date: Thu, 21 Aug 2025 11:32:25 +0200 Subject: [PATCH 2/5] fmtg --- unmarshaler.go | 70 ++++++++++++++-------------- unmarshaler_test.go | 108 ++++++++++++++++++++++---------------------- 2 files changed, 89 insertions(+), 89 deletions(-) diff --git a/unmarshaler.go b/unmarshaler.go index 5ee2e21d..d0df35ec 100644 --- a/unmarshaler.go +++ b/unmarshaler.go @@ -415,41 +415,41 @@ func (d *decoder) handleArrayTableCollection(key unstable.Iterator, v reflect.Va } return v, nil - case reflect.Slice: - // Create a new element when the slice is empty; otherwise operate on - // the last element. Call handleArrayTable once to avoid duplication. - var ( - elem reflect.Value - created bool - ) - if v.Len() == 0 { - created = true - elemType := v.Type().Elem() - if elemType.Kind() == reflect.Interface { - elem = makeMapStringInterface() - } else { - elem = reflect.New(elemType).Elem() - } - } else { - elem = v.Index(v.Len() - 1) - } - - x, err := d.handleArrayTable(key, elem) - if err != nil || d.skipUntilTable { - return reflect.Value{}, err - } - if x.IsValid() { - if created { - elem = x - } else { - elem.Set(x) - } - } - - if created { - return reflect.Append(v, elem), nil - } - return v, err + case reflect.Slice: + // Create a new element when the slice is empty; otherwise operate on + // the last element. + var ( + elem reflect.Value + created bool + ) + if v.Len() == 0 { + created = true + elemType := v.Type().Elem() + if elemType.Kind() == reflect.Interface { + elem = makeMapStringInterface() + } else { + elem = reflect.New(elemType).Elem() + } + } else { + elem = v.Index(v.Len() - 1) + } + + x, err := d.handleArrayTable(key, elem) + if err != nil || d.skipUntilTable { + return reflect.Value{}, err + } + if x.IsValid() { + if created { + elem = x + } else { + elem.Set(x) + } + } + + if created { + return reflect.Append(v, elem), nil + } + return v, err case reflect.Array: idx := d.arrayIndex(false, v) if idx >= v.Len() { diff --git a/unmarshaler_test.go b/unmarshaler_test.go index 728efb6e..8f04886c 100644 --- a/unmarshaler_test.go +++ b/unmarshaler_test.go @@ -4042,47 +4042,47 @@ func TestIssue994_OK(t *testing.T) { } func TestIssue995(t *testing.T) { - // Reproduces https://github.com/pelletier/go-toml/issues/995 - // The decoder used to panic with "reflect: slice index out of range" - // when encountering a nested array table like [[rules.allowlists]] while - // the parent slice (Rules) was still empty. - - type AllowList struct { - Description string - Condition string - Commits []string - Paths []string - RegexTarget string - Regexes []string - StopWords []string - } - - type Rule struct { - ID string - Description string - Regex string - SecretGroup int - Entropy interface{} - Keywords []string - Path string - Tags []string - AllowList *AllowList - Allowlists []AllowList - } - - type GitleaksConfig struct { - Description string - Rules []Rule - Allowlist struct { - Commits []string - Paths []string - RegexTarget string - Regexes []string - StopWords []string - } - } - - doc := ` + // Reproduces https://github.com/pelletier/go-toml/issues/995 + // The decoder used to panic with "reflect: slice index out of range" + // when encountering a nested array table like [[rules.allowlists]] while + // the parent slice (Rules) was still empty. + + type AllowList struct { + Description string + Condition string + Commits []string + Paths []string + RegexTarget string + Regexes []string + StopWords []string + } + + type Rule struct { + ID string + Description string + Regex string + SecretGroup int + Entropy interface{} + Keywords []string + Path string + Tags []string + AllowList *AllowList + Allowlists []AllowList + } + + type GitleaksConfig struct { + Description string + Rules []Rule + Allowlist struct { + Commits []string + Paths []string + RegexTarget string + Regexes []string + StopWords []string + } + } + + doc := ` [[allowlists]] description = "Exception for File " files = [ '''app/src'''] @@ -4094,17 +4094,17 @@ func TestIssue995(t *testing.T) { ] ` - var cfg GitleaksConfig - err := toml.Unmarshal([]byte(doc), &cfg) - assert.NoError(t, err) - - // Ensure no panic and that nested array table was created. - if len(cfg.Rules) == 0 { - t.Fatalf("expected Rules to contain at least one element after unmarshaling nested array table") - } - if len(cfg.Rules[0].Allowlists) != 1 { - t.Fatalf("expected first Rule to have exactly one allowlists entry, got %d", len(cfg.Rules[0].Allowlists)) - } - assert.Equal(t, "policies", cfg.Rules[0].Allowlists[0].Description) - assert.Equal(t, []string{"abc"}, cfg.Rules[0].Allowlists[0].Regexes) + var cfg GitleaksConfig + err := toml.Unmarshal([]byte(doc), &cfg) + assert.NoError(t, err) + + // Ensure no panic and that nested array table was created. + if len(cfg.Rules) == 0 { + t.Fatalf("expected Rules to contain at least one element after unmarshaling nested array table") + } + if len(cfg.Rules[0].Allowlists) != 1 { + t.Fatalf("expected first Rule to have exactly one allowlists entry, got %d", len(cfg.Rules[0].Allowlists)) + } + assert.Equal(t, "policies", cfg.Rules[0].Allowlists[0].Description) + assert.Equal(t, []string{"abc"}, cfg.Rules[0].Allowlists[0].Regexes) } From e0cdce7c5a5d5758817bf569c7e3a5e80f0a9aca Mon Sep 17 00:00:00 2001 From: Thomas Pelletier Date: Thu, 21 Aug 2025 11:33:15 +0200 Subject: [PATCH 3/5] Remove comment --- unmarshaler_test.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/unmarshaler_test.go b/unmarshaler_test.go index 8f04886c..da95017b 100644 --- a/unmarshaler_test.go +++ b/unmarshaler_test.go @@ -412,7 +412,7 @@ foo = "bar"`, assert: func(t *testing.T, test test) { // Despite the documentation: // Pointer variable equality is determined based on the equality of the - // referenced values (as opposed to the memory addresses). + // referenced values (as opposed to the memory addresses). // assert.Equal does not work properly with maps with pointer keys // https://github.com/stretchr/testify/issues/1143 expected := make(map[unmarshalTextKey]string) @@ -3884,9 +3884,9 @@ func TestUnmarshal_Nil(t *testing.T) { { desc: "simplest", input: ` - [foo] - [foo.foo] - `, + [foo] + [foo.foo] + `, expected: "[foo]\n[foo.foo]\n", }, } @@ -4042,11 +4042,6 @@ func TestIssue994_OK(t *testing.T) { } func TestIssue995(t *testing.T) { - // Reproduces https://github.com/pelletier/go-toml/issues/995 - // The decoder used to panic with "reflect: slice index out of range" - // when encountering a nested array table like [[rules.allowlists]] while - // the parent slice (Rules) was still empty. - type AllowList struct { Description string Condition string From e0a180366684372b53fbdac07bc3f63f997c92bd Mon Sep 17 00:00:00 2001 From: Thomas Pelletier Date: Thu, 21 Aug 2025 11:55:09 +0200 Subject: [PATCH 4/5] fix: avoid panic for nested array tables into empty slices; add tests for issue #995 Ensure handleArrayTableCollection initializes first slice element and calls handleArrayTable once. Add comprehensive tests covering interface and concrete slices, non-empty slices, and pointer-to-slice cases. --- unmarshaler_test.go | 136 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 136 insertions(+) diff --git a/unmarshaler_test.go b/unmarshaler_test.go index da95017b..9a111295 100644 --- a/unmarshaler_test.go +++ b/unmarshaler_test.go @@ -4103,3 +4103,139 @@ func TestIssue995(t *testing.T) { assert.Equal(t, "policies", cfg.Rules[0].Allowlists[0].Description) assert.Equal(t, []string{"abc"}, cfg.Rules[0].Allowlists[0].Regexes) } + +func TestIssue995_InterfaceSlice_MultiNested(t *testing.T) { + type Root struct { + Rules []interface{} + } + + doc := ` +[[rules.allowlists]] + description = "a" + +[[rules.allowlists]] + description = "b" +` + + var r Root + err := toml.Unmarshal([]byte(doc), &r) + assert.NoError(t, err) + + if len(r.Rules) != 1 { + t.Fatalf("expected one element in Rules, got %d", len(r.Rules)) + } + + m, ok := r.Rules[0].(map[string]interface{}) + if !ok { + t.Fatalf("expected Rules[0] to be a map[string]any, got %T", r.Rules[0]) + } + + als, ok := m["allowlists"].([]interface{}) + if !ok { + t.Fatalf("expected allowlists to be []any, got %T", m["allowlists"]) + } + if len(als) != 2 { + t.Fatalf("expected 2 allowlists entries, got %d", len(als)) + } + + a0, ok := als[0].(map[string]interface{}) + if !ok { + t.Fatalf("expected allowlists[0] to be map[string]any, got %T", als[0]) + } + a1, ok := als[1].(map[string]interface{}) + if !ok { + t.Fatalf("expected allowlists[1] to be map[string]any, got %T", als[1]) + } + assert.Equal(t, "a", a0["description"]) + assert.Equal(t, "b", a1["description"]) +} + +func TestIssue995_MultiNestedConcrete(t *testing.T) { + type AllowList struct { + Description string + } + type Rule struct { + Allowlists []AllowList + } + type Root struct { + Rules []Rule + } + + doc := ` +[[rules.allowlists]] + description = "a" + +[[rules.allowlists]] + description = "b" +` + + var r Root + err := toml.Unmarshal([]byte(doc), &r) + assert.NoError(t, err) + + if len(r.Rules) != 1 { + t.Fatalf("expected one element in Rules, got %d", len(r.Rules)) + } + assert.Equal(t, 2, len(r.Rules[0].Allowlists)) + assert.Equal(t, "a", r.Rules[0].Allowlists[0].Description) + assert.Equal(t, "b", r.Rules[0].Allowlists[1].Description) +} + +func TestIssue995_PointerToSlice_Rules(t *testing.T) { + type AllowList struct{ Description string } + type Rule struct{ Allowlists []AllowList } + type Root struct{ Rules *[]Rule } + + doc := ` +[[rules.allowlists]] + description = "a" + +[[rules.allowlists]] + description = "b" +` + + var r Root + err := toml.Unmarshal([]byte(doc), &r) + assert.NoError(t, err) + if r.Rules == nil { + t.Fatalf("expected Rules pointer to be initialized") + } + if len(*r.Rules) != 1 { + t.Fatalf("expected one element in Rules, got %d", len(*r.Rules)) + } + rule := (*r.Rules)[0] + assert.Equal(t, 2, len(rule.Allowlists)) + assert.Equal(t, "a", rule.Allowlists[0].Description) + assert.Equal(t, "b", rule.Allowlists[1].Description) +} + +func TestIssue995_SliceNonEmpty_UsesLastElement(t *testing.T) { + type AllowList struct{ Description string } + type Rule struct{ Allowlists []AllowList } + type Root struct{ Rules []Rule } + + // Pre-initialize with one Rule; nested array table should populate + // the last element, not create a new one at this level. + var r Root + r.Rules = []Rule{{}} + + doc := ` +[[rules.allowlists]] + description = "a" + +[[rules.allowlists]] + description = "b" +` + + err := toml.Unmarshal([]byte(doc), &r) + assert.NoError(t, err) + if len(r.Rules) != 1 { + t.Fatalf("expected one element in Rules, got %d", len(r.Rules)) + } + assert.Equal(t, 2, len(r.Rules[0].Allowlists)) + // Values presence check + got := []string{r.Rules[0].Allowlists[0].Description, r.Rules[0].Allowlists[1].Description} + if !(got[0] == "a" && got[1] == "b") && !(got[0] == "b" && got[1] == "a") { + t.Fatalf("unexpected values in allowlists: %v", got) + } +} From f0e5c3b2b84a6d3a5bff980245452b832738b57a Mon Sep 17 00:00:00 2001 From: Thomas Pelletier Date: Thu, 21 Aug 2025 11:56:17 +0200 Subject: [PATCH 5/5] fmt --- unmarshaler_test.go | 182 ++++++++++++++++++++++---------------------- 1 file changed, 91 insertions(+), 91 deletions(-) diff --git a/unmarshaler_test.go b/unmarshaler_test.go index 9a111295..d5663130 100644 --- a/unmarshaler_test.go +++ b/unmarshaler_test.go @@ -4105,11 +4105,11 @@ func TestIssue995(t *testing.T) { } func TestIssue995_InterfaceSlice_MultiNested(t *testing.T) { - type Root struct { - Rules []interface{} - } + type Root struct { + Rules []interface{} + } - doc := ` + doc := ` [[rules.allowlists]] description = "a" @@ -4117,51 +4117,51 @@ func TestIssue995_InterfaceSlice_MultiNested(t *testing.T) { description = "b" ` - var r Root - err := toml.Unmarshal([]byte(doc), &r) - assert.NoError(t, err) - - if len(r.Rules) != 1 { - t.Fatalf("expected one element in Rules, got %d", len(r.Rules)) - } - - m, ok := r.Rules[0].(map[string]interface{}) - if !ok { - t.Fatalf("expected Rules[0] to be a map[string]any, got %T", r.Rules[0]) - } - - als, ok := m["allowlists"].([]interface{}) - if !ok { - t.Fatalf("expected allowlists to be []any, got %T", m["allowlists"]) - } - if len(als) != 2 { - t.Fatalf("expected 2 allowlists entries, got %d", len(als)) - } - - a0, ok := als[0].(map[string]interface{}) - if !ok { - t.Fatalf("expected allowlists[0] to be map[string]any, got %T", als[0]) - } - a1, ok := als[1].(map[string]interface{}) - if !ok { - t.Fatalf("expected allowlists[1] to be map[string]any, got %T", als[1]) - } - assert.Equal(t, "a", a0["description"]) - assert.Equal(t, "b", a1["description"]) + var r Root + err := toml.Unmarshal([]byte(doc), &r) + assert.NoError(t, err) + + if len(r.Rules) != 1 { + t.Fatalf("expected one element in Rules, got %d", len(r.Rules)) + } + + m, ok := r.Rules[0].(map[string]interface{}) + if !ok { + t.Fatalf("expected Rules[0] to be a map[string]any, got %T", r.Rules[0]) + } + + als, ok := m["allowlists"].([]interface{}) + if !ok { + t.Fatalf("expected allowlists to be []any, got %T", m["allowlists"]) + } + if len(als) != 2 { + t.Fatalf("expected 2 allowlists entries, got %d", len(als)) + } + + a0, ok := als[0].(map[string]interface{}) + if !ok { + t.Fatalf("expected allowlists[0] to be map[string]any, got %T", als[0]) + } + a1, ok := als[1].(map[string]interface{}) + if !ok { + t.Fatalf("expected allowlists[1] to be map[string]any, got %T", als[1]) + } + assert.Equal(t, "a", a0["description"]) + assert.Equal(t, "b", a1["description"]) } func TestIssue995_MultiNestedConcrete(t *testing.T) { - type AllowList struct { - Description string - } - type Rule struct { - Allowlists []AllowList - } - type Root struct { - Rules []Rule - } - - doc := ` + type AllowList struct { + Description string + } + type Rule struct { + Allowlists []AllowList + } + type Root struct { + Rules []Rule + } + + doc := ` [[rules.allowlists]] description = "a" @@ -4169,24 +4169,24 @@ func TestIssue995_MultiNestedConcrete(t *testing.T) { description = "b" ` - var r Root - err := toml.Unmarshal([]byte(doc), &r) - assert.NoError(t, err) + var r Root + err := toml.Unmarshal([]byte(doc), &r) + assert.NoError(t, err) - if len(r.Rules) != 1 { - t.Fatalf("expected one element in Rules, got %d", len(r.Rules)) - } - assert.Equal(t, 2, len(r.Rules[0].Allowlists)) - assert.Equal(t, "a", r.Rules[0].Allowlists[0].Description) - assert.Equal(t, "b", r.Rules[0].Allowlists[1].Description) + if len(r.Rules) != 1 { + t.Fatalf("expected one element in Rules, got %d", len(r.Rules)) + } + assert.Equal(t, 2, len(r.Rules[0].Allowlists)) + assert.Equal(t, "a", r.Rules[0].Allowlists[0].Description) + assert.Equal(t, "b", r.Rules[0].Allowlists[1].Description) } func TestIssue995_PointerToSlice_Rules(t *testing.T) { - type AllowList struct{ Description string } - type Rule struct{ Allowlists []AllowList } - type Root struct{ Rules *[]Rule } + type AllowList struct{ Description string } + type Rule struct{ Allowlists []AllowList } + type Root struct{ Rules *[]Rule } - doc := ` + doc := ` [[rules.allowlists]] description = "a" @@ -4194,32 +4194,32 @@ func TestIssue995_PointerToSlice_Rules(t *testing.T) { description = "b" ` - var r Root - err := toml.Unmarshal([]byte(doc), &r) - assert.NoError(t, err) - if r.Rules == nil { - t.Fatalf("expected Rules pointer to be initialized") - } - if len(*r.Rules) != 1 { - t.Fatalf("expected one element in Rules, got %d", len(*r.Rules)) - } - rule := (*r.Rules)[0] - assert.Equal(t, 2, len(rule.Allowlists)) - assert.Equal(t, "a", rule.Allowlists[0].Description) - assert.Equal(t, "b", rule.Allowlists[1].Description) + var r Root + err := toml.Unmarshal([]byte(doc), &r) + assert.NoError(t, err) + if r.Rules == nil { + t.Fatalf("expected Rules pointer to be initialized") + } + if len(*r.Rules) != 1 { + t.Fatalf("expected one element in Rules, got %d", len(*r.Rules)) + } + rule := (*r.Rules)[0] + assert.Equal(t, 2, len(rule.Allowlists)) + assert.Equal(t, "a", rule.Allowlists[0].Description) + assert.Equal(t, "b", rule.Allowlists[1].Description) } func TestIssue995_SliceNonEmpty_UsesLastElement(t *testing.T) { - type AllowList struct{ Description string } - type Rule struct{ Allowlists []AllowList } - type Root struct{ Rules []Rule } + type AllowList struct{ Description string } + type Rule struct{ Allowlists []AllowList } + type Root struct{ Rules []Rule } - // Pre-initialize with one Rule; nested array table should populate - // the last element, not create a new one at this level. - var r Root - r.Rules = []Rule{{}} + // Pre-initialize with one Rule; nested array table should populate + // the last element, not create a new one at this level. + var r Root + r.Rules = []Rule{{}} - doc := ` + doc := ` [[rules.allowlists]] description = "a" @@ -4227,15 +4227,15 @@ func TestIssue995_SliceNonEmpty_UsesLastElement(t *testing.T) { description = "b" ` - err := toml.Unmarshal([]byte(doc), &r) - assert.NoError(t, err) - if len(r.Rules) != 1 { - t.Fatalf("expected one element in Rules, got %d", len(r.Rules)) - } - assert.Equal(t, 2, len(r.Rules[0].Allowlists)) - // Values presence check - got := []string{r.Rules[0].Allowlists[0].Description, r.Rules[0].Allowlists[1].Description} - if !(got[0] == "a" && got[1] == "b") && !(got[0] == "b" && got[1] == "a") { - t.Fatalf("unexpected values in allowlists: %v", got) - } + err := toml.Unmarshal([]byte(doc), &r) + assert.NoError(t, err) + if len(r.Rules) != 1 { + t.Fatalf("expected one element in Rules, got %d", len(r.Rules)) + } + assert.Equal(t, 2, len(r.Rules[0].Allowlists)) + // Values presence check + got := []string{r.Rules[0].Allowlists[0].Description, r.Rules[0].Allowlists[1].Description} + if !(got[0] == "a" && got[1] == "b") && !(got[0] == "b" && got[1] == "a") { + t.Fatalf("unexpected values in allowlists: %v", got) + } }