a70d85c fix(decode): bound map element storage and keep errors deterministic · kvconf

M decode.go +26 -17


@@ -344,8 +344,13 @@ type pendingName struct {
344	344		set bool // at least one key was accepted
345	345		}
346	346
347		-	// linearNames is how many names are searched linearly before index is built.
348		-	const linearNames = 16
	347	+	const (
	348	+	// linearNames is how many names are searched linearly before index
	349	+	// is built.
	350	+	linearNames = 16
	351	+	// maxElemHint caps the elements of a map of structs allocated up front.
	352	+	maxElemHint = 64
	353	+	)
349	354
350	355		// mapFor returns the state of the map field f. The first call sets every
351	356		// map's hint from the entries left, so a config without map keys never

@@ -366,10 +371,21 @@ func (st decodeState) mapFor(s schema, f field) mapState {
366	371		ms := &st.maps[f.slot]
367	372		if !ms.m.IsValid() {
368	373		ms.m = st.root.FieldByIndex(f.index)
369		-	if f.elem == nil {
370		-	if ms.m.IsNil() {
371		-	ms.m.Set(reflect.MakeMapWithSize(f.typ, ms.hint))
	374	+	if f.elem != nil {
	375	+	// A name sets some of its fields: guess half of them per name,
	376	+	// capped, as elements are whole structs. elems grows if needed.
	377	+	ms.hint = min(2*ms.hint/max(len(f.elem.order), 1)+1, maxElemHint)
	378	+	}
	379	+	if ms.m.IsNil() {
	380	+	// Created even if every key is rejected, as before a strict error.
	381	+	// A map of structs gets its names in flush: hint is only a guess.
	382	+	size := ms.hint
	383	+	if f.elem != nil {
	384	+	size = 0
372	385		}
	386	+	ms.m.Set(reflect.MakeMapWithSize(f.typ, size))
	387	+	}
	388	+	if f.elem == nil {
373	389		if f.typ == stringMapType && f.val.plain() {
374	390		ms.plain = ms.m.Interface().(map[string]string)
375	391		} else {

@@ -398,9 +414,12 @@ func (ms mapState) element(f field, name string) int {
398	414		}
399	415		i := len(ms.names)
400	416		if !ms.elems.IsValid() {
401		-	// Every name comes from a key counted in hint: elems never grows.
402	417		ms.elems = reflect.MakeSlice(f.elems, ms.hint, ms.hint)
403	418		ms.names = make([]pendingName, 0, ms.hint)
	419	+	} else if i == ms.elems.Len() {
	420	+	grown := reflect.MakeSlice(f.elems, 2i, 2i)
	421	+	reflect.Copy(grown, ms.elems)
	422	+	ms.elems = grown
404	423		}
405	424		ms.names = append(ms.names, pendingName{name: name})
406	425		if ms.index != nil {

@@ -421,23 +440,13 @@ func (ms mapState) element(f field, name string) int {
421	440		return i
422	441		}
423	442
424		-	// flush writes the pending struct elements back to their maps. A map that
425		-	// a key reached exists afterwards, even when every key was rejected.
	443	+	// flush writes the pending struct elements back to their maps.
426	444		func (st decodeState) flush(s schema) {
427	445		for slot := range st.maps {
428	446		ms, f := &st.maps[slot], s.prefixes[slot]
429	447		if f.elem == nil \|\| !ms.m.IsValid() {
430	448		continue
431	449		}
432		-	if ms.m.IsNil() {
433		-	n := 0
434		-	for _, p := range ms.names {
435		-	if p.set {
436		-	n++
437		-	}
438		-	}
439		-	ms.m.Set(reflect.MakeMapWithSize(f.typ, n))
440		-	}
441	450		for i, p := range ms.names {
442	451		if p.set {
443	452		ms.key.SetString(p.name)

 	set  bool // at least one key was accepted
 }
-// linearNames is how many names are searched linearly before index is built.
+const (
-const linearNames = 16
+	// linearNames is how many names are searched linearly before index
+	// is built.
+	linearNames = 16
+	// maxElemHint caps the elements of a map of structs allocated up front.
+	maxElemHint = 64
+)
 // mapFor returns the state of the map field f. The first call sets every
 // map's hint from the entries left, so a config without map keys never
 	ms := &st.maps[f.slot]
 	if !ms.m.IsValid() {
 		ms.m = st.root.FieldByIndex(f.index)
-		if f.elem == nil {
+		if f.elem != nil {
-			if ms.m.IsNil() {
+			// A name sets some of its fields: guess half of them per name,
-				ms.m.Set(reflect.MakeMapWithSize(f.typ, ms.hint))
+			// capped, as elements are whole structs. elems grows if needed.
+			ms.hint = min(2*ms.hint/max(len(f.elem.order), 1)+1, maxElemHint)
+		}
+		if ms.m.IsNil() {
+			// Created even if every key is rejected, as before a strict error.
+			// A map of structs gets its names in flush: hint is only a guess.
+			size := ms.hint
+			if f.elem != nil {
+				size = 0
 			}
+			ms.m.Set(reflect.MakeMapWithSize(f.typ, size))
+		}
+		if f.elem == nil {
 			if f.typ == stringMapType && f.val.plain() {
 				ms.plain = ms.m.Interface().(map[string]string)
 			} else {
 	}
 	i := len(ms.names)
 	if !ms.elems.IsValid() {
-		// Every name comes from a key counted in hint: elems never grows.
 		ms.elems = reflect.MakeSlice(f.elems, ms.hint, ms.hint)
 		ms.names = make([]pendingName, 0, ms.hint)
+	} else if i == ms.elems.Len() {
+		grown := reflect.MakeSlice(f.elems, 2*i, 2*i)
+		reflect.Copy(grown, ms.elems)
+		ms.elems = grown
 	}
 	ms.names = append(ms.names, pendingName{name: name})
 	if ms.index != nil {
 	return i
 }
-// flush writes the pending struct elements back to their maps. A map that
+// flush writes the pending struct elements back to their maps.
-// a key reached exists afterwards, even when every key was rejected.
 func (st *decodeState) flush(s *schema) {
 	for slot := range st.maps {
 		ms, f := &st.maps[slot], s.prefixes[slot]
 		if f.elem == nil || !ms.m.IsValid() {
 			continue
 		}
-		if ms.m.IsNil() {
-			n := 0
-			for _, p := range ms.names {
-				if p.set {
-					n++
-				}
-			}
-			ms.m.Set(reflect.MakeMapWithSize(f.typ, n))
-		}
 		for i, p := range ms.names {
 			if p.set {
 				ms.key.SetString(p.name)

M decode_test.go +24 -5


@@ -236,24 +236,41 @@ type nestedMaps struct {
236	236		}
237	237
238	238		func TestDecodeMaps(t *testing.T) {
	239	+	// One key per name: more names than the element hint guesses, past the
	240	+	// linear search.
239	241		var many strings.Builder
240	242		want := map[string]output{}
241		-	for i := range 40 { // past the linear search, twice per name
	243	+	for i := range 3 * maxElemHint {
242	244		name := "o" + strconv.Itoa(i)
243		-	fmt.Fprintf(&many, "output.%s.scale = 2\noutput.%s.hdr = on\n", name, name)
244		-	want[name] = output{Scale: 2, HDR: true, Bright: 203}
	245	+	fmt.Fprintf(&many, "output.%s.hdr = on\n", name)
	246	+	want[name] = output{Scale: 1, HDR: true, Bright: 203}
245	247		}
246	248		tests := []struct {
247	249		name string
248	250		in string
	251	+	opts Options
249	252		start func(c *testConfig)
250	253		check func(t testing.T, c testConfig)
	254	+	err bool
251	255		}{
252	256		{
253	257		name: "many names",
254	258		in: many.String(),
255	259		check: func(t testing.T, c testConfig) { eq(t, c.Outputs, want) },
256	260		},
	261	+	{
	262	+	name: "one name, many keys",
	263	+	in: strings.Repeat("output.x.scale = 2\n", 200),
	264	+	opts: Options{AllowDuplicates: true},
	265	+	check: func(t testing.T, c testConfig) { eq(t, c.Outputs, map[string]output{"x": {Scale: 2, Bright: 203}}) },
	266	+	},
	267	+	{
	268	+	name: "map reached before a strict error exists",
	269	+	in: "output.x.scale = 2\ncount = bad\n",
	270	+	opts: Options{Strict: true},
	271	+	check: func(t testing.T, c testConfig) { eq(t, c.Outputs, map[string]output{}) },
	272	+	err: true,
	273	+	},
257	274		{
258	275		name: "existing elements are updated, not reset",
259	276		in: "output.A.hdr = on\noutput.B.scale = 3\nbind.x = y\n",

@@ -283,8 +300,10 @@ func TestDecodeMaps(t *testing.T) {
283	300		if tt.start != nil {
284	301		tt.start(&c)
285	302		}
286		-	_, err := Unmarshal([]byte(tt.in), &c, Options{})
287		-	must(t, err)
	303	+	_, err := Unmarshal([]byte(tt.in), &c, tt.opts)
	304	+	if (err != nil) != tt.err {
	305	+	t.Fatalf("err = %v", err)
	306	+	}
288	307		tt.check(t, &c)
289	308		})
290	309		}

 }
 func TestDecodeMaps(t *testing.T) {
+	// One key per name: more names than the element hint guesses, past the
+	// linear search.
 	var many strings.Builder
 	want := map[string]output{}
-	for i := range 40 { // past the linear search, twice per name
+	for i := range 3 * maxElemHint {
 		name := "o" + strconv.Itoa(i)
-		fmt.Fprintf(&many, "output.%s.scale = 2\noutput.%s.hdr = on\n", name, name)
+		fmt.Fprintf(&many, "output.%s.hdr = on\n", name)
-		want[name] = output{Scale: 2, HDR: true, Bright: 203}
+		want[name] = output{Scale: 1, HDR: true, Bright: 203}
 	}
 	tests := []struct {
 		name  string
 		in    string
+		opts  Options
 		start func(c *testConfig)
 		check func(t *testing.T, c *testConfig)
+		err   bool
 	}{
 		{
 			name:  "many names",
 			in:    many.String(),
 			check: func(t *testing.T, c *testConfig) { eq(t, c.Outputs, want) },
 		},
+		{
+			name:  "one name, many keys",
+			in:    strings.Repeat("output.x.scale = 2\n", 200),
+			opts:  Options{AllowDuplicates: true},
+			check: func(t *testing.T, c *testConfig) { eq(t, c.Outputs, map[string]output{"x": {Scale: 2, Bright: 203}}) },
+		},
+		{
+			name:  "map reached before a strict error exists",
+			in:    "output.x.scale = 2\ncount = bad\n",
+			opts:  Options{Strict: true},
+			check: func(t *testing.T, c *testConfig) { eq(t, c.Outputs, map[string]output{}) },
+			err:   true,
+		},
 		{
 			name: "existing elements are updated, not reset",
 			in:   "output.A.hdr = on\noutput.B.scale = 3\nbind.x = y\n",
 			if tt.start != nil {
 				tt.start(&c)
 			}
-			_, err := Unmarshal([]byte(tt.in), &c, Options{})
+			_, err := Unmarshal([]byte(tt.in), &c, tt.opts)
-			must(t, err)
+			if (err != nil) != tt.err {
+				t.Fatalf("err = %v", err)
+			}
 			tt.check(t, &c)
 		})
 	}

M marshal.go +5 -5


@@ -118,17 +118,17 @@ func Marshal(v any) ([]byte, error) {
118	118		key := reflect.New(f.typ.Key()).Elem()
119	119		for it, i := fv.MapRange(), 0; it.Next(); i++ {
120	120		key.SetIterKey(it)
121		-	name := key.String()
122		-	if strings.ContainsAny(name, "=\n\r") \|\| name != strings.TrimSpace(name) {
123		-	return nil, fmt.Errorf("kvconf: %s: invalid map name %q", f.key, name)
124		-	}
125		-	names = append(names, name)
	121	+	names = append(names, key.String())
126	122		order = append(order, i)
127	123		vals.Index(i).SetIterValue(it)
128	124		}
129	125		slices.SortFunc(order, func(a, b int) int { return strings.Compare(names[a], names[b]) })
130	126		for _, i := range order {
131	127		name, ev := names[i], vals.Index(i)
	128	+	// Checked in name order, so the error is the same on every run.
	129	+	if strings.ContainsAny(name, "=\n\r") \|\| name != strings.TrimSpace(name) {
	130	+	return nil, fmt.Errorf("kvconf: %s: invalid map name %q", f.key, name)
	131	+	}
132	132		if f.elem == nil {
133	133		if err := m.line(f.key, name, "", ev, f.val); err != nil {
134	134		return nil, err

 		key := reflect.New(f.typ.Key()).Elem()
 		for it, i := fv.MapRange(), 0; it.Next(); i++ {
 			key.SetIterKey(it)
-			name := key.String()
+			names = append(names, key.String())
-			if strings.ContainsAny(name, "=\n\r") || name != strings.TrimSpace(name) {
-				return nil, fmt.Errorf("kvconf: %s: invalid map name %q", f.key, name)
-			}
-			names = append(names, name)
 			order = append(order, i)
 			vals.Index(i).SetIterValue(it)
 		}
 		slices.SortFunc(order, func(a, b int) int { return strings.Compare(names[a], names[b]) })
 		for _, i := range order {
 			name, ev := names[i], vals.Index(i)
+			// Checked in name order, so the error is the same on every run.
+			if strings.ContainsAny(name, "=\n\r") || name != strings.TrimSpace(name) {
+				return nil, fmt.Errorf("kvconf: %s: invalid map name %q", f.key, name)
+			}
 			if f.elem == nil {
 				if err := m.line(f.key, name, "", ev, f.val); err != nil {
 					return nil, err

M parse.go +3 -4


@@ -155,7 +155,7 @@ func (p *problems) duplicates(entries []Entry) {
155	155		table = make([]int32, size)
156	156		}
157	157		mask := uint32(size - 1)
158		-	var dups []Warning
	158	+	syntax := len(p.list)
159	159		for i := range entries {
160	160		key := entries[i].Key
161	161		// Seeded, so a crafted file cannot make every key collide.

@@ -167,17 +167,16 @@ func (p *problems) duplicates(entries []Entry) {
167	167		break
168	168		}
169	169		if prev := &entries[k-1]; prev.Key == key {
170		-	dups = append(dups, Warning{Line: entries[i].Line, Key: key, Msg: "overrides line " + strconv.Itoa(prev.Line)})
	170	+	p.list = append(p.list, Warning{Line: entries[i].Line, Key: key, Msg: "overrides line " + strconv.Itoa(prev.Line)})
171	171		table[slot] = int32(i + 1)
172	172		break
173	173		}
174	174		}
175	175		}
176		-	if len(dups) == 0 {
	176	+	if len(p.list) == syntax {
177	177		return
178	178		}
179	179		// Warnings are reported in line order, merged with the syntax ones.
180		-	p.list = append(p.list, dups...)
181	180		slices.SortStableFunc(p.list, func(a, b Warning) int { return a.Line - b.Line })
182	181		if p.strict {
183	182		p.list = p.list[:1]

 		table = make([]int32, size)
 	}
 	mask := uint32(size - 1)
-	var dups []Warning
+	syntax := len(p.list)
 	for i := range entries {
 		key := entries[i].Key
 		// Seeded, so a crafted file cannot make every key collide.
 				break
 			}
 			if prev := &entries[k-1]; prev.Key == key {
-				dups = append(dups, Warning{Line: entries[i].Line, Key: key, Msg: "overrides line " + strconv.Itoa(prev.Line)})
+				p.list = append(p.list, Warning{Line: entries[i].Line, Key: key, Msg: "overrides line " + strconv.Itoa(prev.Line)})
 				table[slot] = int32(i + 1)
 				break
 			}
 		}
 	}
-	if len(dups) == 0 {
+	if len(p.list) == syntax {
 		return
 	}
 	// Warnings are reported in line order, merged with the syntax ones.
-	p.list = append(p.list, dups...)
 	slices.SortStableFunc(p.list, func(a, b Warning) int { return a.Line - b.Line })
 	if p.strict {
 		p.list = p.list[:1]

M parse_test.go +11 -3


@@ -256,15 +256,23 @@ func FuzzDuplicates(f *testing.F) {
256	256		want = append(want, w.Error())
257	257		}
258	258		_, got, _ := ParseBytes([]byte(in), Options{})
	259	+	if !slices.IsSortedFunc(got, func(a, b Warning) int { return a.Line - b.Line }) {
	260	+	t.Fatalf("warnings out of line order: %q", warningStrings(got))
	261	+	}
	262	+	// Strict fails on the first problem in line order.
	263	+	_, _, err = ParseBytes([]byte(in), Options{Strict: true})
	264	+	switch {
	265	+	case len(got) == 0 && err != nil:
	266	+	t.Fatalf("strict err = %v, lenient had no warning", err)
	267	+	case len(got) > 0 && (err == nil \|\| err.Error() != got[0].Error()):
	268	+	t.Fatalf("strict err = %v, want %q", err, got[0].Error())
	269	+	}
259	270		gotS := warningStrings(got)
260	271		slices.Sort(want)
261	272		slices.Sort(gotS)
262	273		if !slices.Equal(gotS, want) {
263	274		t.Fatalf("warnings = %q, want %q", gotS, want)
264	275		}
265		-	if !slices.IsSortedFunc(got, func(a, b Warning) int { return a.Line - b.Line }) {
266		-	t.Fatalf("warnings out of line order: %q", warningStrings(got))
267		-	}
268	276		})
269	277		}
270	278

 			want = append(want, w.Error())
 		}
 		_, got, _ := ParseBytes([]byte(in), Options{})
+		if !slices.IsSortedFunc(got, func(a, b Warning) int { return a.Line - b.Line }) {
+			t.Fatalf("warnings out of line order: %q", warningStrings(got))
+		}
+		// Strict fails on the first problem in line order.
+		_, _, err = ParseBytes([]byte(in), Options{Strict: true})
+		switch {
+		case len(got) == 0 && err != nil:
+			t.Fatalf("strict err = %v, lenient had no warning", err)
+		case len(got) > 0 && (err == nil || err.Error() != got[0].Error()):
+			t.Fatalf("strict err = %v, want %q", err, got[0].Error())
+		}
 		gotS := warningStrings(got)
 		slices.Sort(want)
 		slices.Sort(gotS)
 		if !slices.Equal(gotS, want) {
 			t.Fatalf("warnings = %q, want %q", gotS, want)
 		}
-		if !slices.IsSortedFunc(got, func(a, b Warning) int { return a.Line - b.Line }) {
-			t.Fatalf("warnings out of line order: %q", warningStrings(got))
-		}
 	})
 }

M write_test.go +9 -0


@@ -106,6 +106,15 @@ func TestMarshalRejectsInjection(t *testing.T) {
106	106		}
107	107		})
108	108		}
	109	+	t.Run("first invalid name in order", func(t *testing.T) {
	110	+	v := &marshalConfig{Binds: map[string]string{" d": "", " b": "", " a": "", " c": ""}}
	111	+	for range 20 { // map order is random
	112	+	_, err := Marshal(v)
	113	+	if err == nil \|\| err.Error() != `kvconf: bind.: invalid map name " a"` {
	114	+	t.Fatalf("err = %v", err)
	115	+	}
	116	+	}
	117	+	})
109	118		}
110	119
111	120		// upper marshals with a pointer receiver, like many TextMarshalers.


@@ -106,6 +106,15 @@ func TestMarshalRejectsInjection(t *testing.T) {
106	}	106		}
107	})	107		})
108	}	108		}
		109	+	t.Run("first invalid name in order", func(t *testing.T) {
		110	+	v := &marshalConfig{Binds: map[string]string{" d": "", " b": "", " a": "", " c": ""}}
		111	+	for range 20 { // map order is random
		112	+	_, err := Marshal(v)
		113	+	if err == nil \|\| err.Error() != `kvconf: bind.: invalid map name " a"` {
		114	+	t.Fatalf("err = %v", err)
		115	+	}
		116	+	}
		117	+	})
109	}	118		}
110		119
111	// upper marshals with a pointer receiver, like many TextMarshalers.	120		// upper marshals with a pointer receiver, like many TextMarshalers.