Skip to content

Commit 8437610

Browse files
knzknz
committed
errbase: prevent a call cycle for Formatter/SafeFormatter errors
We've found a need somewhere to implement an `Error()` method on a leaf by delegating the behavior to the `Formatter` or `SafeFormatter` interfaces. In that case, the `errors.IsAny` call inside the special handler causes a call cycle. This commit breaks the cycle by handling the special cases after the interfaces. A possible regression would be that one of the stdlib errors starts implementing `fmt.Formatter`, whereby we'd lose the special handling (which, as of this writing, extracts safe strings). At the time of this writing, none of the stdlib errors of interest implement `fmt.Formatter`, so we're punting dealing with that when/if it arises in the future.
1 parent c713676 commit 8437610

15 files changed

+1513
-52
lines changed

‎errbase/format_error.go‎

Lines changed: 62 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -349,66 +349,76 @@ func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
349349

350350
bufIsRedactable := false
351351

352-
printDone := false
353-
for _, fn := range specialCases {
354-
if handled, desiredShortening := fn(err, (*safePrinter)(s), cause == nil /* leaf */); handled {
355-
printDone = true
356-
bufIsRedactable = true
357-
if desiredShortening == nil {
358-
// The error wants to elide the short messages from inner
359-
// causes. Do it.
360-
for i := range s.entries {
361-
s.entries[i].elideShort = true
362-
}
352+
switch v := err.(type) {
353+
case SafeFormatter:
354+
bufIsRedactable = true
355+
desiredShortening := v.SafeFormatError((*safePrinter)(s))
356+
if desiredShortening == nil {
357+
// The error wants to elide the short messages from inner
358+
// causes. Do it.
359+
for i := range s.entries {
360+
s.entries[i].elideShort = true
363361
}
364-
break
365362
}
366-
}
367-
if !printDone {
368-
switch v := err.(type) {
369-
case SafeFormatter:
370-
bufIsRedactable = true
371-
desiredShortening := v.SafeFormatError((*safePrinter)(s))
372-
if desiredShortening == nil {
373-
// The error wants to elide the short messages from inner
374-
// causes. Do it.
375-
for i := range s.entries {
376-
s.entries[i].elideShort = true
377-
}
363+
364+
case Formatter:
365+
desiredShortening := v.FormatError((*printer)(s))
366+
if desiredShortening == nil {
367+
// The error wants to elide the short messages from inner
368+
// causes. Do it.
369+
for i := range s.entries {
370+
s.entries[i].elideShort = true
378371
}
372+
}
379373

380-
case Formatter:
381-
desiredShortening := v.FormatError((*printer)(s))
382-
if desiredShortening == nil {
383-
// The error wants to elide the short messages from inner
384-
// causes. Do it.
385-
for i := range s.entries {
386-
s.entries[i].elideShort = true
387-
}
374+
case fmt.Formatter:
375+
// We can only use a fmt.Formatter when both the following
376+
// conditions are true:
377+
// - when it is the leaf error, because a fmt.Formatter
378+
// on a wrapper also recurses.
379+
// - when it is not the outermost wrapper, because
380+
// the Format() method is likely to be calling FormatError()
381+
// to do its job and we want to avoid an infinite recursion.
382+
if !isOutermost && cause == nil {
383+
v.Format(s, 'v')
384+
if st, ok := err.(StackTraceProvider); ok {
385+
// This is likely a leaf error from github/pkg/errors.
386+
// The thing probably printed its stack trace on its own.
387+
seenTrace = true
388+
// We'll subsequently simplify stack traces in wrappers.
389+
s.lastStack = st.StackTrace()
388390
}
391+
} else {
392+
s.formatSimple(err, cause)
393+
}
389394

390-
case fmt.Formatter:
391-
// We can only use a fmt.Formatter when both the following
392-
// conditions are true:
393-
// - when it is the leaf error, because a fmt.Formatter
394-
// on a wrapper also recurses.
395-
// - when it is not the outermost wrapper, because
396-
// the Format() method is likely to be calling FormatError()
397-
// to do its job and we want to avoid an infinite recursion.
398-
if !isOutermost && cause == nil {
399-
v.Format(s, 'v')
400-
if st, ok := err.(StackTraceProvider); ok {
401-
// This is likely a leaf error from github/pkg/errors.
402-
// The thing probably printed its stack trace on its own.
403-
seenTrace = true
404-
// We'll subsequently simplify stack traces in wrappers.
405-
s.lastStack = st.StackTrace()
395+
default:
396+
// Handle the special case overrides for context.Canceled,
397+
// os.PathError, etc for which we know how to extract some safe
398+
// strings.
399+
//
400+
// We need to do this in the `default` branch, instead of doing
401+
// this above the switch, because the special handler could call a
402+
// .Error() that delegates its implementation to fmt.Formatter,
403+
// errors.Safeformatter or errors.Formattable, which brings us
404+
// back to this method in a call cycle. So we need to handle the
405+
// various interfaces first.
406+
printDone := false
407+
for _, fn := range specialCases {
408+
if handled, desiredShortening := fn(err, (*safePrinter)(s), cause == nil /* leaf */); handled {
409+
printDone = true
410+
bufIsRedactable = true
411+
if desiredShortening == nil {
412+
// The error wants to elide the short messages from inner
413+
// causes. Do it.
414+
for i := range s.entries {
415+
s.entries[i].elideShort = true
416+
}
406417
}
407-
} else {
408-
s.formatSimple(err, cause)
418+
break
409419
}
410-
411-
default:
420+
}
421+
if !printDone {
412422
// If the error did not implement errors.Formatter nor
413423
// fmt.Formatter, but it is a wrapper, still attempt best effort:
414424
// print what we can at this level.

‎fmttests/datadriven_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,9 @@ var leafCommands = map[string]commandFn{
9191
// errFmt has both Format() and FormatError(),
9292
// and demonstrates the common case of "rich" errors.
9393
"fmt": func(_ error, args []arg) error { return &errFmt{strfy(args)} },
94+
// errSafeFormat has SafeFormatError(), and has Error() and Format()
95+
// redirect to that.
96+
"safefmt": func(_ error, args []arg) error { return &errSafeFormat{strfy(args)} },
9497

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

‎fmttests/format_error_test.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,13 @@ Wraps: (3) woo
317317
| multi-line leaf payload
318318
Error types: (1) *fmttests.werrDelegateEmpty (2) *errors.withStack (3) *fmttests.errFmt`, ``,
319319
},
320+
321+
{"safeformatter leaf",
322+
&errSafeFormat{msg: "world"},
323+
`safe world`, `
324+
safe world
325+
(1) safe world
326+
Error types: (1) *fmttests.errSafeFormat`, ``},
320327
}
321328

322329
for _, test := range testCases {
@@ -702,3 +709,27 @@ func (w *werrMigrated) Format(s fmt.State, verb rune) { errbase.FormatError(w, s
702709
func init() {
703710
errbase.RegisterTypeMigration("some/previous/path", "prevpkg.prevType", (*werrMigrated)(nil))
704711
}
712+
713+
type errSafeFormat struct {
714+
msg string
715+
}
716+
717+
func (e *errSafeFormat) Error() string { return fmt.Sprint(e) }
718+
func (e *errSafeFormat) Format(s fmt.State, verb rune) { errbase.FormatError(e, s, verb) }
719+
func (e *errSafeFormat) SafeFormatError(p errbase.Printer) (next error) {
720+
p.Printf("safe %s", e.msg)
721+
return nil
722+
}
723+
724+
type werrSafeFormat struct {
725+
cause error
726+
msg string
727+
}
728+
729+
func (w *werrSafeFormat) Cause() error { return w.cause }
730+
func (w *werrSafeFormat) Error() string { return fmt.Sprint(w) }
731+
func (w *werrSafeFormat) Format(s fmt.State, verb rune) { errbase.FormatError(w, s, verb) }
732+
func (w *werrSafeFormat) SafeFormatError(p errbase.Printer) (next error) {
733+
p.Printf("safe %s", w.msg)
734+
return w.cause
735+
}

‎fmttests/testdata/format/leaves‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1491,6 +1491,66 @@ Title: "*errors.fundamental: ×\n×"
14911491
<path>:<lineno>:
14921492
(github.com/cockroachdb/errors/fmttests.glob.)...funcNN...
14931493

1494+
run
1495+
safefmt oneline twoline
1496+
1497+
require (?s)oneline.*twoline
1498+
----
1499+
&fmttests.errSafeFormat{msg:"oneline\ntwoline"}
1500+
=====
1501+
===== non-redactable formats
1502+
=====
1503+
== %#v
1504+
&fmttests.errSafeFormat{msg:"oneline\ntwoline"}
1505+
== Error()
1506+
safe oneline
1507+
twoline
1508+
== %v = Error(), good
1509+
== %s = Error(), good
1510+
== %q = quoted Error(), good
1511+
== %x = hex Error(), good
1512+
== %X = HEX Error(), good
1513+
== %+v
1514+
safe oneline
1515+
(1) safe oneline
1516+
| twoline
1517+
Error types: (1) *fmttests.errSafeFormat
1518+
== %#v via Formattable() = %#v, good
1519+
== %v via Formattable() = Error(), good
1520+
== %s via Formattable() = %v via Formattable(), good
1521+
== %q via Formattable() = quoted %v via Formattable(), good
1522+
== %+v via Formattable() == %+v, good
1523+
=====
1524+
===== redactable formats
1525+
=====
1526+
== printed via redact Print(), ok - congruent with %v
1527+
safe ‹oneline›
1528+
‹twoline›
1529+
== printed via redact Printf() %v = Print(), good
1530+
== printed via redact Printf() %s = Print(), good
1531+
== printed via redact Printf() %q, refused - good
1532+
== printed via redact Printf() %x, refused - good
1533+
== printed via redact Printf() %X, refused - good
1534+
== printed via redact Printf() %+v, ok - congruent with %+v
1535+
safe ‹oneline›
1536+
(1) safe ‹oneline›
1537+
| ‹twoline›
1538+
Error types: (1) *fmttests.errSafeFormat
1539+
=====
1540+
===== Sentry reporting
1541+
=====
1542+
== Message payload
1543+
safe ×
1544+
×
1545+
--
1546+
*fmttests.errSafeFormat
1547+
== Extra "error types"
1548+
github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat (*::)
1549+
== Exception 1 (Module: "error domain: <none>")
1550+
Type: "*fmttests.errSafeFormat"
1551+
Title: "safe ×\n×"
1552+
(NO STACKTRACE)
1553+
14941554
run
14951555
unimplemented oneline twoline
14961556

‎fmttests/testdata/format/leaves-via-network‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1660,6 +1660,89 @@ Title: "*errors.fundamental: ×\n×"
16601660
<path>:<lineno>:
16611661
(github.com/cockroachdb/errors/fmttests.glob.)...funcNN...
16621662

1663+
run
1664+
safefmt oneline twoline
1665+
opaque
1666+
1667+
require (?s)oneline.*twoline
1668+
----
1669+
&errbase.opaqueLeaf{
1670+
msg: "safe oneline\ntwoline",
1671+
details: errorspb.EncodedErrorDetails{
1672+
OriginalTypeName: "github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat",
1673+
ErrorTypeMark: errorspb.ErrorTypeMark{FamilyName:"github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat", Extension:""},
1674+
ReportablePayload: nil,
1675+
FullDetails: (*types.Any)(nil),
1676+
},
1677+
}
1678+
=====
1679+
===== non-redactable formats
1680+
=====
1681+
== %#v
1682+
&errbase.opaqueLeaf{
1683+
msg: "safe oneline\ntwoline",
1684+
details: errorspb.EncodedErrorDetails{
1685+
OriginalTypeName: "github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat",
1686+
ErrorTypeMark: errorspb.ErrorTypeMark{FamilyName:"github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat", Extension:""},
1687+
ReportablePayload: nil,
1688+
FullDetails: (*types.Any)(nil),
1689+
},
1690+
}
1691+
== Error()
1692+
safe oneline
1693+
twoline
1694+
== %v = Error(), good
1695+
== %s = Error(), good
1696+
== %q = quoted Error(), good
1697+
== %x = hex Error(), good
1698+
== %X = HEX Error(), good
1699+
== %+v
1700+
safe oneline
1701+
(1) safe oneline
1702+
| twoline
1703+
|
1704+
| (opaque error leaf)
1705+
| type name: github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat
1706+
Error types: (1) *errbase.opaqueLeaf
1707+
== %#v via Formattable() = %#v, good
1708+
== %v via Formattable() = Error(), good
1709+
== %s via Formattable() = %v via Formattable(), good
1710+
== %q via Formattable() = quoted %v via Formattable(), good
1711+
== %+v via Formattable() == %+v, good
1712+
=====
1713+
===== redactable formats
1714+
=====
1715+
== printed via redact Print(), ok - congruent with %v
1716+
‹safe oneline›
1717+
‹twoline›
1718+
== printed via redact Printf() %v = Print(), good
1719+
== printed via redact Printf() %s = Print(), good
1720+
== printed via redact Printf() %q, refused - good
1721+
== printed via redact Printf() %x, refused - good
1722+
== printed via redact Printf() %X, refused - good
1723+
== printed via redact Printf() %+v, ok - congruent with %+v
1724+
‹safe oneline›
1725+
(1) ‹safe oneline›
1726+
| ‹twoline›
1727+
|
1728+
| (opaque error leaf)
1729+
| type name: github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat
1730+
Error types: (1) *errbase.opaqueLeaf
1731+
=====
1732+
===== Sentry reporting
1733+
=====
1734+
== Message payload
1735+
×
1736+
×
1737+
--
1738+
*fmttests.errSafeFormat
1739+
== Extra "error types"
1740+
github.com/cockroachdb/errors/fmttests/*fmttests.errSafeFormat (*::)
1741+
== Exception 1 (Module: "error domain: <none>")
1742+
Type: "*fmttests.errSafeFormat"
1743+
Title: "×\n×"
1744+
(NO STACKTRACE)
1745+
16631746
run
16641747
unimplemented oneline twoline
16651748
opaque

0 commit comments

Comments
 (0)