From 178b61df95d216cef6266bf584eb81c018913db0 Mon Sep 17 00:00:00 2001 From: truffle Date: Mon, 29 Jun 2026 09:12:38 +0000 Subject: [PATCH] Fix Marshal mutating *Node input #371 Marshal is meant to treat its input as read-only, but passing a *Node left the caller's tree with all resolvable tags stripped. The representer returns a user-supplied document node as-is, and nodev returns nested *Node values as-is, so the caller's node flows straight into the desolver, which clears inferable tags (!!str, !!int, !!map, ...) in place to keep output clean. The mutation surfaces on the caller's own node after the call returns. Fix by deep-copying the user-supplied node in the representer before it enters the rest of the pipeline. A deepCopyNode helper copies Content, Alias, and Stream, using a seen map so shared anchors keep their identity and cyclic alias graphs do not recurse forever. It is wired at both *Node entry points: the document-node branch in Represent and nodev (which also covers *Node values nested in a struct, map, or slice). Verified: - go test . -run TestMarshalDoesNotMutateNode: passes with the fix, fails without it (tags wiped on the input node). - go test ./... : main, internal, and cmd suites green; yaml-test-suite unchanged at 1383 pass / 225 known-fail. --- internal/libyaml/representer.go | 59 ++++++++++++++++++++++++++++++--- node_test.go | 18 ++++++++++ 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/internal/libyaml/representer.go b/internal/libyaml/representer.go index c9f1b21f..ad22c742 100644 --- a/internal/libyaml/representer.go +++ b/internal/libyaml/representer.go @@ -55,8 +55,10 @@ func (r *Representer) Represent(tag string, in reflect.Value) *Node { node, _ = in.Interface().(*Node) } if node != nil && node.Kind == DocumentNode { - // Already a document node, return as-is - return node + // Deep-copy the user-supplied document so downstream stages (the + // desolver in particular) never mutate the caller's node tree. + // Marshal must treat its input as read-only. + return deepCopyNode(node, map[*Node]*Node{}) } else { // Wrap the represented value in a document node contentNode := r.represent(tag, in) @@ -67,6 +69,52 @@ func (r *Representer) Represent(tag string, in reflect.Value) *Node { } } +// deepCopyNode returns a deep copy of n so that a caller passing a *Node to +// Marshal is not mutated by later pipeline stages. The seen map preserves +// shared node identity and prevents infinite recursion on cyclic anchor and +// alias graphs. +func deepCopyNode(n *Node, seen map[*Node]*Node) *Node { + if n == nil { + return nil + } + if c, ok := seen[n]; ok { + return c + } + c := &Node{ + Kind: n.Kind, + Style: n.Style, + Tag: n.Tag, + Value: n.Value, + Anchor: n.Anchor, + HeadComment: n.HeadComment, + LineComment: n.LineComment, + FootComment: n.FootComment, + Line: n.Line, + Column: n.Column, + } + seen[n] = c + c.Alias = deepCopyNode(n.Alias, seen) + if n.Content != nil { + c.Content = make([]*Node, len(n.Content)) + for i, child := range n.Content { + c.Content[i] = deepCopyNode(child, seen) + } + } + if n.Stream != nil { + s := &Stream{Encoding: n.Stream.Encoding} + if n.Stream.Version != nil { + v := *n.Stream.Version + s.Version = &v + } + if n.Stream.TagDirectives != nil { + s.TagDirectives = make([]StreamTagDirective, len(n.Stream.TagDirectives)) + copy(s.TagDirectives, n.Stream.TagDirectives) + } + c.Stream = s + } + return c +} + // From http://yaml.org/type/float.html, except the regular expression there // is bogus. In practice parsers do not enforce the "\.[0-9_]*" suffix. var base60float = regexp.MustCompile(`^[-+]?[0-9][0-9_]*(?::[0-5]?[0-9])+(?:\.[0-9_]*)?$`) @@ -397,10 +445,11 @@ func (r *Representer) nilv() *Node { } } -// nodev returns a node value as-is without conversion. +// nodev returns a deep copy of a node value. Copying ensures the caller's +// node tree is never mutated by later pipeline stages, whether the *Node is +// passed to Marshal directly or nested inside a struct, map, or slice. func (r *Representer) nodev(in reflect.Value) *Node { - // Return the node as-is - no conversion needed - return in.Interface().(*Node) + return deepCopyNode(in.Interface().(*Node), map[*Node]*Node{}) } // Len returns the number of keys in the list. diff --git a/node_test.go b/node_test.go index 8875132e..61d927e4 100644 --- a/node_test.go +++ b/node_test.go @@ -784,3 +784,21 @@ func TestNodeDumpInvalidOptions(t *testing.T) { assert.NotNil(t, err) assert.ErrorMatches(t, ".*indent must be.*", err) } + +func TestMarshalDoesNotMutateNode(t *testing.T) { + const data = "type: array\nlimit: 5\n" + + var node yaml.Node + assert.NoError(t, yaml.Unmarshal([]byte(data), &node)) + + // An independent decode of the same input is the read-only baseline. + var want yaml.Node + assert.NoError(t, yaml.Unmarshal([]byte(data), &want)) + + _, err := yaml.Marshal(&node) + assert.NoError(t, err) + + // Marshal must treat its input as read-only: the resolved tags + // (!!map, !!str, !!int) decoded into node must survive unchanged. + assert.DeepEqual(t, &want, &node) +}