diff --git a/pkg/ccl/importccl/import_processor_test.go b/pkg/ccl/importccl/import_processor_test.go index d8aa2e6a58ba..84e89de9e6c3 100644 --- a/pkg/ccl/importccl/import_processor_test.go +++ b/pkg/ccl/importccl/import_processor_test.go @@ -10,7 +10,6 @@ package importccl import ( "context" - "errors" "fmt" "io/ioutil" "math" @@ -46,6 +45,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/cockroach/pkg/util/retry" "github.com/cockroachdb/cockroach/pkg/util/syncutil" + "github.com/cockroachdb/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -536,7 +536,7 @@ func queryJob(db sqlutils.DBHandle, jobID int64) (js jobState) { payload := &jobspb.Payload{} js.err = protoutil.Unmarshal(payloadBytes, payload) if js.err == nil { - js.err = errors.New(payload.Error) + js.err = errors.Newf("%s", payload.Error) } return } diff --git a/pkg/ccl/importccl/read_import_base.go b/pkg/ccl/importccl/read_import_base.go index 6a79b07789ce..2d241d87a1fa 100644 --- a/pkg/ccl/importccl/read_import_base.go +++ b/pkg/ccl/importccl/read_import_base.go @@ -195,13 +195,11 @@ func readInputFiles( for s := range rejected { countRejected++ if countRejected > 1000 { // TODO(spaskob): turn the magic constant into an option - return pgerror.New( + return pgerror.Newf( pgcode.DataCorrupted, - fmt.Sprintf( - "too many parsing errors (%d) encountered for file %s", - countRejected, - dataFile, - ), + "too many parsing errors (%d) encountered for file %s", + countRejected, + dataFile, ) } buf = append(buf, s...) @@ -339,8 +337,9 @@ func isMultiTableFormat(format roachpb.IOFileFormat_FileFormat) bool { } func makeRowErr(_ string, row int64, code, format string, args ...interface{}) error { - return pgerror.NewWithDepthf(1, code, - "row %d: "+format, append([]interface{}{row}, args...)...) + err := pgerror.NewWithDepthf(1, code, format, args...) + err = errors.WrapWithDepthf(1, err, "row %d", row) + return err } func wrapRowErr(err error, _ string, row int64, code, format string, args ...interface{}) error { diff --git a/pkg/ccl/importccl/read_import_mysql.go b/pkg/ccl/importccl/read_import_mysql.go index db42185e994c..0c5f1c0d62cc 100644 --- a/pkg/ccl/importccl/read_import_mysql.go +++ b/pkg/ccl/importccl/read_import_mysql.go @@ -676,18 +676,18 @@ func mysqlColToCockroach( def.Type = types.Jsonb case mysqltypes.Set: - return nil, unimplemented.NewWithIssueHint(32560, - "cannot import SET columns at this time", + return nil, errors.WithHint( + unimplemented.NewWithIssue(32560, "cannot import SET columns at this time"), "try converting the column to a 64-bit integer before import") case mysqltypes.Geometry: - return nil, unimplemented.NewWithIssuef(32559, - "cannot import GEOMETRY columns at this time") + return nil, unimplemented.NewWithIssue(32559, "cannot import GEOMETRY columns at this time") case mysqltypes.Bit: - return nil, unimplemented.NewWithIssueHint(32561, - "cannot improt BIT columns at this time", + return nil, errors.WithHint( + unimplemented.NewWithIssue(32561, "cannot import BIT columns at this time"), "try converting the column to a 64-bit integer before import") default: - return nil, unimplemented.Newf(fmt.Sprintf("import.mysqlcoltype.%s", typ), "unsupported mysql type %q", col.Type) + return nil, unimplemented.Newf(fmt.Sprintf("import.mysqlcoltype.%s", typ), + "unsupported mysql type %q", col.Type) } if col.NotNull { diff --git a/pkg/cli/start.go b/pkg/cli/start.go index 50e8f0909c3a..dda4d21c87e9 100644 --- a/pkg/cli/start.go +++ b/pkg/cli/start.go @@ -661,7 +661,7 @@ If problems persist, please see ` + base.DocsURL("cluster-setup-troubleshooting. } else { // Don't shout to stderr since the server will have detached by // the time this function gets called. - log.Warningf(ctx, msg) + log.Warning(ctx, msg) } } @@ -1265,7 +1265,7 @@ func setupAndInitializeLoggingAndProfiling( // We log build information to stdout (for the short summary), but also // to stderr to coincide with the full logs. info := build.GetInfo() - log.Infof(ctx, info.Short()) + log.Info(ctx, info.Short()) initMemProfile(ctx, outputDirectory) initCPUProfile(ctx, outputDirectory) diff --git a/pkg/cmd/roachprod-stress/main.go b/pkg/cmd/roachprod-stress/main.go index f9b8306ed6e9..59a9f1e87602 100644 --- a/pkg/cmd/roachprod-stress/main.go +++ b/pkg/cmd/roachprod-stress/main.go @@ -14,7 +14,6 @@ import ( "bufio" "bytes" "context" - "errors" "flag" "fmt" "io" @@ -33,6 +32,7 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/util/timeutil" + "github.com/cockroachdb/errors" ) var ( @@ -62,7 +62,7 @@ func run() error { var b bytes.Buffer flags.SetOutput(&b) flags.Usage() - return errors.New(b.String()) + return errors.Newf("%s", b.String()) } cluster := os.Args[1] @@ -109,7 +109,7 @@ func run() error { var b bytes.Buffer flags.SetOutput(&b) flags.Usage() - return errors.New(b.String()) + return errors.Newf("%s", b.String()) } if *flagFailure != "" { if _, err := regexp.Compile(*flagFailure); err != nil { diff --git a/pkg/cmd/roachtest/test_runner.go b/pkg/cmd/roachtest/test_runner.go index 5ce878dd45cd..6d3154d70bad 100644 --- a/pkg/cmd/roachtest/test_runner.go +++ b/pkg/cmd/roachtest/test_runner.go @@ -35,9 +35,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/cockroach/pkg/util/version" + "github.com/cockroachdb/errors" "github.com/cockroachdb/logtags" "github.com/petermattis/goid" - "github.com/pkg/errors" ) // testRunner runs tests. @@ -812,7 +812,7 @@ func (r *testRunner) runTest( r.collectClusterLogs(ctx, c, t.l) // We return an error here because the test goroutine is still running, so // we want to alert the caller of this unusual situation. - return false, fmt.Errorf(msg) + return false, errors.New(msg) } } diff --git a/pkg/cmd/roachvet/main.go b/pkg/cmd/roachvet/main.go index c1543f0a5446..2d313586137c 100644 --- a/pkg/cmd/roachvet/main.go +++ b/pkg/cmd/roachvet/main.go @@ -15,6 +15,7 @@ package main import ( "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/descriptormarshal" + "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/fmtsafe" "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/hash" "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/nocopy" "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/returnerrcheck" @@ -55,6 +56,7 @@ func main() { returnerrcheck.Analyzer, timer.Analyzer, unconvert.Analyzer, + fmtsafe.Analyzer, // Standard go vet analyzers: asmdecl.Analyzer, diff --git a/pkg/cmd/urlcheck/lib/urlcheck/urlcheck.go b/pkg/cmd/urlcheck/lib/urlcheck/urlcheck.go index 277d99127dd9..49fd4b79ab34 100644 --- a/pkg/cmd/urlcheck/lib/urlcheck/urlcheck.go +++ b/pkg/cmd/urlcheck/lib/urlcheck/urlcheck.go @@ -13,7 +13,6 @@ package urlcheck import ( "bytes" "crypto/tls" - "errors" "fmt" "log" "net" @@ -23,6 +22,7 @@ import ( "strings" "time" + "github.com/cockroachdb/errors" "github.com/ghemawat/stream" ) @@ -138,7 +138,7 @@ func checkURL(client *http.Client, url string) error { return nil } - return errors.New(resp.Status) + return errors.Newf("%s", errors.Safe(resp.Status)) } func checkURLWithRetries(client *http.Client, url string) error { @@ -239,7 +239,7 @@ func checkURLs(uniqueURLs map[string][]string) error { for _, loc := range locs { fmt.Fprintln(&buf, " ", loc) } - errChan <- errors.New(buf.String()) + errChan <- errors.Newf("%s", buf.String()) } else { errChan <- nil } @@ -258,7 +258,7 @@ func checkURLs(uniqueURLs map[string][]string) error { fmt.Fprint(&buf, err) } fmt.Fprintf(&buf, "%d errors\n", len(errs)) - return errors.New(buf.String()) + return errors.Newf("%s", buf.String()) } return nil } diff --git a/pkg/internal/rsg/yacc/parse.go b/pkg/internal/rsg/yacc/parse.go index 66c0030fd0c0..05e7fed3ff91 100644 --- a/pkg/internal/rsg/yacc/parse.go +++ b/pkg/internal/rsg/yacc/parse.go @@ -23,6 +23,8 @@ package yacc import ( "fmt" "runtime" + + "github.com/pkg/errors" ) // Tree is the representation of a single parsed file. @@ -80,8 +82,9 @@ func New(name string) *Tree { // errorf formats the error and terminates processing. func (t *Tree) errorf(format string, args ...interface{}) { - format = fmt.Sprintf("parse: %s:%d: %s", t.Name, t.lex.lineNumber(), format) - panic(fmt.Errorf(format, args...)) + err := fmt.Errorf(format, args...) + err = errors.Wrapf(err, "parse: %s:%d", t.Name, t.lex.lineNumber()) + panic(err) } // expect consumes the next token and guarantees it has the required type. diff --git a/pkg/kv/kvclient/kvcoord/dist_sender.go b/pkg/kv/kvclient/kvcoord/dist_sender.go index 330ff588c106..4d502d0ace55 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender.go @@ -1834,10 +1834,10 @@ func (ds *DistSender) sendToReplicas( // Has the caller given up? if ctx.Err() != nil { - errMsg := fmt.Sprintf("context done during DistSender.Send: %s", ctx.Err()) - log.Eventf(ctx, errMsg) + reportedErr := errors.Wrap(ctx.Err(), "context done during DistSender.Send") + log.Eventf(ctx, "%v", reportedErr) if ambiguousError != nil { - return nil, roachpb.NewAmbiguousResultError(errMsg) + return nil, roachpb.NewAmbiguousResultError(reportedErr.Error()) } // Don't consider this a SendError, because SendErrors indicate that we // were unable to reach a replica that could serve the request, and they diff --git a/pkg/kv/kvclient/kvcoord/transport_test.go b/pkg/kv/kvclient/kvcoord/transport_test.go index bb467b1cf40a..47d04ad3ef65 100644 --- a/pkg/kv/kvclient/kvcoord/transport_test.go +++ b/pkg/kv/kvclient/kvcoord/transport_test.go @@ -115,7 +115,7 @@ func TestSpanImport(t *testing.T) { server := mockInternalClient{} // Let's spice things up and simulate an error from the server. expectedErr := "my expected error" - server.pErr = roachpb.NewErrorf(expectedErr) + server.pErr = roachpb.NewErrorf(expectedErr /* nolint:fmtsafe */) recCtx, getRec, cancel := tracing.ContextWithRecordingSpan(ctx, "test") defer cancel() diff --git a/pkg/kv/kvserver/batcheval/cmd_push_txn.go b/pkg/kv/kvserver/batcheval/cmd_push_txn.go index f0dbc5124c64..7a5545a6f2bf 100644 --- a/pkg/kv/kvserver/batcheval/cmd_push_txn.go +++ b/pkg/kv/kvserver/batcheval/cmd_push_txn.go @@ -244,9 +244,12 @@ func PushTxn( if !pusherWins { s = "failed to push" } - log.Infof(ctx, "%s "+s+" (push type=%s) %s: %s (pushee last active: %s)", - args.PusherTxn.Short(), pushType, args.PusheeTxn.Short(), - reason, reply.PusheeTxn.LastActive()) + log.Infof(ctx, "%s %s (push type=%s) %s: %s (pushee last active: %s)", + args.PusherTxn.Short(), log.Safe(s), + log.Safe(pushType), + args.PusheeTxn.Short(), + log.Safe(reason), + reply.PusheeTxn.LastActive()) } // If the pushed transaction is in the staging state, we can't change its diff --git a/pkg/kv/kvserver/queue.go b/pkg/kv/kvserver/queue.go index b038f15ecef8..86e2a90e5e41 100644 --- a/pkg/kv/kvserver/queue.go +++ b/pkg/kv/kvserver/queue.go @@ -564,18 +564,21 @@ type queueHelper interface { // is not available, the 'wait' parameter decides whether to wait or to return // as a noop. Note that if the system is quiescing, fn may never be called in- // dependent of the value of 'wait'. +// +// The caller is responsible for ensuring that opName does not contain PII. +// (Best is to pass a constant string.) func (bq *baseQueue) Async( ctx context.Context, opName string, wait bool, fn func(ctx context.Context, h queueHelper), ) { if log.V(3) { - log.InfofDepth(ctx, 2, opName) + log.InfofDepth(ctx, 2, "%s", log.Safe(opName)) } opName += " (" + bq.name + ")" if err := bq.store.stopper.RunLimitedAsyncTask(context.Background(), opName, bq.addOrMaybeAddSem, wait, func(ctx context.Context) { fn(ctx, baseQueueHelper{bq}) }); err != nil && bq.addLogN.ShouldLog() { - log.Infof(ctx, "rate limited in %s: %s", opName, err) + log.Infof(ctx, "rate limited in %s: %s", log.Safe(opName), err) } } diff --git a/pkg/kv/kvserver/raft_log_queue.go b/pkg/kv/kvserver/raft_log_queue.go index 4a21745f75d4..88f0ef30fbac 100644 --- a/pkg/kv/kvserver/raft_log_queue.go +++ b/pkg/kv/kvserver/raft_log_queue.go @@ -277,6 +277,10 @@ func (input truncateDecisionInput) LogTooLarge() bool { return input.LogSize > input.MaxLogSize } +// truncateDecision describes a truncation decision. +// Beware: when extending this struct, be sure to adjust .String() +// so that it is guaranteed to not contain any PII or confidential +// cluster data. type truncateDecision struct { Input truncateDecisionInput CommitIndex uint64 @@ -318,6 +322,9 @@ func (td *truncateDecision) NumNewRaftSnapshots() int { return td.raftSnapshotsForIndex(td.NewFirstIndex) - td.raftSnapshotsForIndex(td.Input.FirstIndex) } +// String returns a representation for the decision. +// It is guaranteed to not return PII or confidential +// information from the cluster. func (td *truncateDecision) String() string { var buf strings.Builder _, _ = fmt.Fprintf(&buf, "should truncate: %t [", td.ShouldTruncate()) @@ -612,7 +619,7 @@ func (rlq *raftLogQueue) process(ctx context.Context, r *Replica, _ *config.Syst } r.store.metrics.RaftLogTruncated.Inc(int64(decision.NumTruncatableIndexes())) } else { - log.VEventf(ctx, 3, decision.String()) + log.VEventf(ctx, 3, "%s", log.Safe(decision.String())) } return nil } diff --git a/pkg/kv/kvserver/replica_rangefeed.go b/pkg/kv/kvserver/replica_rangefeed.go index 4540dff73b0b..36d249c0e1da 100644 --- a/pkg/kv/kvserver/replica_rangefeed.go +++ b/pkg/kv/kvserver/replica_rangefeed.go @@ -131,7 +131,7 @@ func (r *Replica) RangeFeed( args *roachpb.RangeFeedRequest, stream roachpb.Internal_RangeFeedServer, ) *roachpb.Error { if !RangefeedEnabled.Get(&r.store.cfg.Settings.SV) { - return roachpb.NewErrorf("rangefeeds require the kv.rangefeed.enabled setting. See " + + return roachpb.NewErrorf("rangefeeds require the kv.rangefeed.enabled setting. See %s", base.DocsURL(`change-data-capture.html#enable-rangefeeds-to-reduce-latency`)) } ctx := r.AnnotateCtx(stream.Context()) diff --git a/pkg/roachpb/errors.go b/pkg/roachpb/errors.go index c929182dc04a..77df5698a242 100644 --- a/pkg/roachpb/errors.go +++ b/pkg/roachpb/errors.go @@ -144,10 +144,10 @@ func NewErrorWithTxn(err error, txn *Transaction) *Error { // passthrough to fmt.Errorf, with an additional prefix containing the // filename and line number. func NewErrorf(format string, a ...interface{}) *Error { - // Cannot use errors.Errorf here due to cyclic dependency. + err := errors.Newf(format, a...) file, line, _ := caller.Lookup(1) - s := fmt.Sprintf("%s:%d: ", file, line) - return NewError(fmt.Errorf(s+format, a...)) + err = errors.Wrapf(err, "%s:%d", file, line) + return NewError(err) } // String implements fmt.Stringer. diff --git a/pkg/server/authentication.go b/pkg/server/authentication.go index 0c84c68c4842..066b5fbdcc2b 100644 --- a/pkg/server/authentication.go +++ b/pkg/server/authentication.go @@ -31,8 +31,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/cockroach/pkg/util/timeutil" + "github.com/cockroachdb/errors" gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime" - "github.com/pkg/errors" "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" @@ -175,9 +175,9 @@ func (s *authenticationServer) UserLogout( ); err != nil { return nil, apiInternalError(ctx, err) } else if n == 0 { - msg := fmt.Sprintf("session with id %d nonexistent", sessionID) - log.Info(ctx, msg) - return nil, fmt.Errorf(msg) + err := errors.Newf("session with id %d nonexistent", sessionID) + log.Info(ctx, err) + return nil, err } // Send back a header which will cause the browser to destroy the cookie. diff --git a/pkg/server/status/recorder.go b/pkg/server/status/recorder.go index eb4ee7230d15..5d30384407ef 100644 --- a/pkg/server/status/recorder.go +++ b/pkg/server/status/recorder.go @@ -594,7 +594,7 @@ func GetTotalMemory(ctx context.Context) (int64, error) { return 0, err } if warning != "" { - log.Infof(ctx, warning) + log.Infof(ctx, "%s", warning) } return memory, nil } diff --git a/pkg/sql/colexec/routers_test.go b/pkg/sql/colexec/routers_test.go index 41d34a5f5685..16e7cfc68f0b 100644 --- a/pkg/sql/colexec/routers_test.go +++ b/pkg/sql/colexec/routers_test.go @@ -140,6 +140,10 @@ func TestRouterOutputAddBatch(t *testing.T) { defer cleanup() for _, tc := range testCases { + if len(tc.selection) == 0 { + // No data to work with, probably due to a low coldata.BatchSize. + continue + } for _, mtc := range memoryTestCases { t.Run(fmt.Sprintf("%s/memoryLimit=%s", tc.name, humanizeutil.IBytes(mtc.bytes)), func(t *testing.T) { // Clear the testAllocator for use. diff --git a/pkg/sql/create_index.go b/pkg/sql/create_index.go index 9a4f5dc81192..edc7fd6b978b 100644 --- a/pkg/sql/create_index.go +++ b/pkg/sql/create_index.go @@ -12,7 +12,6 @@ package sql import ( "context" - "fmt" "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/geo/geoindex" @@ -297,8 +296,8 @@ func maybeCreateAndAddShardCol( if !existingShardCol.Hidden { // The user managed to reverse-engineer our crazy shard column name, so // we'll return an error here rather than try to be tricky. - return nil, false, pgerror.New(pgcode.DuplicateColumn, - fmt.Sprintf("column %s already specified; can't be used for sharding", shardCol.Name)) + return nil, false, pgerror.Newf(pgcode.DuplicateColumn, + "column %s already specified; can't be used for sharding", shardCol.Name) } return existingShardCol, false, nil } diff --git a/pkg/sql/drop_view.go b/pkg/sql/drop_view.go index 976072bb91cf..1b2fff72adec 100644 --- a/pkg/sql/drop_view.go +++ b/pkg/sql/drop_view.go @@ -12,7 +12,6 @@ package sql import ( "context" - "fmt" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/privilege" @@ -252,14 +251,14 @@ func (p *planner) getViewDescForCascade( viewName, err = p.getQualifiedTableName(ctx, viewDesc.TableDesc()) if err != nil { log.Warningf(ctx, "unable to retrieve qualified name of view %d: %v", viewID, err) - msg := fmt.Sprintf("cannot drop %s %q because a view depends on it", typeName, objName) - return nil, sqlbase.NewDependentObjectError(msg) + return nil, sqlbase.NewDependentObjectErrorf( + "cannot drop %s %q because a view depends on it", typeName, objName) } } - msg := fmt.Sprintf("cannot drop %s %q because view %q depends on it", - typeName, objName, viewName) - hint := fmt.Sprintf("you can drop %s instead.", viewName) - return nil, sqlbase.NewDependentObjectErrorWithHint(msg, hint) + return nil, errors.WithHintf( + sqlbase.NewDependentObjectErrorf("cannot drop %s %q because view %q depends on it", + typeName, objName, viewName), + "you can drop %s instead.", viewName) } return viewDesc, nil } diff --git a/pkg/sql/execute.go b/pkg/sql/execute.go index e56b1dcf95d9..9579d5e0efa7 100644 --- a/pkg/sql/execute.go +++ b/pkg/sql/execute.go @@ -45,7 +45,7 @@ func fillInPlaceholders( e, typ, "EXECUTE parameter", /* context */ &semaCtx, true /* allowImpure */) if err != nil { - return nil, pgerror.New(pgcode.WrongObjectType, err.Error()) + return nil, pgerror.WithCandidateCode(err, pgcode.WrongObjectType) } qArgs[idx] = typedExpr diff --git a/pkg/sql/export.go b/pkg/sql/export.go index 493bcb35d0e3..f841a93872e2 100644 --- a/pkg/sql/export.go +++ b/pkg/sql/export.go @@ -12,7 +12,6 @@ package sql import ( "context" - "fmt" "strconv" "strings" @@ -117,7 +116,7 @@ func (ef *execFactory) ConstructExport( if override, ok := optVals[exportOptionChunkSize]; ok { chunkSize, err = strconv.Atoi(override) if err != nil { - return nil, pgerror.New(pgcode.InvalidParameterValue, err.Error()) + return nil, pgerror.WithCandidateCode(err, pgcode.InvalidParameterValue) } if chunkSize < 1 { return nil, pgerror.New(pgcode.InvalidParameterValue, "invalid csv chunk size") @@ -131,7 +130,8 @@ func (ef *execFactory) ConstructExport( if strings.EqualFold(name, exportCompressionCodec) { codec = execinfrapb.FileCompression_Gzip } else { - return nil, pgerror.New(pgcode.InvalidParameterValue, fmt.Sprintf("unsupported compression codec %s", name)) + return nil, pgerror.Newf(pgcode.InvalidParameterValue, + "unsupported compression codec %s", name) } } diff --git a/pkg/sql/grant_role.go b/pkg/sql/grant_role.go index 7f17574d3cec..1ad3cded9b28 100644 --- a/pkg/sql/grant_role.go +++ b/pkg/sql/grant_role.go @@ -67,11 +67,12 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR continue } if isAdmin, ok := allRoles[string(r)]; !ok || !isAdmin { - errMsg := "%s is not a superuser or role admin for role %s" if string(r) == sqlbase.AdminRole { - errMsg = "%s is not a role admin for role %s" + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, + "%s is not a role admin for role %s", p.User(), r) } - return nil, pgerror.Newf(pgcode.InsufficientPrivilege, errMsg, p.User(), r) + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, + "%s is not a superuser or role admin for role %s", p.User(), r) } } diff --git a/pkg/sql/max_one_row.go b/pkg/sql/max_one_row.go index d84ec7c32751..e05982d54e50 100644 --- a/pkg/sql/max_one_row.go +++ b/pkg/sql/max_one_row.go @@ -57,7 +57,11 @@ func (m *max1RowNode) Next(params runParams) (bool, error) { var secondOk bool secondOk, err = m.plan.Next(params) if secondOk { - return false, pgerror.New(pgcode.CardinalityViolation, m.errorText) + // TODO(knz): m.errorText could be passed via log.Safe if there + // was a guarantee that it does not contain PII. Or better yet, + // the caller would construct an `error` object to return here + // instead of a string. + return false, pgerror.Newf(pgcode.CardinalityViolation, "%s", m.errorText) } } return ok, err diff --git a/pkg/sql/opt/exec/execbuilder/mutation.go b/pkg/sql/opt/exec/execbuilder/mutation.go index e00172c014ae..c8ac662789a5 100644 --- a/pkg/sql/opt/exec/execbuilder/mutation.go +++ b/pkg/sql/opt/exec/execbuilder/mutation.go @@ -691,7 +691,7 @@ func mkFKCheckErr(md *opt.Metadata, c *memo.FKChecksItem, keyVals tree.Datums) e } return errors.WithDetail( - pgerror.New(pgcode.ForeignKeyViolation, msg.String()), + pgerror.Newf(pgcode.ForeignKeyViolation, "%s", msg.String()), details.String(), ) } diff --git a/pkg/sql/opt/optbuilder/create_table.go b/pkg/sql/opt/optbuilder/create_table.go index 598e9d96d06e..3907841094aa 100644 --- a/pkg/sql/opt/optbuilder/create_table.go +++ b/pkg/sql/opt/optbuilder/create_table.go @@ -11,8 +11,6 @@ package optbuilder import ( - "fmt" - "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" "github.com/cockroachdb/cockroach/pkg/sql/opt/props/physical" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" @@ -76,10 +74,10 @@ func (b *Builder) buildCreateTable(ct *tree.CreateTable, inScope *scope) (outSco } numColumns := len(outScope.cols) if numColNames != 0 && numColNames != numColumns { - panic(sqlbase.NewSyntaxError(fmt.Sprintf( + panic(sqlbase.NewSyntaxErrorf( "CREATE TABLE specifies %d column name%s, but data source has %d column%s", numColNames, util.Pluralize(int64(numColNames)), - numColumns, util.Pluralize(int64(numColumns))))) + numColumns, util.Pluralize(int64(numColumns)))) } input = outScope.expr diff --git a/pkg/sql/opt/optbuilder/create_view.go b/pkg/sql/opt/optbuilder/create_view.go index 70133f285a0e..845b7044c20b 100644 --- a/pkg/sql/opt/optbuilder/create_view.go +++ b/pkg/sql/opt/optbuilder/create_view.go @@ -11,8 +11,6 @@ package optbuilder import ( - "fmt" - "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" @@ -46,11 +44,11 @@ func (b *Builder) buildCreateView(cv *tree.CreateView, inScope *scope) (outScope p := defScope.makePhysicalProps().Presentation if len(cv.ColumnNames) != 0 { if len(p) != len(cv.ColumnNames) { - panic(sqlbase.NewSyntaxError(fmt.Sprintf( + panic(sqlbase.NewSyntaxErrorf( "CREATE VIEW specifies %d column name%s, but data source has %d column%s", len(cv.ColumnNames), util.Pluralize(int64(len(cv.ColumnNames))), len(p), util.Pluralize(int64(len(p)))), - )) + ) } // Override the columns. for i := range p { diff --git a/pkg/sql/opt/optbuilder/scope.go b/pkg/sql/opt/optbuilder/scope.go index f5186ba561ed..e0f600991b31 100644 --- a/pkg/sql/opt/optbuilder/scope.go +++ b/pkg/sql/opt/optbuilder/scope.go @@ -25,6 +25,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/sql/types" + "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" ) @@ -1229,11 +1230,14 @@ func analyzeWindowFrame(s *scope, windowDef *tree.WindowDef) error { // At least one of the bounds is of type 'value' PRECEDING or 'value' FOLLOWING. // We require ordering on a single column that supports addition/subtraction. if len(windowDef.OrderBy) != 1 { - return pgerror.Newf(pgcode.Windowing, "RANGE with offset PRECEDING/FOLLOWING requires exactly one ORDER BY column") + return pgerror.Newf(pgcode.Windowing, + "RANGE with offset PRECEDING/FOLLOWING requires exactly one ORDER BY column") } requiredType = windowDef.OrderBy[0].Expr.(tree.TypedExpr).ResolvedType() if !types.IsAdditiveType(requiredType) { - return pgerror.Newf(pgcode.Windowing, fmt.Sprintf("RANGE with offset PRECEDING/FOLLOWING is not supported for column type %s", requiredType)) + return pgerror.Newf(pgcode.Windowing, + "RANGE with offset PRECEDING/FOLLOWING is not supported for column type %s", + log.Safe(requiredType)) } if types.IsDateTimeType(requiredType) { // Spec: for datetime ordering columns, the required type is an 'interval'. diff --git a/pkg/sql/parser/scan_test.go b/pkg/sql/parser/scan_test.go index a65b21de7b7f..e28240ace0aa 100644 --- a/pkg/sql/parser/scan_test.go +++ b/pkg/sql/parser/scan_test.go @@ -11,7 +11,6 @@ package parser import ( - "errors" "fmt" "reflect" "strings" @@ -20,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/lex" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/errors" ) func TestScanner(t *testing.T) { @@ -359,7 +359,7 @@ func TestScanError(t *testing.T) { if lval.id != ERROR { t.Errorf("%s: expected ERROR, but found %d", d.sql, lval.id) } - if !testutils.IsError(errors.New(lval.str), d.err) { + if !testutils.IsError(errors.Newf("%s", lval.str), d.err) { t.Errorf("%s: expected %s, but found %v", d.sql, d.err, lval.str) } } diff --git a/pkg/sql/pgwire/hba/parser.go b/pkg/sql/pgwire/hba/parser.go index 974ccfbf4cdf..c5f6d51f7760 100644 --- a/pkg/sql/pgwire/hba/parser.go +++ b/pkg/sql/pgwire/hba/parser.go @@ -114,8 +114,9 @@ func parseHbaLine(inputLine hbaLine) (entry Entry, err error) { case token.IsKeyword("all"): entry.Address = token case token.IsKeyword("samehost"), token.IsKeyword("samenet"): - return entry, unimplemented.New(fmt.Sprintf("hba-net-%s", token.Value), - "address specification "+token.Value+" is not yet supported") + return entry, unimplemented.Newf( + fmt.Sprintf("hba-net-%s", token.Value), + "address specification %s is not yet supported", errors.Safe(token.Value)) default: // Split name/mask. addr := token.Value diff --git a/pkg/sql/pgwire/pgerror/errors.go b/pkg/sql/pgwire/pgerror/errors.go index 6ef4821ed456..f3029d5ccffa 100644 --- a/pkg/sql/pgwire/pgerror/errors.go +++ b/pkg/sql/pgwire/pgerror/errors.go @@ -12,7 +12,6 @@ package pgerror import ( "bytes" - goErr "errors" "fmt" "regexp" "strings" @@ -106,7 +105,7 @@ func DangerousStatementf(format string, args ...interface{}) error { buf.WriteString("rejected: ") fmt.Fprintf(&buf, format, args...) buf.WriteString(" (sql_safe_updates = true)") - err := goErr.New(buf.String()) + err := errors.Newf("%s", buf.String()) err = errors.WithSafeDetails(err, format, args...) err = WithCandidateCode(err, pgcode.Warning) return err diff --git a/pkg/sql/pgwire/server.go b/pkg/sql/pgwire/server.go index d5dddaf7ec42..07f1421b069e 100644 --- a/pkg/sql/pgwire/server.go +++ b/pkg/sql/pgwire/server.go @@ -828,5 +828,5 @@ func (s *Server) sendErr(ctx context.Context, conn net.Conn, err error) error { } func newAdminShutdownErr(msg string) error { - return pgerror.Newf(pgcode.AdminShutdown, msg) + return pgerror.New(pgcode.AdminShutdown, msg) } diff --git a/pkg/sql/rename_database.go b/pkg/sql/rename_database.go index b73f2afabed1..b97a6346376c 100644 --- a/pkg/sql/rename_database.go +++ b/pkg/sql/rename_database.go @@ -151,11 +151,9 @@ func (n *renameDatabaseNode) startExec(params runParams) error { dependentDesc.ID, err, ) - msg := fmt.Sprintf( + return sqlbase.NewDependentObjectErrorf( "cannot rename database because a relation depends on relation %q", - tbTableName.String(), - ) - return sqlbase.NewDependentObjectError(msg) + tbTableName.String()) } } else { dependentDescTableName := tree.MakeTableNameWithSchema( @@ -165,7 +163,7 @@ func (n *renameDatabaseNode) startExec(params runParams) error { ) dependentDescQualifiedString = dependentDescTableName.String() } - msg := fmt.Sprintf( + depErr := sqlbase.NewDependentObjectErrorf( "cannot rename database because relation %q depends on relation %q", dependentDescQualifiedString, tbTableName.String(), @@ -185,12 +183,12 @@ func (n *renameDatabaseNode) startExec(params runParams) error { dbDesc.Name, ) } - return sqlbase.NewDependentObjectErrorWithHint(msg, hint) + return errors.WithHint(depErr, hint) } // Otherwise, we default to the view error message. - hint := fmt.Sprintf("you can drop %q instead", dependentDescQualifiedString) - return sqlbase.NewDependentObjectErrorWithHint(msg, hint) + return errors.WithHintf(depErr, + "you can drop %q instead", dependentDescQualifiedString) } } } diff --git a/pkg/sql/rename_table.go b/pkg/sql/rename_table.go index 30e6e81f2d6b..4cb3b7bd0ae9 100644 --- a/pkg/sql/rename_table.go +++ b/pkg/sql/rename_table.go @@ -12,13 +12,13 @@ package sql import ( "context" - "fmt" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/errors" ) type renameTableNode struct { @@ -180,13 +180,13 @@ func (p *planner) dependentViewRenameError( viewName, err = p.getQualifiedTableName(ctx, viewDesc) if err != nil { log.Warningf(ctx, "unable to retrieve name of view %d: %v", viewID, err) - msg := fmt.Sprintf("cannot rename %s %q because a view depends on it", + return sqlbase.NewDependentObjectErrorf( + "cannot rename %s %q because a view depends on it", typeName, objName) - return sqlbase.NewDependentObjectError(msg) } } - msg := fmt.Sprintf("cannot rename %s %q because view %q depends on it", - typeName, objName, viewName) - hint := fmt.Sprintf("you can drop %s instead.", viewName) - return sqlbase.NewDependentObjectErrorWithHint(msg, hint) + return errors.WithHintf( + sqlbase.NewDependentObjectErrorf("cannot rename %s %q because view %q depends on it", + typeName, objName, viewName), + "you can drop %s instead.", viewName) } diff --git a/pkg/sql/revoke_role.go b/pkg/sql/revoke_role.go index 72b49e78df31..deed193657d4 100644 --- a/pkg/sql/revoke_role.go +++ b/pkg/sql/revoke_role.go @@ -64,11 +64,12 @@ func (p *planner) RevokeRoleNode(ctx context.Context, n *tree.RevokeRole) (*Revo continue } if isAdmin, ok := allRoles[string(r)]; !ok || !isAdmin { - errMsg := "%s is not a superuser or role admin for role %s" if string(r) == sqlbase.AdminRole { - errMsg = "%s is not a role admin for role %s" + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, + "%s is not a role admin for role %s", p.User(), r) } - return nil, pgerror.Newf(pgcode.InsufficientPrivilege, errMsg, p.User(), r) + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, + "%s is not a superuser or role admin for role %s", p.User(), r) } } diff --git a/pkg/sql/rowexec/distinct.go b/pkg/sql/rowexec/distinct.go index ba8b4fd93297..e39c48ba6782 100644 --- a/pkg/sql/rowexec/distinct.go +++ b/pkg/sql/rowexec/distinct.go @@ -275,8 +275,15 @@ func (d *distinct) Next() (sqlbase.EncDatumRow, *execinfrapb.ProducerMetadata) { // Check whether row is distinct. if _, ok := d.seen[string(encoding)]; ok { if d.errorOnDup != "" { - // Row is a duplicate input to an Upsert operation, so raise an error. - err = pgerror.Newf(pgcode.CardinalityViolation, d.errorOnDup) + // Row is a duplicate input to an Upsert operation, so raise + // an error. + // + // TODO(knz): errorOnDup could be passed via log.Safe() if + // there was a guarantee that it does not contain PII. Or + // better yet, the caller would construct an `error` object to + // return here instead of a string. + // See: https://github.com/cockroachdb/cockroach/issues/48166 + err = pgerror.Newf(pgcode.CardinalityViolation, "%s", d.errorOnDup) d.MoveToDraining(err) break } @@ -321,7 +328,9 @@ func (d *sortedDistinct) Next() (sqlbase.EncDatumRow, *execinfrapb.ProducerMetad if matched { if d.errorOnDup != "" { // Row is a duplicate input to an Upsert operation, so raise an error. - err = pgerror.Newf(pgcode.CardinalityViolation, d.errorOnDup) + // TODO(knz): errorOnDup could be passed via log.Safe() if + // there was a guarantee that it does not contain PII. + err = pgerror.Newf(pgcode.CardinalityViolation, "%s", d.errorOnDup) d.MoveToDraining(err) break } diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index be31355a7e88..7208f11877d6 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -66,7 +66,7 @@ var ( errChrValueTooLarge = pgerror.Newf(pgcode.InvalidParameterValue, "input value must be <= %d (maximum Unicode code point)", utf8.MaxRune) errStringTooLarge = pgerror.Newf(pgcode.ProgramLimitExceeded, - fmt.Sprintf("requested length too large, exceeds %s", humanizeutil.IBytes(maxAllocatedStringSize))) + "requested length too large, exceeds %s", humanizeutil.IBytes(maxAllocatedStringSize)) ) const maxAllocatedStringSize = 128 * 1024 * 1024 @@ -2898,10 +2898,12 @@ may increase either contention or retry errors, or both.`, Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { errCode := string(*args[0].(*tree.DString)) msg := string(*args[1].(*tree.DString)) + // We construct the errors below via %s as the + // message may contain PII. if errCode == "" { - return nil, errors.New(msg) + return nil, errors.Newf("%s", msg) } - return nil, pgerror.New(errCode, msg) + return nil, pgerror.Newf(errCode, "%s", msg) }, Info: "This function is used only by CockroachDB's developers for testing purposes.", }, diff --git a/pkg/sql/sem/tree/name_resolution.go b/pkg/sql/sem/tree/name_resolution.go index 89fc74036c8c..f563a9eb0b7b 100644 --- a/pkg/sql/sem/tree/name_resolution.go +++ b/pkg/sql/sem/tree/name_resolution.go @@ -68,7 +68,7 @@ func classifyTablePattern(n *UnresolvedName) (TablePattern, error) { // Used e.g. in SELECT clauses. func classifyColumnItem(n *UnresolvedName) (VarName, error) { if n.NumParts < 1 || n.NumParts > 4 { - return nil, newInvColRef("invalid column name: %s", n) + return nil, newInvColRef(n) } // Check that all the parts specified are not empty. @@ -85,7 +85,7 @@ func classifyColumnItem(n *UnresolvedName) (VarName, error) { } for i := firstCheck; i < lastCheck; i++ { if len(n.Parts[i]) == 0 { - return nil, newInvColRef("invalid column name: %s", n) + return nil, newInvColRef(n) } } @@ -537,8 +537,9 @@ func (n *UnresolvedName) ResolveFunction( return def, nil } -func newInvColRef(fmt string, n *UnresolvedName) error { - return pgerror.NewWithDepthf(1, pgcode.InvalidColumnReference, fmt, n) +func newInvColRef(n *UnresolvedName) error { + return pgerror.NewWithDepthf(1, pgcode.InvalidColumnReference, + "invalid column name: %s", n) } func newInvTableNameError(n fmt.Stringer) error { diff --git a/pkg/sql/sqlbase/errors.go b/pkg/sql/sqlbase/errors.go index 1b65e98839c1..ac6ae09e8f09 100644 --- a/pkg/sql/sqlbase/errors.go +++ b/pkg/sql/sqlbase/errors.go @@ -15,7 +15,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" - "github.com/cockroachdb/errors" ) const ( @@ -122,26 +121,22 @@ func NewWrongObjectTypeError(name tree.NodeFormatter, desiredObjType string) err tree.ErrString(name), desiredObjType) } -// NewSyntaxError creates a syntax error. -func NewSyntaxError(msg string) error { - return pgerror.New(pgcode.Syntax, msg) +// NewSyntaxErrorf creates a syntax error. +func NewSyntaxErrorf(format string, args ...interface{}) error { + return pgerror.Newf(pgcode.Syntax, format, args...) } -// NewDependentObjectError creates a dependent object error. -func NewDependentObjectError(msg string) error { - return pgerror.New(pgcode.DependentObjectsStillExist, msg) -} - -// NewDependentObjectErrorWithHint creates a dependent object error with a hint -func NewDependentObjectErrorWithHint(msg string, hint string) error { - err := pgerror.New(pgcode.DependentObjectsStillExist, msg) - return errors.WithHint(err, hint) +// NewDependentObjectErrorf creates a dependent object error. +func NewDependentObjectErrorf(format string, args ...interface{}) error { + return pgerror.Newf(pgcode.DependentObjectsStillExist, format, args...) } // NewRangeUnavailableError creates an unavailable range error. func NewRangeUnavailableError( rangeID roachpb.RangeID, origErr error, nodeIDs ...roachpb.NodeID, ) error { + // TODO(knz): This could should really use errors.Wrap or + // errors.WithSecondaryError. return pgerror.Newf(pgcode.RangeUnavailable, "key range id:%d is unavailable; missing nodes: %s. Original error: %v", rangeID, nodeIDs, origErr) diff --git a/pkg/sql/sqlbase/structured.go b/pkg/sql/sqlbase/structured.go index 6b497df45a37..9b4770773fd0 100644 --- a/pkg/sql/sqlbase/structured.go +++ b/pkg/sql/sqlbase/structured.go @@ -2323,7 +2323,7 @@ func notIndexableError(cols []ColumnDescriptor, inverted bool) error { } } } - return unimplemented.NewWithIssueDetailf(35730, typInfo, msg) + return unimplemented.NewWithIssueDetailf(35730, typInfo, "%s", msg) } func checkColumnsValidForIndex(tableDesc *MutableTableDescriptor, indexColNames []string) error { @@ -2758,6 +2758,7 @@ func (desc *MutableTableDescriptor) DropConstraint( case ConstraintTypeFK: if detail.FK.Validity == ConstraintValidity_Validating { return unimplemented.NewWithIssueDetailf(42844, + "drop-constraint-fk-validating", "constraint %q in the middle of being added, try again later", name) } if detail.FK.Validity == ConstraintValidity_Dropping { diff --git a/pkg/sql/tests/monotonic_insert_test.go b/pkg/sql/tests/monotonic_insert_test.go index 4b2d2d9fdf0b..8e9d1e7e15c6 100644 --- a/pkg/sql/tests/monotonic_insert_test.go +++ b/pkg/sql/tests/monotonic_insert_test.go @@ -133,7 +133,7 @@ INSERT INTO mono.mono VALUES(-1, '0', -1, -1)`); err != nil { invoke := func(client mtClient) { logPrefix := fmt.Sprintf("%03d.%03d: ", atomic.AddUint64(&idGen, 1), client.ID) l := func(msg string, args ...interface{}) { - log.Infof(ctx, logPrefix+msg, args...) + log.Infof(ctx, logPrefix+msg /* nolint:fmtsafe */, args...) } l("begin") defer l("done") diff --git a/pkg/testutils/lint/passes/fmtsafe/fmtsafe.go b/pkg/testutils/lint/passes/fmtsafe/fmtsafe.go new file mode 100644 index 000000000000..7a585a3983cf --- /dev/null +++ b/pkg/testutils/lint/passes/fmtsafe/fmtsafe.go @@ -0,0 +1,283 @@ +// Copyright 2020 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package fmtsafe + +import ( + "fmt" + "go/ast" + "go/token" + "go/types" + "strings" + + "github.com/cockroachdb/errors" + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/go/types/typeutil" +) + +// Doc documents this pass. +var Doc = `checks that log and error functions don't leak PII. + +This linter checks the following: + +- that the format string in Infof(), Errorf() and similar calls is a + constant string. + + This check is essential for correctness because format strings + are assumed to be PII-free and always safe for reporting in + telemetry or PII-free logs. + +- that the message strings in errors.New() and similar calls that + construct error objects is a constant string. + + This check is provided to encourage hygiene: errors + constructed using non-constant strings are better constructed using + a formatting function instead, which makes the construction more + readable and encourage the provision of PII-free reportable details. + +It is possible for a call site *in a test file* to opt the format/message +string out of the linter using /* nolint:fmtsafe */ after the format +argument. This escape hatch is not available in non-test code. +` + +// Analyzer defines this pass. +var Analyzer = &analysis.Analyzer{ + Name: "fmtsafe", + Doc: Doc, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + // Our analyzer just wants to see function definitions + // and call points. + nodeFilter := []ast.Node{ + (*ast.FuncDecl)(nil), + (*ast.CallExpr)(nil), + } + + // fmtOrMsgStr, if non-nil, indicates an incoming + // format or message string in the argument list of the + // current function. + // + // The pointer is set at the beginning of every function declaration + // for a function recognized by this linter (= any of those listed + // in functions.go). In non-recognized function, it is set to nil to + // indicate there is no known format or message string. + var fmtOrMsgStr *types.Var + + // Now traverse the ASTs. The preorder traversal visits each + // function declaration node before its body, so we always get to + // set fmtOrMsgStr before the call points inside the body are + // visited. + inspect.Preorder(nodeFilter, func(n ast.Node) { + // Catch-all for possible bugs in the linter code. + defer func() { + if r := recover(); r != nil { + if err, ok := r.(error); ok { + pass.Reportf(n.Pos(), "internal linter error: %v", err) + return + } + panic(r) + } + }() + + if fd, ok := n.(*ast.FuncDecl); ok { + // This is the function declaration header. Obtain the formal + // parameter. + fmtOrMsgStr = maybeGetConstStr(pass, fd) + return + } + // At a call site. + call := n.(*ast.CallExpr) + checkCallExpr(pass, call, fmtOrMsgStr) + }) + return nil, nil +} + +func maybeGetConstStr(pass *analysis.Pass, fd *ast.FuncDecl) (res *types.Var) { + if fd.Body == nil { + // No body. Since there won't be any callee, take + // an early return. + return nil + } + + // What's the function being defined? + fn := pass.TypesInfo.Defs[fd.Name].(*types.Func) + if fn == nil { + return nil + } + fnName := fn.FullName() + + var wantVariadic bool + var argIdx int + + if requireConstFmt[fnName] { + // Expect a variadic function and the format parameter + // next-to-last in the parameter list. + wantVariadic = true + argIdx = -2 + } else if requireConstMsg[fnName] { + // Expect a non-variadic function and the format parameter last in + // the parameter list. + wantVariadic = false + argIdx = -1 + } else { + // Not a recognized function. Bail. + return nil + } + + sig := fn.Type().(*types.Signature) + if sig.Variadic() != wantVariadic { + panic(errors.Newf("expected variadic %v, got %v", wantVariadic, sig.Variadic())) + } + + params := sig.Params() + nparams := params.Len() + + // Is message or format param a string? + if nparams+argIdx < 0 { + panic(errors.New("not enough arguments")) + } + if p := params.At(nparams + argIdx); p.Type() == types.Typ[types.String] { + // Found it! + return p + } + return nil +} + +func checkCallExpr(pass *analysis.Pass, call *ast.CallExpr, fv *types.Var) { + // What's the function being called? + cfn := typeutil.Callee(pass.TypesInfo, call) + if cfn == nil { + // Not a call to a statically identified function. + // We can't lint. + return + } + fn, ok := cfn.(*types.Func) + if !ok { + // Not a function with a name. We can't lint either. + return + } + + // What's the full name of the callee? This includes the package + // path and, for methods, the type of the supporting object. + fnName := fn.FullName() + + var wantVariadic bool + var argIdx int + var argType string + + // Do the analysis of the callee. + if requireConstFmt[fnName] { + // Expect a variadic function and the format parameter + // next-to-last in the parameter list. + wantVariadic = true + argIdx = -2 + argType = "format" + } else if requireConstMsg[fnName] { + // Expect a non-variadic function and the format parameter last in + // the parameter list. + wantVariadic = false + argIdx = -1 + argType = "message" + } else { + // Not a recognized function. Bail. + return + } + + typ := pass.TypesInfo.Types[call.Fun].Type + if typ == nil { + panic(errors.New("can't find function type")) + } + + sig, ok := typ.(*types.Signature) + if !ok { + panic(errors.New("can't derive signature")) + } + if sig.Variadic() != wantVariadic { + panic(errors.Newf("expected variadic %v, got %v", wantVariadic, sig.Variadic())) + } + + idx := sig.Params().Len() + argIdx + if idx < 0 { + panic(errors.New("not enough parameters")) + } + + lit := pass.TypesInfo.Types[call.Args[idx]].Value + if lit != nil { + // A literal constant! All is well. + return + } + + // Not a constant. If it's a variable and the variable + // refers to the incoming format/message from the arg list, + // tolerate that. + if fv != nil { + if id, ok := call.Args[idx].(*ast.Ident); ok { + if pass.TypesInfo.ObjectOf(id) == fv { + // Same arg as incoming. All good. + return + } + } + } + + // If the argument is opting out of the linter with a special + // comment, tolerate that. + if hasNoLintComment(pass, call, idx) { + return + } + + pass.Reportf(call.Lparen, "%s argument is not a constant expression%s", argType, Tip) +} + +// Tip is exported for use in tests. +var Tip = ` +Tip: use YourFuncf("%s", ...) or list new formatting wrappers in pkg/testutils/lint/passes/fmtsafe/functions.go.` + +func hasNoLintComment(pass *analysis.Pass, call *ast.CallExpr, idx int) bool { + fPos, f := findContainingFile(pass, call) + + if !strings.HasSuffix(fPos.Name(), "_test.go") { + // The nolint: escape hatch is only supported in test files. + return false + } + + startPos := call.Args[idx].End() + endPos := call.Rparen + if idx < len(call.Args)-1 { + endPos = call.Args[idx+1].Pos() + } + for _, cg := range f.Comments { + if cg.Pos() > endPos || cg.End() < startPos { + continue + } + for _, c := range cg.List { + if strings.Contains(c.Text, "nolint:fmtsafe") { + return true + } + } + } + return false +} + +func findContainingFile(pass *analysis.Pass, n ast.Node) (*token.File, *ast.File) { + fPos := pass.Fset.File(n.Pos()) + for _, f := range pass.Files { + if pass.Fset.File(f.Pos()) == fPos { + return fPos, f + } + } + panic(fmt.Errorf("cannot file file for %v", n)) +} diff --git a/pkg/testutils/lint/passes/fmtsafe/fmtsafe_test.go b/pkg/testutils/lint/passes/fmtsafe/fmtsafe_test.go new file mode 100644 index 000000000000..2d3913c7d002 --- /dev/null +++ b/pkg/testutils/lint/passes/fmtsafe/fmtsafe_test.go @@ -0,0 +1,33 @@ +// Copyright 2020 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package fmtsafe_test + +import ( + "testing" + + "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/fmtsafe" + "golang.org/x/tools/go/analysis/analysistest" +) + +func Test(t *testing.T) { + if testutils.NightlyStress() { + t.Skip("Go cache files don't work under stress") + } + fmtsafe.Tip = "" + testdata := analysistest.TestData() + results := analysistest.Run(t, testdata, fmtsafe.Analyzer, "a") + for _, r := range results { + for _, d := range r.Diagnostics { + t.Logf("%s: %v: %s", r.Pass.Analyzer.Name, d.Pos, d.Message) + } + } +} diff --git a/pkg/testutils/lint/passes/fmtsafe/functions.go b/pkg/testutils/lint/passes/fmtsafe/functions.go new file mode 100644 index 000000000000..636ff4697275 --- /dev/null +++ b/pkg/testutils/lint/passes/fmtsafe/functions.go @@ -0,0 +1,157 @@ +// Copyright 2020 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package fmtsafe + +// requireConstMsg records functions for which the last string +// argument must be a constant string. +var requireConstMsg = map[string]bool{ + "errors.New": true, + + "github.com/pkg/errors.New": true, + "github.com/pkg/errors.Wrap": true, + + "github.com/cockroachdb/errors.New": true, + "github.com/cockroachdb/errors.Error": true, + "github.com/cockroachdb/errors.NewWithDepth": true, + "github.com/cockroachdb/errors.WithMessage": true, + "github.com/cockroachdb/errors.Wrap": true, + "github.com/cockroachdb/errors.WrapWithDepth": true, + "github.com/cockroachdb/errors.AssertionFailed": true, + "github.com/cockroachdb/errors.HandledWithMessage": true, + "github.com/cockroachdb/errors.HandledInDomainWithMessage": true, + + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror.New": true, + + "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented.New": true, + "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented.NewWithIssue": true, + "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented.NewWithIssueDetail": true, + + "github.com/cockroachdb/cockroach/pkg/sql/pgwire.newAdminShutdownErr": true, +} + +// requireConstFmt records functions for which the string arg +// before the final ellipsis must be a constant string. +var requireConstFmt = map[string]bool{ + // Logging things. + "log.Printf": true, + "log.Fatalf": true, + "log.Panicf": true, + "(*log.Logger).Fatalf": true, + "(*log.Logger).Panicf": true, + "(*log.Logger).Printf": true, + + "github.com/cockroachdb/cockroach/pkg/util/log.Infof": true, + "github.com/cockroachdb/cockroach/pkg/util/log.Warningf": true, + "github.com/cockroachdb/cockroach/pkg/util/log.Errorf": true, + "github.com/cockroachdb/cockroach/pkg/util/log.Eventf": true, + "github.com/cockroachdb/cockroach/pkg/util/log.VEventf": true, + "github.com/cockroachdb/cockroach/pkg/util/log.VErrEventf": true, + "github.com/cockroachdb/cockroach/pkg/util/log.InfofDepth": true, + "github.com/cockroachdb/cockroach/pkg/util/log.WarningfDepth": true, + "github.com/cockroachdb/cockroach/pkg/util/log.ErrorfDepth": true, + "github.com/cockroachdb/cockroach/pkg/util/log.FatalfDepth": true, + "github.com/cockroachdb/cockroach/pkg/util/log.VEventfDepth": true, + "github.com/cockroachdb/cockroach/pkg/util/log.VErrEventfDepth": true, + "github.com/cockroachdb/cockroach/pkg/util/log.ReportOrPanic": true, + + "(github.com/cockroachdb/cockroach/pkg/rpc.breakerLogger).Debugf": true, + "(github.com/cockroachdb/cockroach/pkg/rpc.breakerLogger).Infof": true, + + "(*github.com/cockroachdb/cockroach/pkg/internal/rsg/yacc.Tree).errorf": true, + + "(github.com/cockroachdb/cockroach/pkg/storage.pebbleLogger).Infof": true, + "(github.com/cockroachdb/cockroach/pkg/storage.pebbleLogger).Fatalf": true, + + "(*github.com/cockroachdb/cockroach/pkg/util/grpcutil.logger).Infof": true, + "(*github.com/cockroachdb/cockroach/pkg/util/grpcutil.logger).Warningf": true, + "(*github.com/cockroachdb/cockroach/pkg/util/grpcutil.logger).Errorf": true, + "(*github.com/cockroachdb/cockroach/pkg/util/grpcutil.logger).Fatalf": true, + + "(*github.com/cockroachdb/cockroach/pkg/kv/kvserver.raftLogger).Debugf": true, + "(*github.com/cockroachdb/cockroach/pkg/kv/kvserver.raftLogger).Infof": true, + "(*github.com/cockroachdb/cockroach/pkg/kv/kvserver.raftLogger).Warningf": true, + "(*github.com/cockroachdb/cockroach/pkg/kv/kvserver.raftLogger).Errorf": true, + "(*github.com/cockroachdb/cockroach/pkg/kv/kvserver.raftLogger).Fatalf": true, + "(*github.com/cockroachdb/cockroach/pkg/kv/kvserver.raftLogger).Panicf": true, + + "(go.etcd.io/etcd/raft.Logger).Debugf": true, + "(go.etcd.io/etcd/raft.Logger).Infof": true, + "(go.etcd.io/etcd/raft.Logger).Warningf": true, + "(go.etcd.io/etcd/raft.Logger).Errorf": true, + "(go.etcd.io/etcd/raft.Logger).Fatalf": true, + "(go.etcd.io/etcd/raft.Logger).Panicf": true, + + "(google.golang.org/grpc/grpclog.Logger).Infof": true, + "(google.golang.org/grpc/grpclog.Logger).Warningf": true, + "(google.golang.org/grpc/grpclog.Logger).Errorf": true, + + "(github.com/cockroachdb/pebble.Logger).Infof": true, + "(github.com/cockroachdb/pebble.Logger).Fatalf": true, + + "(github.com/cockroachdb/circuitbreaker.Logger).Infof": true, + "(github.com/cockroachdb/circuitbreaker.Logger).Debugf": true, + + "github.com/cockroachdb/cockroach/pkg/sql/opt/optgen/exprgen.errorf": true, + "github.com/cockroachdb/cockroach/pkg/sql/opt/optgen/exprgen.wrapf": true, + + "(*github.com/cockroachdb/cockroach/pkg/sql.connExecutor).sessionEventf": true, + + "(*github.com/cockroachdb/cockroach/pkg/sql/logictest.logicTest).outf": true, + "(*github.com/cockroachdb/cockroach/pkg/sql/logictest.logicTest).Errorf": true, + "(*github.com/cockroachdb/cockroach/pkg/sql/logictest.logicTest).Fatalf": true, + + "(*github.com/cockroachdb/cockroach/pkg/server.adminServer).serverErrorf": true, + + "(*github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl.kafkaLogAdapter).Printf": true, + + // Error things. + "fmt.Errorf": true, + + "github.com/pkg/errors.Errorf": true, + "github.com/pkg/errors.Wrapf": true, + + "github.com/cockroachdb/errors.Newf": true, + "github.com/cockroachdb/errors.Errorf": true, + "github.com/cockroachdb/errors.NewWithDepthf": true, + "github.com/cockroachdb/errors.WithMessagef": true, + "github.com/cockroachdb/errors.Wrapf": true, + "github.com/cockroachdb/errors.WrapWithDepthf": true, + "github.com/cockroachdb/errors.AssertionFailedf": true, + "github.com/cockroachdb/errors.AssertionFailedWithDepthf": true, + "github.com/cockroachdb/errors.NewAssertionErrorWithWrappedErrf": true, + "github.com/cockroachdb/errors.WithSafeDetails": true, + + "github.com/cockroachdb/cockroach/pkg/roachpb.NewErrorf": true, + + "github.com/cockroachdb/cockroach/pkg/ccl/importccl.makeRowErr": true, + "github.com/cockroachdb/cockroach/pkg/ccl/importccl.wrapRowErr": true, + + "github.com/cockroachdb/cockroach/pkg/sql/sqlbase.NewSyntaxErrorf": true, + "github.com/cockroachdb/cockroach/pkg/sql/sqlbase.NewDependentObjectErrorf": true, + + "github.com/cockroachdb/cockroach/pkg/sql/sem/tree.newSourceNotFoundError": true, + + "github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder.unimplementedWithIssueDetailf": true, + + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror.Newf": true, + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror.NewWithDepthf": true, + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror.Noticef": true, + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror.DangerousStatementf": true, + + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgwirebase.NewProtocolViolationErrorf": true, + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgwirebase.NewInvalidBinaryRepresentationErrorf": true, + + "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented.Newf": true, + "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented.NewWithDepthf": true, + "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented.NewWithIssuef": true, + "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented.NewWithIssueDetailf": true, + "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented.unimplementedInternal": true, +} diff --git a/pkg/testutils/lint/passes/fmtsafe/testdata/src/a/a.go b/pkg/testutils/lint/passes/fmtsafe/testdata/src/a/a.go new file mode 100644 index 000000000000..48b54949dcd6 --- /dev/null +++ b/pkg/testutils/lint/passes/fmtsafe/testdata/src/a/a.go @@ -0,0 +1,72 @@ +// Copyright 2020 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package a + +import ( + "context" + + "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/errors" + "go.etcd.io/etcd/raft" +) + +var unsafeStr = "abc %d" + +const constOk = "safe %d" + +func init() { + _ = recover() + + _ = errors.New(unsafeStr) // want `message argument is not a constant expression` + + // Even though the following is trying to opt out of the linter, + // the opt out fails because the code is not in a test. + + _ = errors.New(unsafeStr /*nolint:fmtsafe*/) // want `message argument is not a constant expression` + + _ = errors.New("safestr") + _ = errors.New(constOk) + _ = errors.New("abo" + constOk) + _ = errors.New("abo" + unsafeStr) // want `message argument is not a constant expression` + + _ = errors.Newf("safe %d", 123) + _ = errors.Newf(constOk, 123) + _ = errors.Newf(unsafeStr, 123) // want `format argument is not a constant expression` + _ = errors.Newf("abo"+constOk, 123) + _ = errors.Newf("abo"+unsafeStr, 123) // want `format argument is not a constant expression` + + ctx := context.Background() + + log.Errorf(ctx, "safe %d", 123) + log.Errorf(ctx, constOk, 123) + log.Errorf(ctx, unsafeStr, 123) // want `format argument is not a constant expression` + log.Errorf(ctx, "abo"+constOk, 123) + log.Errorf(ctx, "abo"+unsafeStr, 123) // want `format argument is not a constant expression` + + var m myLogger + var l raft.Logger = m + + l.Infof("safe %d", 123) + l.Infof(constOk, 123) + l.Infof(unsafeStr, 123) // want `format argument is not a constant expression` + l.Infof("abo"+constOk, 123) + l.Infof("abo"+unsafeStr, 123) // want `format argument is not a constant expression` +} + +type myLogger struct{} + +func (m myLogger) Info(args ...interface{}) { + log.Error(context.Background(), args...) +} + +func (m myLogger) Infof(_ string, args ...interface{}) { + log.Errorf(context.Background(), "ignoredfmt", args...) +} diff --git a/pkg/testutils/lint/passes/fmtsafe/testdata/src/a/a_test.go b/pkg/testutils/lint/passes/fmtsafe/testdata/src/a/a_test.go new file mode 100644 index 000000000000..973570c8fe93 --- /dev/null +++ b/pkg/testutils/lint/passes/fmtsafe/testdata/src/a/a_test.go @@ -0,0 +1,22 @@ +// Copyright 2020 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package a + +import ( + "testing" + + "github.com/cockroachdb/errors" +) + +func Test(t *testing.T) { + _ = errors.New(unsafeStr) // want `message argument is not a constant expression` + _ = errors.New(unsafeStr /*nolint:fmtsafe*/) +} diff --git a/pkg/testutils/lint/passes/fmtsafe/testdata/src/github.com/cockroachdb/cockroach/pkg/util/log/log.go b/pkg/testutils/lint/passes/fmtsafe/testdata/src/github.com/cockroachdb/cockroach/pkg/util/log/log.go new file mode 100644 index 000000000000..70299233697e --- /dev/null +++ b/pkg/testutils/lint/passes/fmtsafe/testdata/src/github.com/cockroachdb/cockroach/pkg/util/log/log.go @@ -0,0 +1,24 @@ +// Copyright 2020 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package log + +import ( + "context" + "fmt" +) + +func Errorf(ctx context.Context, format string, args ...interface{}) { + fmt.Println(fmt.Sprintf(format, args...)) +} + +func Error(ctx context.Context, msg ...interface{}) { + fmt.Println(msg...) +} diff --git a/pkg/testutils/lint/passes/fmtsafe/testdata/src/github.com/cockroachdb/errors/errors.go b/pkg/testutils/lint/passes/fmtsafe/testdata/src/github.com/cockroachdb/errors/errors.go new file mode 100644 index 000000000000..2f90ea9548ca --- /dev/null +++ b/pkg/testutils/lint/passes/fmtsafe/testdata/src/github.com/cockroachdb/errors/errors.go @@ -0,0 +1,21 @@ +// Copyright 2020 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package errors + +import "fmt" + +func New(msg string) error { + return fmt.Errorf("abc") +} + +func Newf(format string, args ...interface{}) error { + return fmt.Errorf(format, args...) +} diff --git a/pkg/testutils/lint/passes/fmtsafe/testdata/src/go.etcd.io/etcd/raft/log.go b/pkg/testutils/lint/passes/fmtsafe/testdata/src/go.etcd.io/etcd/raft/log.go new file mode 100644 index 000000000000..05208e6d7891 --- /dev/null +++ b/pkg/testutils/lint/passes/fmtsafe/testdata/src/go.etcd.io/etcd/raft/log.go @@ -0,0 +1,16 @@ +// Copyright 2020 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package raft + +type Logger interface { + Info(args ...interface{}) + Infof(format string, args ...interface{}) +} diff --git a/pkg/util/errorutil/tenant.go b/pkg/util/errorutil/tenant.go index 95b6ac66d93f..27aac2cff6ed 100644 --- a/pkg/util/errorutil/tenant.go +++ b/pkg/util/errorutil/tenant.go @@ -11,9 +11,10 @@ package errorutil import ( - "errors" "fmt" "strings" + + "github.com/cockroachdb/errors" ) // UnsupportedWithMultiTenancy returns an error suitable for returning when an @@ -30,5 +31,7 @@ func UnsupportedWithMultiTenancy(issues ...int) error { fmt.Fprint(&buf, prefix, n) } } - return errors.New(buf.String()) + // TODO(knz): This should be done in a different way, + // see: https://github.com/cockroachdb/cockroach/issues/48164 + return errors.Newf("%s", buf.String()) } diff --git a/pkg/util/errorutil/unimplemented/unimplemented.go b/pkg/util/errorutil/unimplemented/unimplemented.go index e31031ef9c76..c71dc0fb0452 100644 --- a/pkg/util/errorutil/unimplemented/unimplemented.go +++ b/pkg/util/errorutil/unimplemented/unimplemented.go @@ -49,15 +49,6 @@ func NewWithIssuef(issue int, format string, args ...interface{}) error { return unimplementedInternal(1 /*depth*/, issue, "" /*detail*/, true /*format*/, format, args...) } -// NewWithIssueHint constructs an error with the given -// message, hint, and a link to the passed issue. Recorded as "#" -// in tracking. -func NewWithIssueHint(issue int, msg, hint string) error { - err := unimplementedInternal(1 /*depth*/, issue, "" /*detail*/, false /*format*/, msg) - err = errors.WithHint(err, hint) - return err -} - // NewWithIssueDetail constructs an error with the given message // and a link to the passed issue. Recorded as "#.detail" in tracking. // This is useful when we need an extra axis of information to drill down into. diff --git a/pkg/util/errorutil/unimplemented/unimplemented_test.go b/pkg/util/errorutil/unimplemented/unimplemented_test.go index 2319b4e535c7..b5a14f54ec6d 100644 --- a/pkg/util/errorutil/unimplemented/unimplemented_test.go +++ b/pkg/util/errorutil/unimplemented/unimplemented_test.go @@ -31,7 +31,6 @@ func TestUnimplemented(t *testing.T) { {NewWithDepthf(1, "woo", "hello %s", "world"), "unimplemented: hello world", "woo", "", 0}, {NewWithIssue(123, "waa"), "unimplemented: waa", "", "", 123}, {NewWithIssuef(123, "hello %s", "world"), "unimplemented: hello world", "", "", 123}, - {NewWithIssueHint(123, "waa", "woo"), "unimplemented: waa", "", "woo", 123}, {NewWithIssueDetail(123, "waa", "woo"), "unimplemented: woo", "waa", "", 123}, {NewWithIssueDetailf(123, "waa", "hello %s", "world"), "unimplemented: hello world", "waa", "", 123}, } diff --git a/pkg/util/leaktest/leaktest.go b/pkg/util/leaktest/leaktest.go index bfc8a6a6e625..609cd4927395 100644 --- a/pkg/util/leaktest/leaktest.go +++ b/pkg/util/leaktest/leaktest.go @@ -30,6 +30,7 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/util/timeutil" + "github.com/cockroachdb/errors" "github.com/petermattis/goid" ) @@ -142,5 +143,5 @@ func diffGoroutines(base map[int64]string) error { for _, g := range leaked { b.WriteString(fmt.Sprintf("Leaked goroutine: %v\n\n", g)) } - return fmt.Errorf(b.String()) + return errors.Newf("%s", b.String()) } diff --git a/pkg/util/mon/bytes_usage.go b/pkg/util/mon/bytes_usage.go index 7e3802a10786..0ad5406809b6 100644 --- a/pkg/util/mon/bytes_usage.go +++ b/pkg/util/mon/bytes_usage.go @@ -396,7 +396,8 @@ func (mm *BytesMonitor) doStop(ctx context.Context, check bool) { if check && mm.mu.curAllocated != 0 { log.ReportOrPanic( ctx, &mm.settings.SV, - fmt.Sprintf("%s: unexpected %d leftover bytes", mm.name, mm.mu.curAllocated)) + "%s: unexpected %d leftover bytes", + log.Safe(mm.name), log.Safe(mm.mu.curAllocated)) mm.releaseBytes(ctx, mm.mu.curAllocated) }