From def111fe1e05aa08463a17746fa8404c7383c124 Mon Sep 17 00:00:00 2001 From: Martin Desrumaux <9059840+gnuletik@users.noreply.github.com> Date: Wed, 24 Jun 2026 14:21:07 +0200 Subject: [PATCH 1/2] fix: don't mutate shared yaml.Node when rendering schemas concurrently --- datamodel/high/base/schema_proxy_test.go | 105 +++++++++++++++++++++++ datamodel/high/node_builder.go | 34 +++++++- 2 files changed, 137 insertions(+), 2 deletions(-) diff --git a/datamodel/high/base/schema_proxy_test.go b/datamodel/high/base/schema_proxy_test.go index 28e25863..a346c75b 100644 --- a/datamodel/high/base/schema_proxy_test.go +++ b/datamodel/high/base/schema_proxy_test.go @@ -11,6 +11,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "testing" "github.com/pb33f/libopenapi/datamodel" @@ -2215,3 +2216,107 @@ func TestCreateSchemaProxyRefWithSchema_IsRefWithSiblings(t *testing.T) { refWithNilSchema := CreateSchemaProxyRefWithSchema("#/components/schemas/Pet", nil) assert.False(t, refWithNilSchema.isRefWithSiblings()) } + +// findScalarNode returns the first scalar node in the tree whose value matches. +func findScalarNode(n *yaml.Node, value string) *yaml.Node { + if n == nil { + return nil + } + if n.Kind == yaml.ScalarNode && n.Value == value { + return n + } + for _, c := range n.Content { + if found := findScalarNode(c, value); found != nil { + return found + } + } + return nil +} + +// TestSchemaProxy_ConcurrentRender_DoesNotMutateSharedNodes is a regression test +// for a data race in inline rendering. Rendering a schema runs the underlying +// *yaml.Node through (*yaml.Node).Encode, whose desolve stage rewrites Tag/Style +// in place. Because the representer aliases input nodes, rendering mutated the +// model's shared scalar nodes: while one goroutine rendered a schema, another +// reading the same node (e.g. a const type check) could observe the tag briefly +// cleared to "" and report a bogus "const value type does not match" error. +// +// The renders below must never mutate the captured `const` scalar's tag. +// Run under `-race` to also catch the underlying data race directly. +func TestSchemaProxy_ConcurrentRender_DoesNotMutateSharedNodes(t *testing.T) { + const ymlComponents = `components: + schemas: + Status: + type: object + properties: + state: + type: string + const: ok` + + idx := func() *index.SpecIndex { + var idxNode yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(ymlComponents), &idxNode)) + return index.NewSpecIndexWithConfig(&idxNode, index.CreateOpenAPIIndexConfig()) + }() + + const ymlSchema = `type: object +properties: + state: + type: string + const: ok` + var node yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(ymlSchema), &node)) + + // The `const: ok` scalar is a string; rendering must leave its tag intact. + constNode := findScalarNode(node.Content[0], "ok") + require.NotNil(t, constNode) + require.Equal(t, "!!str", constNode.Tag) + + lowProxy := new(lowbase.SchemaProxy) + require.NoError(t, lowProxy.Build(context.Background(), nil, node.Content[0], idx)) + highProxy := NewSchemaProxy(&low.NodeReference[*lowbase.SchemaProxy]{ + Value: lowProxy, + ValueNode: node.Content[0], + }) + + var corrupted atomic.Int64 + stop := make(chan struct{}) + + var readers sync.WaitGroup + for i := 0; i < 4; i++ { + readers.Add(1) + go func() { + defer readers.Done() + for { + select { + case <-stop: + return + default: + if constNode.Tag != "!!str" { + corrupted.Add(1) + } + } + } + }() + } + + var writers sync.WaitGroup + for i := 0; i < 50; i++ { + writers.Add(1) + go func() { + defer writers.Done() + for j := 0; j < 20; j++ { + _, err := highProxy.MarshalYAMLInlineWithContext(NewInlineRenderContext()) + assert.NoError(t, err) + } + }() + } + + writers.Wait() + close(stop) + readers.Wait() + + assert.Equal(t, int64(0), corrupted.Load(), + "rendering mutated the shared const scalar's tag (was cleared by desolve)") + assert.Equal(t, "!!str", constNode.Tag, "shared const scalar tag must survive rendering") +} diff --git a/datamodel/high/node_builder.go b/datamodel/high/node_builder.go index 04f78cda..378eac81 100644 --- a/datamodel/high/node_builder.go +++ b/datamodel/high/node_builder.go @@ -348,6 +348,36 @@ func (n *NodeBuilder) Render() *yaml.Node { return m } +// encodeSafeValue returns a value safe to pass to (*yaml.Node).Encode. When the +// value is a *yaml.Node it returns a deep copy: Encode desolves the represented +// graph in place (Desolve rewrites Tag/Style), and the representer aliases input +// nodes, so encoding a model-owned node would mutate it. With concurrent renders +// (e.g. linters running rules in parallel) that mutation races with readers of +// the same node. Encoding a copy keeps shared nodes immutable. +func encodeSafeValue(value any) any { + if vn, ok := value.(*yaml.Node); ok { + return deepCopyYAMLNode(vn) + } + return value +} + +func deepCopyYAMLNode(n *yaml.Node) *yaml.Node { + if n == nil { + return nil + } + c := *n + if n.Alias != nil { + c.Alias = deepCopyYAMLNode(n.Alias) + } + if n.Content != nil { + c.Content = make([]*yaml.Node, len(n.Content)) + for i, child := range n.Content { + c.Content[i] = deepCopyYAMLNode(child) + } + } + return &c +} + // AddYAMLNode will add a new *yaml.Node to the parent node, using the tag, key and value provided. // If the value is nil, then the node will not be added. This method is recursive, so it will dig down // into any non-scalar types. @@ -492,7 +522,7 @@ func (n *NodeBuilder) AddYAMLNode(parent *yaml.Node, entry *nodes.NodeEntry) *ya break } - err := rawNode.Encode(value) + err := rawNode.Encode(encodeSafeValue(value)) if err != nil { return parent } else { @@ -645,7 +675,7 @@ func (n *NodeBuilder) AddYAMLNode(parent *yaml.Node, entry *nodes.NodeEntry) *ya } } - err := rawNode.Encode(value) + err := rawNode.Encode(encodeSafeValue(value)) if err != nil { return parent } else { From 8972039591298cfcc69dee5621f5d95a95ec3ad6 Mon Sep 17 00:00:00 2001 From: Martin Desrumaux <9059840+gnuletik@users.noreply.github.com> Date: Wed, 24 Jun 2026 14:44:31 +0200 Subject: [PATCH 2/2] test: cover encodeSafeValue and deepCopyYAMLNode branches Adds a unit test exercising the passthrough, nil, alias and content branches of the new node-copy helpers, bringing patch coverage to 100%. Co-Authored-By: Claude Opus 4.8 --- datamodel/high/node_builder_test.go | 40 +++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/datamodel/high/node_builder_test.go b/datamodel/high/node_builder_test.go index da323baf..47dd6e2f 100644 --- a/datamodel/high/node_builder_test.go +++ b/datamodel/high/node_builder_test.go @@ -1700,3 +1700,43 @@ func TestOriginalFloatLexeme(t *testing.T) { }) } } + +func TestEncodeSafeValue_DeepCopiesYAMLNodes(t *testing.T) { + // non-*yaml.Node values pass through unchanged. + assert.Equal(t, 42, encodeSafeValue(42)) + assert.Equal(t, "hello", encodeSafeValue("hello")) + + // nil nodes return nil rather than panicking. + assert.Nil(t, deepCopyYAMLNode(nil)) + assert.Nil(t, encodeSafeValue((*yaml.Node)(nil))) + + // a *yaml.Node is deep-copied: every node is a distinct pointer, including + // content children and aliases, so mutating the copy can't affect the model. + anchor := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: "anchored"} + orig := &yaml.Node{ + Kind: yaml.MappingNode, + Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "key"}, + {Kind: yaml.ScalarNode, Tag: "!!str", Value: "val", Style: yaml.DoubleQuotedStyle}, + {Kind: yaml.AliasNode, Value: "ref", Alias: anchor}, + }, + } + + copied, ok := encodeSafeValue(orig).(*yaml.Node) + assert.True(t, ok) + assert.NotSame(t, orig, copied) + assert.Equal(t, len(orig.Content), len(copied.Content)) + for i := range orig.Content { + assert.NotSame(t, orig.Content[i], copied.Content[i]) + assert.Equal(t, orig.Content[i].Value, copied.Content[i].Value) + } + + // the alias child is deep-copied too. + assert.NotNil(t, copied.Content[2].Alias) + assert.NotSame(t, anchor, copied.Content[2].Alias) + assert.Equal(t, "anchored", copied.Content[2].Alias.Value) + + // mutating the copy must not touch the original (this is the whole point). + copied.Content[1].Tag = "" + assert.Equal(t, "!!str", orig.Content[1].Tag) +}