errbase: prevent a call cycle for Formatter/SafeFormatter errors

errbase/format_error.go

@@ -349,66 +349,76 @@ func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
 
 	bufIsRedactable := false
 
-	printDone := false
-	for _, fn := range specialCases {
-		if handled, desiredShortening := fn(err, (*safePrinter)(s), cause == nil /* leaf */); handled {
-			printDone = true
-			bufIsRedactable = true
-			if desiredShortening == nil {
-				// The error wants to elide the short messages from inner
-				// causes. Do it.
-				for i := range s.entries {
-					s.entries[i].elideShort = true
-				}
+	switch v := err.(type) {
+	case SafeFormatter:
+		bufIsRedactable = true
+		desiredShortening := v.SafeFormatError((*safePrinter)(s))
+		if desiredShortening == nil {
+			// The error wants to elide the short messages from inner
+			// causes. Do it.
+			for i := range s.entries {
+				s.entries[i].elideShort = true
 			}
-			break
 		}
-	}
-	if !printDone {
-		switch v := err.(type) {
-		case SafeFormatter:
-			bufIsRedactable = true
-			desiredShortening := v.SafeFormatError((*safePrinter)(s))
-			if desiredShortening == nil {
-				// The error wants to elide the short messages from inner
-				// causes. Do it.
-				for i := range s.entries {
-					s.entries[i].elideShort = true
-				}
+
+	case Formatter:
+		desiredShortening := v.FormatError((*printer)(s))
+		if desiredShortening == nil {
+			// The error wants to elide the short messages from inner
+			// causes. Do it.
+			for i := range s.entries {
+				s.entries[i].elideShort = true
 			}
+		}
 
-		case Formatter:
-			desiredShortening := v.FormatError((*printer)(s))
-			if desiredShortening == nil {
-				// The error wants to elide the short messages from inner
-				// causes. Do it.
-				for i := range s.entries {
-					s.entries[i].elideShort = true
-				}
+	case fmt.Formatter:
+		// We can only use a fmt.Formatter when both the following
+		// conditions are true:
+		// - when it is the leaf error, because a fmt.Formatter
+		//   on a wrapper also recurses.
+		// - when it is not the outermost wrapper, because
+		//   the Format() method is likely to be calling FormatError()
+		//   to do its job and we want to avoid an infinite recursion.
+		if !isOutermost && cause == nil {
+			v.Format(s, 'v')
+			if st, ok := err.(StackTraceProvider); ok {
+				// This is likely a leaf error from github/pkg/errors.
+				// The thing probably printed its stack trace on its own.
+				seenTrace = true
+				// We'll subsequently simplify stack traces in wrappers.
+				s.lastStack = st.StackTrace()
 			}
+		} else {
+			s.formatSimple(err, cause)
+		}
 
-		case fmt.Formatter:
-			// We can only use a fmt.Formatter when both the following
-			// conditions are true:
-			// - when it is the leaf error, because a fmt.Formatter
-			//   on a wrapper also recurses.
-			// - when it is not the outermost wrapper, because
-			//   the Format() method is likely to be calling FormatError()
-			//   to do its job and we want to avoid an infinite recursion.
-			if !isOutermost && cause == nil {
-				v.Format(s, 'v')
-				if st, ok := err.(StackTraceProvider); ok {
-					// This is likely a leaf error from github/pkg/errors.
-					// The thing probably printed its stack trace on its own.
-					seenTrace = true
-					// We'll subsequently simplify stack traces in wrappers.
-					s.lastStack = st.StackTrace()
+	default:
+		// Handle the special case overrides for context.Canceled,
+		// os.PathError, etc for which we know how to extract some safe
+		// strings.
+		//
+		// We need to do this in the `default` branch, instead of doing
+		// this above the switch, because the special handler could call a
+		// .Error() that delegates its implementation to fmt.Formatter,
+		// errors.Safeformatter or errors.Formattable, which brings us
+		// back to this method in a call cycle. So we need to handle the
+		// various interfaces first.
+		printDone := false
+		for _, fn := range specialCases {
+			if handled, desiredShortening := fn(err, (*safePrinter)(s), cause == nil /* leaf */); handled {
+				printDone = true
+				bufIsRedactable = true
+				if desiredShortening == nil {
+					// The error wants to elide the short messages from inner
+					// causes. Do it.
+					for i := range s.entries {
+						s.entries[i].elideShort = true
+					}
 				}
-			} else {
-				s.formatSimple(err, cause)
+				break
 			}
-
-		default:
+		}
+		if !printDone {
 			// If the error did not implement errors.Formatter nor
 			// fmt.Formatter, but it is a wrapper, still attempt best effort:
 			// print what we can at this level.

fmttests/datadriven_test.go

@@ -91,6 +91,9 @@ var leafCommands = map[string]commandFn{
 	// errFmt has both Format() and FormatError(),
 	// and demonstrates the common case of "rich" errors.
 	"fmt": func(_ error, args []arg) error { return &errFmt{strfy(args)} },
+	// errSafeFormat has SafeFormatError(), and has Error() and Format()
+	// redirect to that.
+	"safefmt": func(_ error, args []arg) error { return &errSafeFormat{strfy(args)} },
 
 	// errutil.New implements multi-layer errors.
 	"newf": func(_ error, args []arg) error { return errutil.Newf("new-style %s", strfy(args)) },
@@ -176,6 +179,9 @@ var wrapCommands = map[string]commandFn{
 	// werrFmt has both Format() and FormatError(),
 	// and demonstrates the common case of "rich" errors.
 	"fmt": func(err error, args []arg) error { return &werrFmt{err, strfy(args)} },
+	// werrSafeFormat has SafeFormatError(), and has Error() and Format()
+	// redirect to that.
+	"safefmt": func(err error, args []arg) error { return &werrSafeFormat{cause: err, msg: strfy(args)} },
 	// werrEmpty has no message of its own. Its Error() is implemented via its cause.
 	"empty": func(err error, _ []arg) error { return &werrEmpty{err} },
 	// werrDelegate delegates its Error() behavior to FormatError().

fmttests/format_error_test.go

@@ -317,6 +317,13 @@ Wraps: (3) woo
   | multi-line leaf payload
 Error types: (1) *fmttests.werrDelegateEmpty (2) *errors.withStack (3) *fmttests.errFmt`, ``,
 		},
+
+		{"safeformatter leaf",
+			&errSafeFormat{msg: "world"},
+			`safe world`, `
+safe world
+(1) safe world
+Error types: (1) *fmttests.errSafeFormat`, ``},
 	}
 
 	for _, test := range testCases {
@@ -702,3 +709,27 @@ func (w *werrMigrated) Format(s fmt.State, verb rune) { errbase.FormatError(w, s
 func init() {
 	errbase.RegisterTypeMigration("some/previous/path", "prevpkg.prevType", (*werrMigrated)(nil))
 }
+
+type errSafeFormat struct {
+	msg string
+}
+
+func (e *errSafeFormat) Error() string                 { return fmt.Sprint(e) }
+func (e *errSafeFormat) Format(s fmt.State, verb rune) { errbase.FormatError(e, s, verb) }
+func (e *errSafeFormat) SafeFormatError(p errbase.Printer) (next error) {
+	p.Printf("safe %s", e.msg)
+	return nil
+}
+
+type werrSafeFormat struct {
+	cause error
+	msg   string
+}
+
+func (w *werrSafeFormat) Cause() error                  { return w.cause }
+func (w *werrSafeFormat) Error() string                 { return fmt.Sprint(w) }
+func (w *werrSafeFormat) Format(s fmt.State, verb rune) { errbase.FormatError(w, s, verb) }
+func (w *werrSafeFormat) SafeFormatError(p errbase.Printer) (next error) {
+	p.Printf("safe %s", w.msg)
+	return w.cause
+}

fmttests/testdata/format/leaves

@@ -1491,6 +1491,66 @@ Title: "*errors.fundamental: ×\n×"
 <path>:<lineno>:
    (github.com/cockroachdb/errors/fmttests.glob.)...funcNN...
 
+run
+safefmt oneline twoline
+
+require (?s)oneline.*twoline
+----
+&fmttests.errSafeFormat{msg:"oneline\ntwoline"}
+=====
+===== non-redactable formats
+=====
+== %#v
+&fmttests.errSafeFormat{msg:"oneline\ntwoline"}
+== Error()
+safe oneline
+twoline
+== %v = Error(), good
+== %s = Error(), good
+== %q = quoted Error(), good
+== %x = hex Error(), good
+== %X = HEX Error(), good
+== %+v
+safe oneline
+(1) safe oneline
+  | twoline
+Error types: (1) *fmttests.errSafeFormat
+== %#v via Formattable() = %#v, good
+== %v via Formattable() = Error(), good
+== %s via Formattable() = %v via Formattable(), good
+== %q via Formattable() = quoted %v via Formattable(), good
+== %+v via Formattable() == %+v, good
+=====
+===== redactable formats
+=====
+== printed via redact Print(), ok - congruent with %v
+safe ‹oneline›
+‹twoline›
+== printed via redact Printf() %v = Print(), good
+== printed via redact Printf() %s = Print(), good
+== printed via redact Printf() %q, refused - good
+== printed via redact Printf() %x, refused - good
+== printed via redact Printf() %X, refused - good
+== printed via redact Printf() %+v, ok - congruent with %+v
+safe ‹oneline›
+(1) safe ‹oneline›
+  | ‹twoline›
+Error types: (1) *fmttests.errSafeFormat
+=====
+===== Sentry reporting
+=====
+== Message payload
+safe ×
+×
+--
+*fmttests.errSafeFormat
+== Extra "error types"
+github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat (*::)
+== Exception 1 (Module: "error domain: <none>")
+Type: "*fmttests.errSafeFormat"
+Title: "safe ×\n×"
+(NO STACKTRACE)
+
 run
 unimplemented oneline twoline
 

fmttests/testdata/format/leaves-via-network

@@ -1660,6 +1660,89 @@ Title: "*errors.fundamental: ×\n×"
 <path>:<lineno>:
    (github.com/cockroachdb/errors/fmttests.glob.)...funcNN...
 
+run
+safefmt oneline twoline
+opaque
+
+require (?s)oneline.*twoline
+----
+&errbase.opaqueLeaf{
+    msg:     "safe oneline\ntwoline",
+    details: errorspb.EncodedErrorDetails{
+        OriginalTypeName:  "github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat",
+        ErrorTypeMark:     errorspb.ErrorTypeMark{FamilyName:"github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat", Extension:""},
+        ReportablePayload: nil,
+        FullDetails:       (*types.Any)(nil),
+    },
+}
+=====
+===== non-redactable formats
+=====
+== %#v
+&errbase.opaqueLeaf{
+    msg:     "safe oneline\ntwoline",
+    details: errorspb.EncodedErrorDetails{
+        OriginalTypeName:  "github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat",
+        ErrorTypeMark:     errorspb.ErrorTypeMark{FamilyName:"github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat", Extension:""},
+        ReportablePayload: nil,
+        FullDetails:       (*types.Any)(nil),
+    },
+}
+== Error()
+safe oneline
+twoline
+== %v = Error(), good
+== %s = Error(), good
+== %q = quoted Error(), good
+== %x = hex Error(), good
+== %X = HEX Error(), good
+== %+v
+safe oneline
+(1) safe oneline
+  | twoline
+  |
+  | (opaque error leaf)
+  | type name: github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat
+Error types: (1) *errbase.opaqueLeaf
+== %#v via Formattable() = %#v, good
+== %v via Formattable() = Error(), good
+== %s via Formattable() = %v via Formattable(), good
+== %q via Formattable() = quoted %v via Formattable(), good
+== %+v via Formattable() == %+v, good
+=====
+===== redactable formats
+=====
+== printed via redact Print(), ok - congruent with %v
+‹safe oneline›
+‹twoline›
+== printed via redact Printf() %v = Print(), good
+== printed via redact Printf() %s = Print(), good
+== printed via redact Printf() %q, refused - good
+== printed via redact Printf() %x, refused - good
+== printed via redact Printf() %X, refused - good
+== printed via redact Printf() %+v, ok - congruent with %+v
+‹safe oneline›
+(1) ‹safe oneline›
+  | ‹twoline›
+  |
+  | (opaque error leaf)
+  | type name: github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat
+Error types: (1) *errbase.opaqueLeaf
+=====
+===== Sentry reporting
+=====
+== Message payload
+×
+×
+--
+*fmttests.errSafeFormat
+== Extra "error types"
+github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat (*::)
+== Exception 1 (Module: "error domain: <none>")
+Type: "*fmttests.errSafeFormat"
+Title: "×\n×"
+(NO STACKTRACE)
+
 run
 unimplemented oneline twoline
 opaque