From 31cc3a63e6906d07b15ae1dfd4367304b05e57fe Mon Sep 17 00:00:00 2001 From: Nick Fagerlund Date: Wed, 18 Jan 2023 18:40:54 -0800 Subject: [PATCH] Fix accidental mutation of shared `cty.Path`s in ValueMarks funcs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Go's `append()` reserves the right to mutate its primary argument in-place, and expects the caller to assign its return value to the same variable that was passed as the primary argument. Due to what was almost definitely a typo (followed by copy-paste mishap), the configschema `Block.ValueMarks` and `Object.ValueMarks` functions were treating it like an immutable function that returns a new slice. In rare and hard-to-reproduce cases, this was causing bizarre malfunctions when marking sensitive schema attributes in deeply-nested block structures -- omitting the marks for some sensitive values (🚨), and marking other entire blocks as sensitive (which is supposed to be impossible). The chaotic and unreliable nature of the bugs is likely related to `append()`'s automatic slice reallocation behavior (if the append operation overflows the original array allocation, the resulting behavior can _look_ immutable), but there might be other contributing factors too. This commit fixes existing instances of the problem, and wraps the desired copy-and-append behavior in a helper function to simplify handling shared parent paths in an immutable way. --- internal/configs/configschema/marks.go | 34 ++++++++++++++------------ 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/internal/configs/configschema/marks.go b/internal/configs/configschema/marks.go index c581b187b943..f16bad711e1c 100644 --- a/internal/configs/configschema/marks.go +++ b/internal/configs/configschema/marks.go @@ -7,6 +7,15 @@ import ( "github.com/zclconf/go-cty/cty" ) +// copyAndExtendPath returns a copy of a cty.Path with some additional +// `cty.PathStep`s appended to its end, to simplify creating new child paths. +func copyAndExtendPath(path cty.Path, nextSteps ...cty.PathStep) cty.Path { + newPath := make(cty.Path, len(path), len(path)+len(nextSteps)) + copy(newPath, path) + newPath = append(newPath, nextSteps...) + return newPath +} + // ValueMarks returns a set of path value marks for a given value and path, // based on the sensitive flag for each attribute within the schema. Nested // blocks are descended (if present in the given value). @@ -17,9 +26,7 @@ func (b *Block) ValueMarks(val cty.Value, path cty.Path) []cty.PathValueMarks { for name, attrS := range b.Attributes { if attrS.Sensitive { // Create a copy of the path, with this step added, to add to our PathValueMarks slice - attrPath := make(cty.Path, len(path), len(path)+1) - copy(attrPath, path) - attrPath = append(path, cty.GetAttrStep{Name: name}) + attrPath := copyAndExtendPath(path, cty.GetAttrStep{Name: name}) pvm = append(pvm, cty.PathValueMarks{ Path: attrPath, Marks: cty.NewValueMarks(marks.Sensitive), @@ -41,9 +48,7 @@ func (b *Block) ValueMarks(val cty.Value, path cty.Path) []cty.PathValueMarks { } // Create a copy of the path, with this step added, to add to our PathValueMarks slice - attrPath := make(cty.Path, len(path), len(path)+1) - copy(attrPath, path) - attrPath = append(path, cty.GetAttrStep{Name: name}) + attrPath := copyAndExtendPath(path, cty.GetAttrStep{Name: name}) pvm = append(pvm, attrS.NestedType.ValueMarks(val.GetAttr(name), attrPath)...) } @@ -61,9 +66,7 @@ func (b *Block) ValueMarks(val cty.Value, path cty.Path) []cty.PathValueMarks { } // Create a copy of the path, with this step added, to add to our PathValueMarks slice - blockPath := make(cty.Path, len(path), len(path)+1) - copy(blockPath, path) - blockPath = append(path, cty.GetAttrStep{Name: name}) + blockPath := copyAndExtendPath(path, cty.GetAttrStep{Name: name}) switch blockS.Nesting { case NestingSingle, NestingGroup: @@ -71,7 +74,10 @@ func (b *Block) ValueMarks(val cty.Value, path cty.Path) []cty.PathValueMarks { case NestingList, NestingMap, NestingSet: for it := blockV.ElementIterator(); it.Next(); { idx, blockEV := it.Element() - morePaths := blockS.Block.ValueMarks(blockEV, append(blockPath, cty.IndexStep{Key: idx})) + // Create a copy of the path, with this block instance's index + // step added, to add to our PathValueMarks slice + blockInstancePath := copyAndExtendPath(blockPath, cty.IndexStep{Key: idx}) + morePaths := blockS.Block.ValueMarks(blockEV, blockInstancePath) pvm = append(pvm, morePaths...) } default: @@ -100,9 +106,7 @@ func (o *Object) ValueMarks(val cty.Value, path cty.Path) []cty.PathValueMarks { switch o.Nesting { case NestingSingle, NestingGroup: // Create a path to this attribute - attrPath := make(cty.Path, len(path), len(path)+1) - copy(attrPath, path) - attrPath = append(path, cty.GetAttrStep{Name: name}) + attrPath := copyAndExtendPath(path, cty.GetAttrStep{Name: name}) if attrS.Sensitive { // If the entire attribute is sensitive, mark it so @@ -127,9 +131,7 @@ func (o *Object) ValueMarks(val cty.Value, path cty.Path) []cty.PathValueMarks { // of the loops: index into the collection, then the contained // attribute name. This is because we have one type // representing multiple collection elements. - attrPath := make(cty.Path, len(path), len(path)+2) - copy(attrPath, path) - attrPath = append(path, cty.IndexStep{Key: idx}, cty.GetAttrStep{Name: name}) + attrPath := copyAndExtendPath(path, cty.IndexStep{Key: idx}, cty.GetAttrStep{Name: name}) if attrS.Sensitive { // If the entire attribute is sensitive, mark it so