From 59ed4032f35cd64788bf9c15d6b0b1bb64dc30d2 Mon Sep 17 00:00:00 2001 From: Marcel van Lohuizen Date: Thu, 11 Sep 2025 11:57:28 +0200 Subject: [PATCH] internal/core/debug: prevent stack overflow Previously, an error argument referring to a parent node could result in a stack overflow. This was because the printer recurses into fmt.Format, losing the state of the printer. We now wrap arguments to the Go formatter with a pointer to a printer before printing errors so that the state can be retained. In order to do so, we had to change the previously existing wrapper to be able to be unwrapped. value.txtar now adds a test that would previously cause a stack overflow. To make the error message nicer, we also modified the old "TODO" message. Note that this change the printing order of some of the nested error messages. This is overall an improvement. OmitPath: we added the flag "OmitPath" to allow choosing to not print a path prefix. Previously, the path was printed in some cases, but not others. The new implementation mimics this behavior. However, we may opt to always print it in the future. For now we want to keep diffs small. This is not tied to a bug, as it was uncovered with the implementation of self. Signed-off-by: Marcel van Lohuizen Change-Id: I5d3ab3d027069662e9f0a6593709c1f4d3504a10 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1222367 Reviewed-by: Roger Peppe TryBot-Result: CUEcueckoo Unity-Result: CUE porcuepine --- cue/errors/errors.go | 47 ++++++++++--- cue/testdata/eval/dynamic_field.txtar | 4 +- cue/testdata/eval/issue2235.txtar | 12 ++-- cue/testdata/references/value.txtar | 59 +++++++++++++++- cue/types_test.go | 2 +- internal/core/adt/context.go | 26 +++++-- internal/core/debug/compact.go | 14 +++- internal/core/debug/debug.go | 74 +++++++++++++++----- internal/core/export/testdata/main/let.txtar | 8 +-- internal/core/format/printer.go | 26 +++++++ 10 files changed, 225 insertions(+), 47 deletions(-) create mode 100644 internal/core/format/printer.go diff --git a/cue/errors/errors.go b/cue/errors/errors.go index 708091095..26c99a7c5 100644 --- a/cue/errors/errors.go +++ b/cue/errors/errors.go @@ -29,6 +29,7 @@ import ( "strings" "cuelang.org/go/cue/token" + "cuelang.org/go/internal/core/format" ) // New is a convenience wrapper for [errors.New] in the core library. @@ -495,6 +496,12 @@ type Config struct { // ToSlash sets whether to use Unix paths. Mostly used for testing. ToSlash bool + + // OmitPath removes the path prefix from error messages. + OmitPath bool + + // Printer is used internally to detect printing cycles. + Printer format.Printer } var zeroConfig = &Config{} @@ -526,10 +533,20 @@ func String(err Error) string { return b.String() } +// StringWithConfig generates a short message from a given Error, using the +// provided configuration. +func StringWithConfig(err Error, cfg *Config) string { + var b strings.Builder + writeErr(&b, err, cfg) + return b.String() +} + func writeErr(w io.Writer, err Error, cfg *Config) { - if path := strings.Join(err.Path(), "."); path != "" { - _, _ = io.WriteString(w, path) - _, _ = io.WriteString(w, ": ") + if !cfg.OmitPath { + if path := strings.Join(err.Path(), "."); path != "" { + _, _ = io.WriteString(w, path) + _, _ = io.WriteString(w, ": ") + } } for { @@ -544,21 +561,33 @@ func writeErr(w io.Writer, err Error, cfg *Config) { // so we make a copy if we need to replace any arguments. didCopy := false for i, arg := range args { - var pos token.Position + var alt any switch arg := arg.(type) { case token.Pos: - pos = arg.Position() + pos := arg.Position() + pos.Filename = relPath(pos.Filename, cfg) + alt = pos case token.Position: - pos = arg + pos := arg + pos.Filename = relPath(pos.Filename, cfg) + alt = pos default: - continue + if cfg.Printer == nil { + // We should always do something. Consider replacing + // vertices with a path if this is not set. + continue + } + var replaced bool + alt, replaced = cfg.Printer.ReplaceArg(arg) + if !replaced { + continue + } } if !didCopy { args = slices.Clone(args) didCopy = true } - pos.Filename = relPath(pos.Filename, cfg) - args[i] = pos + args[i] = alt } n, _ := fmt.Fprintf(w, msg, args...) diff --git a/cue/testdata/eval/dynamic_field.txtar b/cue/testdata/eval/dynamic_field.txtar index 3831437c9..60eacc310 100644 --- a/cue/testdata/eval/dynamic_field.txtar +++ b/cue/testdata/eval/dynamic_field.txtar @@ -187,7 +187,7 @@ Result: // [incomplete] noCycleError.foo.baz.#ID: invalid interpolation: non-concrete value string (type string): // ./in.cue:59:8 // ./in.cue:59:11 - // noCycleError.foo.bar.entries: key value of dynamic field must be concrete, found _|_(invalid interpolation: noCycleError.foo.baz.#ID: non-concrete value string (type string)): + // noCycleError.foo.bar.entries: key value of dynamic field must be concrete, found _|_(noCycleError.foo.baz.#ID: invalid interpolation: non-concrete value string (type string)): // ./in.cue:61:22 } #ID: (_|_){ @@ -261,7 +261,7 @@ diff old new // [incomplete] noCycleError.foo.baz.#ID: invalid interpolation: non-concrete value string (type string): // ./in.cue:59:8 // ./in.cue:59:11 -+ // noCycleError.foo.bar.entries: key value of dynamic field must be concrete, found _|_(invalid interpolation: noCycleError.foo.baz.#ID: non-concrete value string (type string)): ++ // noCycleError.foo.bar.entries: key value of dynamic field must be concrete, found _|_(noCycleError.foo.baz.#ID: invalid interpolation: non-concrete value string (type string)): + // ./in.cue:61:22 } #ID: (_|_){ diff --git a/cue/testdata/eval/issue2235.txtar b/cue/testdata/eval/issue2235.txtar index 613ee27b2..7517c00ee 100644 --- a/cue/testdata/eval/issue2235.txtar +++ b/cue/testdata/eval/issue2235.txtar @@ -233,16 +233,16 @@ NumCloseIDs: 14 } } #GlobalIngressController: (_|_){ - // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: key value of dynamic field must be concrete, found _|_(invalid interpolation: #GlobalIngressController.objects.namespaced.ingress.Service: non-concrete value string (type string)): + // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: key value of dynamic field must be concrete, found _|_(#GlobalIngressController.objects.namespaced.ingress.Service: invalid interpolation: non-concrete value string (type string)): // ./issue2235.cue:43:12 class: (string){ string } objects: (#struct){ namespaced: (#struct){ ingress: (_|_){ - // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: key value of dynamic field must be concrete, found _|_(invalid interpolation: #GlobalIngressController.objects.namespaced.ingress.Service: non-concrete value string (type string)): + // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: key value of dynamic field must be concrete, found _|_(#GlobalIngressController.objects.namespaced.ingress.Service: invalid interpolation: non-concrete value string (type string)): // ./issue2235.cue:43:12 Service: (_|_){ - // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: key value of dynamic field must be concrete, found _|_(invalid interpolation: #GlobalIngressController.objects.namespaced.ingress.Service: non-concrete value string (type string)): + // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: key value of dynamic field must be concrete, found _|_(#GlobalIngressController.objects.namespaced.ingress.Service: invalid interpolation: non-concrete value string (type string)): // ./issue2235.cue:43:12 } } @@ -343,7 +343,7 @@ diff old new } #GlobalIngressController: (_|_){ - // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: invalid interpolation: non-concrete value string (type string): -+ // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: key value of dynamic field must be concrete, found _|_(invalid interpolation: #GlobalIngressController.objects.namespaced.ingress.Service: non-concrete value string (type string)): ++ // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: key value of dynamic field must be concrete, found _|_(#GlobalIngressController.objects.namespaced.ingress.Service: invalid interpolation: non-concrete value string (type string)): // ./issue2235.cue:43:12 - // ./issue2235.cue:40:9 class: (string){ string } @@ -351,12 +351,12 @@ diff old new namespaced: (#struct){ ingress: (_|_){ - // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: invalid interpolation: non-concrete value string (type string): -+ // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: key value of dynamic field must be concrete, found _|_(invalid interpolation: #GlobalIngressController.objects.namespaced.ingress.Service: non-concrete value string (type string)): ++ // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: key value of dynamic field must be concrete, found _|_(#GlobalIngressController.objects.namespaced.ingress.Service: invalid interpolation: non-concrete value string (type string)): // ./issue2235.cue:43:12 - // ./issue2235.cue:40:9 Service: (_|_){ - // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: invalid interpolation: non-concrete value string (type string): -+ // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: key value of dynamic field must be concrete, found _|_(invalid interpolation: #GlobalIngressController.objects.namespaced.ingress.Service: non-concrete value string (type string)): ++ // [incomplete] #GlobalIngressController.objects.namespaced.ingress.Service: key value of dynamic field must be concrete, found _|_(#GlobalIngressController.objects.namespaced.ingress.Service: invalid interpolation: non-concrete value string (type string)): // ./issue2235.cue:43:12 - // ./issue2235.cue:40:9 } diff --git a/cue/testdata/references/value.txtar b/cue/testdata/references/value.txtar index 3917496f4..eb3ad055e 100644 --- a/cue/testdata/references/value.txtar +++ b/cue/testdata/references/value.txtar @@ -10,6 +10,14 @@ valueCycle: b: X=3 + X // Issue #1003 listValueAlias: X = [1, 2, X[0]] + +-- err.cue -- +@experiment(structcmp) + +cycleErr: X={ + err3: == (X + 1) + err3: 4 +} -- out/eval/stats -- Leaks: 0 Freed: 13 @@ -21,7 +29,24 @@ Unifications: 13 Conjuncts: 20 Disjuncts: 13 -- out/evalalpha -- -(struct){ +Errors: +cycleErr.err3: invalid operands {err3:_|_(cycleErr.err3: invalid operands value at path 'cycleErr' and 1 to '+' (type struct and int))} and 1 to '+' (type struct and int): + ./err.cue:4:12 + ./err.cue:3:13 + ./err.cue:4:16 + +Result: +(_|_){ + // [eval] + cycleErr: (_|_){ + // [eval] + err3: (_|_){ + // [eval] cycleErr.err3: invalid operands {err3:_|_(cycleErr.err3: invalid operands value at path 'cycleErr' and 1 to '+' (type struct and int))} and 1 to '+' (type struct and int): + // ./err.cue:4:12 + // ./err.cue:3:13 + // ./err.cue:4:16 + } + } structShorthand: (struct){ b: (int){ 3 } c: (int){ 3 } @@ -48,7 +73,30 @@ Disjuncts: 13 diff old new --- old +++ new -@@ -11,8 +11,8 @@ +@@ -1,4 +1,21 @@ +-(struct){ ++Errors: ++cycleErr.err3: invalid operands {err3:_|_(cycleErr.err3: invalid operands value at path 'cycleErr' and 1 to '+' (type struct and int))} and 1 to '+' (type struct and int): ++ ./err.cue:4:12 ++ ./err.cue:3:13 ++ ./err.cue:4:16 ++ ++Result: ++(_|_){ ++ // [eval] ++ cycleErr: (_|_){ ++ // [eval] ++ err3: (_|_){ ++ // [eval] cycleErr.err3: invalid operands {err3:_|_(cycleErr.err3: invalid operands value at path 'cycleErr' and 1 to '+' (type struct and int))} and 1 to '+' (type struct and int): ++ // ./err.cue:4:12 ++ // ./err.cue:3:13 ++ // ./err.cue:4:16 ++ } ++ } + structShorthand: (struct){ + b: (int){ 3 } + c: (int){ 3 } +@@ -11,8 +28,8 @@ } valueCycle: (struct){ b: (_|_){ @@ -84,6 +132,13 @@ diff old new } } -- out/compile -- +--- err.cue +{ + cycleErr: { + err3: ==(〈1〉 + 1) + err3: 4 + } +} --- in.cue { structShorthand: { diff --git a/cue/types_test.go b/cue/types_test.go index 16e3f7bd0..9666e958f 100644 --- a/cue/types_test.go +++ b/cue/types_test.go @@ -3183,7 +3183,7 @@ func TestMarshalJSON(t *testing.T) { }, { // Issue #326 value: `x: "\(string)": "v"`, - err: `x: key value of dynamic field must be concrete, found _|_(invalid interpolation: x: non-concrete value string (type string)) (and 1 more errors)`, + err: `x: key value of dynamic field must be concrete, found _|_(x: invalid interpolation: non-concrete value string (type string)) (and 1 more errors)`, }, { // Issue #326 value: `x: "\(bool)": "v"`, diff --git a/internal/core/adt/context.go b/internal/core/adt/context.go index cba0a0801..76a9f36eb 100644 --- a/internal/core/adt/context.go +++ b/internal/core/adt/context.go @@ -1327,15 +1327,31 @@ func (c *OpContext) String(x Node) string { return c.Format(c.Runtime, x) } -type stringerFunc func() string +// Formatter wraps an adt.Node with the necessary information to print it. +// +// TODO: we could eliminate the need for this by ensuring that errors are +// _always_ formatted with a printer. We are not far off from this goal, but +// we need to verify several things. +// This is mainly possible because we intend to have a global string index +// using weak references. It also assumes that errors are always printed +// equally. +type Formatter struct { + X Node + + // F formats Node, resolving references as needed.using Runtime. + // TODO: only used for cases where the debug printer is somehow + // circumvented. Verify this no longer happens. + F func(Runtime, Node) string -func (f stringerFunc) String() string { return f() } + // TODO: is runtime needed? Probably not if we have a global string index. + R Runtime +} + +func (f Formatter) String() string { return f.F(f.R, f.X) } // Str reports a string of x via a [fmt.Stringer], for use in errors or debugging. func (c *OpContext) Str(x Node) fmt.Stringer { - return stringerFunc(func() string { - return c.String(x) - }) + return Formatter{X: x, F: c.Format, R: c.Runtime} } // NewList returns a new list for the given values. diff --git a/internal/core/debug/compact.go b/internal/core/debug/compact.go index 0d9629e0a..2402d7693 100644 --- a/internal/core/debug/compact.go +++ b/internal/core/debug/compact.go @@ -45,6 +45,11 @@ func (w *printer) compactNode(n adt.Node) { switch v := x.BaseValue.(type) { case *adt.StructMarker: + if !w.pushVertex(x) { + return + } + defer w.popVertex() + w.string("{") for i, a := range x.Arcs { if i > 0 { @@ -72,6 +77,11 @@ func (w *printer) compactNode(n adt.Node) { w.string("}") case *adt.ListMarker: + if !w.pushVertex(x) { + return + } + defer w.popVertex() + w.string("[") for i, a := range x.Arcs { if i > 0 { @@ -82,6 +92,8 @@ func (w *printer) compactNode(n adt.Node) { w.string("]") case *adt.Vertex: + // Disjunction, structure shared, etc. + if v, ok := w.printShared(x); !ok { w.node(v) w.popVertex() @@ -154,7 +166,7 @@ func (w *printer) compactNode(n adt.Node) { w.string(`_|_`) if x.Err != nil { w.string("(") - w.string(x.Err.Error()) + w.shortError(x.Err, false) w.string(")") } diff --git a/internal/core/debug/debug.go b/internal/core/debug/debug.go index e255e72cf..6cc55c7ee 100644 --- a/internal/core/debug/debug.go +++ b/internal/core/debug/debug.go @@ -81,6 +81,49 @@ type printer struct { // - auto } +// ReplaceArg implements the format.Printer interface. It wraps Vertex arguments +// with a formatter value, that holds a pointer to w. This allows the stack +// of processed vertices to be passed down, which in turn is used for cycle +// detection. +func (w *printer) ReplaceArg(arg any) (replacement any, replaced bool) { + var x adt.Node + var r adt.Runtime + switch v := arg.(type) { + case adt.Node: + x = v + case adt.Formatter: + x = v.X + r = v.R + } + + switch x := x.(type) { + default: + return arg, false + case *adt.Vertex: + // We replace the formatter (or node) with our own formatter that is + // capable of detecting cycles. + return formatter{p: w, x: x, r: r}, true + } +} + +type formatter struct { + p *printer + x adt.Node + r adt.Runtime `` +} + +func (f formatter) String() string { + p := printer{ + dst: make([]byte, 0, 128), + index: f.r, + cfg: f.p.cfg, + compact: true, // Always compact for error arguments. + stack: f.p.stack, + } + p.node(f.x) + return string(p.dst) +} + func (w *printer) string(s string) { if !w.compact && len(w.indent) > 0 { s = strings.Replace(s, "\n", "\n"+w.indent, -1) @@ -163,7 +206,9 @@ func (w *printer) printShared(v0 *adt.Vertex) (x *adt.Vertex, ok bool) { func (w *printer) pushVertex(v *adt.Vertex) bool { for _, x := range w.stack { if x == v { - w.string("") + w.string("value at path '") + w.path(v) + w.string("'") return false } } @@ -175,21 +220,15 @@ func (w *printer) popVertex() { w.stack = w.stack[:len(w.stack)-1] } -func (w *printer) shortError(errs errors.Error) { - for { - msg, args := errs.Msg() - w.dst = fmt.Appendf(w.dst, msg, args...) - - err := errors.Unwrap(errs) - if err == nil { - break - } - - if errs, _ = err.(errors.Error); errs != nil { - w.string(err.Error()) - break - } - } +// TODO: always print path? We allow a choice for keeping the error diff at a +// minimum. +func (w *printer) shortError(errs errors.Error, omitPath bool) { + w.string(errors.StringWithConfig(errs, &errors.Config{ + Cwd: w.cfg.Cwd, + ToSlash: true, + OmitPath: omitPath, + Printer: w, + })) } func (w *printer) interpolation(x *adt.Interpolation) { @@ -277,6 +316,7 @@ func (w *printer) node(n adt.Node) { msg := errors.Details(v.Err, &errors.Config{ Cwd: w.cfg.Cwd, ToSlash: true, + Printer: w, }) msg = strings.TrimSpace(msg) if msg != "" { @@ -438,7 +478,7 @@ func (w *printer) node(n adt.Node) { w.string(`_|_`) if x.Err != nil { w.string("(") - w.shortError(x.Err) + w.shortError(x.Err, true) w.string(")") } diff --git a/internal/core/export/testdata/main/let.txtar b/internal/core/export/testdata/main/let.txtar index 1d5c8c436..771cd4878 100644 --- a/internal/core/export/testdata/main/let.txtar +++ b/internal/core/export/testdata/main/let.txtar @@ -598,8 +598,8 @@ y: Y & Y_1 a: "foo" } } - comprehension: _|_ // comprehension: key value of dynamic field must be concrete, found _|_(invalid interpolation: invalid interpolation: comprehension.filepath: undefined field: name) (and 1 more errors) - files: _|_ // files: key value of dynamic field must be concrete, found _|_(invalid interpolation: invalid interpolation: filepath: undefined field: name) (and 3 more errors) + comprehension: _|_ // comprehension: key value of dynamic field must be concrete, found _|_(comprehension.filepath: invalid interpolation: invalid interpolation: undefined field: name) (and 1 more errors) + files: _|_ // files: key value of dynamic field must be concrete, found _|_(filepath: invalid interpolation: invalid interpolation: undefined field: name) (and 3 more errors) incomplete: { a: { x: _|_ // invalid interpolation: incomplete.a.x: non-concrete value string (type string) @@ -901,8 +901,8 @@ diff old new } - comprehension: _|_ // invalid interpolation: cycle error - files: _|_ // invalid interpolation: cycle error (and 1 more errors) -+ comprehension: _|_ // comprehension: key value of dynamic field must be concrete, found _|_(invalid interpolation: invalid interpolation: comprehension.filepath: undefined field: name) (and 1 more errors) -+ files: _|_ // files: key value of dynamic field must be concrete, found _|_(invalid interpolation: invalid interpolation: filepath: undefined field: name) (and 3 more errors) ++ comprehension: _|_ // comprehension: key value of dynamic field must be concrete, found _|_(comprehension.filepath: invalid interpolation: invalid interpolation: undefined field: name) (and 1 more errors) ++ files: _|_ // files: key value of dynamic field must be concrete, found _|_(filepath: invalid interpolation: invalid interpolation: undefined field: name) (and 3 more errors) incomplete: { a: { x: _|_ // invalid interpolation: incomplete.a.x: non-concrete value string (type string) diff --git a/internal/core/format/printer.go b/internal/core/format/printer.go new file mode 100644 index 000000000..a907a73b0 --- /dev/null +++ b/internal/core/format/printer.go @@ -0,0 +1,26 @@ +// Copyright 2025 CUE Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package format provides functionality for pretty-printing CUE values. +// These types need to be in a separate package to avoid import cycles. +package format + +// Printer is the interface used to print CUE values. The only implementation so +// far is the one in internal/core/debug. Note that most packages cannot +// directly import the debug package. +type Printer interface { + // ReplaceArg is a function that may be called to replace arguments to + // errors. This is mostly used for cycle detection. + ReplaceArg(x any) (r any, wasReplaced bool) +} -- 2.51.2