diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 6d7f0f302260..08d19bb3c780 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -265,6 +265,7 @@ ALL_TESTS = [ "//pkg/testutils/lint/passes/forbiddenmethod:forbiddenmethod_test", "//pkg/testutils/lint/passes/hash:hash_test", "//pkg/testutils/lint/passes/leaktestcall:leaktestcall_test", + "//pkg/testutils/lint/passes/nilness:nilness_test", "//pkg/testutils/lint/passes/nocopy:nocopy_test", "//pkg/testutils/lint/passes/passesutil:passesutil_test", "//pkg/testutils/lint/passes/returnerrcheck:returnerrcheck_test", diff --git a/pkg/cmd/roachtest/cluster.go b/pkg/cmd/roachtest/cluster.go index b0de67d5be4d..af347bf3bb85 100644 --- a/pkg/cmd/roachtest/cluster.go +++ b/pkg/cmd/roachtest/cluster.go @@ -1698,7 +1698,7 @@ WHERE t.status NOT IN ('RANGE_CONSISTENT', 'RANGE_INDETERMINATE')`) for rows.Next() { var rangeID int32 var prettyKey, status, detail string - if scanErr := rows.Scan(&rangeID, &prettyKey, &status, &detail); err != nil { + if scanErr := rows.Scan(&rangeID, &prettyKey, &status, &detail); scanErr != nil { return scanErr } finalErr = errors.CombineErrors(finalErr, diff --git a/pkg/cmd/roachvet/BUILD.bazel b/pkg/cmd/roachvet/BUILD.bazel index 17d9f8057fc3..8936679e1f70 100644 --- a/pkg/cmd/roachvet/BUILD.bazel +++ b/pkg/cmd/roachvet/BUILD.bazel @@ -11,6 +11,7 @@ go_library( "//pkg/testutils/lint/passes/forbiddenmethod", "//pkg/testutils/lint/passes/hash", "//pkg/testutils/lint/passes/leaktestcall", + "//pkg/testutils/lint/passes/nilness", "//pkg/testutils/lint/passes/nocopy", "//pkg/testutils/lint/passes/returnerrcheck", "//pkg/testutils/lint/passes/timer", diff --git a/pkg/cmd/roachvet/main.go b/pkg/cmd/roachvet/main.go index a7e7b96ada17..3030a6b5800a 100644 --- a/pkg/cmd/roachvet/main.go +++ b/pkg/cmd/roachvet/main.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/forbiddenmethod" "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/hash" "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/leaktestcall" + "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/nilness" "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/nocopy" "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/returnerrcheck" "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/timer" @@ -63,6 +64,7 @@ func main() { unconvert.Analyzer, fmtsafe.Analyzer, errcmp.Analyzer, + nilness.CRDBAnalyzer, ) // Standard go vet analyzers: diff --git a/pkg/cmd/smithtest/main.go b/pkg/cmd/smithtest/main.go index fedaa0686147..3d7f2d3754e9 100644 --- a/pkg/cmd/smithtest/main.go +++ b/pkg/cmd/smithtest/main.go @@ -188,9 +188,6 @@ func (s WorkerSetup) run(ctx context.Context, rnd *rand.Rand) error { return errors.Wrap(err, "connector error") } db = gosql.OpenDB(connector) - if err != nil { - return errors.Wrap(err, "connect") - } fmt.Println("connected to", match[1]) break } diff --git a/pkg/geo/geomfn/linestring.go b/pkg/geo/geomfn/linestring.go index 526ca72ec9d3..4ce9d12d8366 100644 --- a/pkg/geo/geomfn/linestring.go +++ b/pkg/geo/geomfn/linestring.go @@ -91,10 +91,6 @@ func LineLocatePoint(line geo.Geometry, point geo.Geometry) (float64, error) { } p := closestT.(*geom.Point) - if err != nil { - return 0, err - } - // build new line segment to the closest point we found lineSegment := geom.NewLineString(geom.XY).MustSetCoords([]geom.Coord{lineString.Coord(0), p.Coords()}) diff --git a/pkg/jobs/job_scheduler.go b/pkg/jobs/job_scheduler.go index c60b9b7c7b98..eee7f9dc2f03 100644 --- a/pkg/jobs/job_scheduler.go +++ b/pkg/jobs/job_scheduler.go @@ -88,7 +88,7 @@ SELECT FROM %s S WHERE next_run < %s ORDER BY random() -%s +%s FOR UPDATE`, env.SystemJobsTableName(), CreatedByScheduledJobs, StatusSucceeded, StatusCanceled, StatusFailed, env.ScheduledJobsTableName(), env.NowExpr(), limitClause) @@ -313,7 +313,7 @@ func (s *jobScheduler) executeSchedules( return s.processSchedule(ctx, schedule, numRunning, stats, txn) }); processErr != nil { if errors.HasType(processErr, (*savePointError)(nil)) { - return errors.Wrapf(err, "savepoint error for schedule %d", schedule.ScheduleID()) + return errors.Wrapf(processErr, "savepoint error for schedule %d", schedule.ScheduleID()) } // Failed to process schedule. diff --git a/pkg/jobs/registry.go b/pkg/jobs/registry.go index 1e38a3fb3872..3b795d2b9cf9 100644 --- a/pkg/jobs/registry.go +++ b/pkg/jobs/registry.go @@ -1200,7 +1200,7 @@ func (r *Registry) stepThroughStateMachine( jm.ResumeFailed.Inc(1) if sErr := (*InvalidStatusError)(nil); errors.As(err, &sErr) { if sErr.status != StatusCancelRequested && sErr.status != StatusPauseRequested { - return errors.NewAssertionErrorWithWrappedErrf(jobErr, + return errors.NewAssertionErrorWithWrappedErrf(sErr, "job %d: unexpected status %s provided for a running job", job.ID(), sErr.status) } return sErr @@ -1281,8 +1281,7 @@ func (r *Registry) stepThroughStateMachine( errors.Wrapf(err, "job %d: cannot be reverted, manual cleanup may be required", job.ID())) case StatusFailed: if jobErr == nil { - return errors.NewAssertionErrorWithWrappedErrf(jobErr, - "job %d: has StatusFailed but no error was provided", job.ID()) + return errors.AssertionFailedf("job %d: has StatusFailed but no error was provided", job.ID()) } if err := job.failed(ctx, nil /* txn */, jobErr, nil /* fn */); err != nil { // If we can't transactionally mark the job as failed then it will be diff --git a/pkg/server/init.go b/pkg/server/init.go index 777fffcbb217..b41f67096e74 100644 --- a/pkg/server/init.go +++ b/pkg/server/init.go @@ -272,13 +272,11 @@ func (s *initServer) ServeAndWait( return nil, false, err } - if err != nil { - // We expect the join RPC to blindly retry on all - // "connection" errors save for one above. If we're - // here, we failed to initialize our first store after a - // successful join attempt. - return nil, false, errors.NewAssertionErrorWithWrappedErrf(err, "unexpected error: %v", err) - } + // We expect the join RPC to blindly retry on all + // "connection" errors save for one above. If we're + // here, we failed to initialize our first store after a + // successful join attempt. + return nil, false, errors.NewAssertionErrorWithWrappedErrf(err, "unexpected error: %v", err) } state := result.state diff --git a/pkg/server/init_handshake.go b/pkg/server/init_handshake.go index 5642ee29ba2e..7919cd823d0f 100644 --- a/pkg/server/init_handshake.go +++ b/pkg/server/init_handshake.go @@ -348,15 +348,13 @@ func (t *tlsInitHandshaker) getPeerCACert( return nodeHostnameAndCA{}, err } - // Read and validate server provided ack. - // HMAC(hostname + server CA public certificate, secretToken) - var msg nodeHostnameAndCA - if err != nil { - return nodeHostnameAndCA{}, err - } if res.StatusCode != 200 { return nodeHostnameAndCA{}, errors.Errorf("unexpected error returned from peer: HTTP %d", res.StatusCode) } + + // Read and validate server provided ack. + // HMAC(hostname + server CA public certificate, secretToken) + var msg nodeHostnameAndCA if err := json.NewDecoder(res.Body).Decode(&msg); err != nil { return nodeHostnameAndCA{}, err } diff --git a/pkg/sql/alter_sequence.go b/pkg/sql/alter_sequence.go index cdf80c11ea48..e3e55a33d3ad 100644 --- a/pkg/sql/alter_sequence.go +++ b/pkg/sql/alter_sequence.go @@ -74,9 +74,6 @@ func (n *alterSequenceNode) startExec(params runParams) error { } opts := desc.SequenceOpts seqValueKey := params.p.ExecCfg().Codec.SequenceKey(uint32(desc.ID)) - if err != nil { - return err - } getSequenceValue := func() (int64, error) { kv, err := params.p.txn.Get(params.ctx, seqValueKey) diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index db419db4419e..543058dc8b8e 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -620,9 +620,6 @@ func (n *alterTableNode) startExec(params runParams) error { descriptorChanged = true } - if err != nil { - return err - } if err := params.p.removeColumnComment(params.ctx, n.tableDesc.ID, colToDrop.GetID()); err != nil { return err } diff --git a/pkg/sql/opt/optbuilder/orderby.go b/pkg/sql/opt/optbuilder/orderby.go index 2bb7a12ea9fa..1010897959eb 100644 --- a/pkg/sql/opt/optbuilder/orderby.go +++ b/pkg/sql/opt/optbuilder/orderby.go @@ -133,10 +133,6 @@ func (b *Builder) analyzeOrderByIndex( for i, n := 0, index.KeyColumnCount(); i < n; i++ { // Columns which are indexable are always orderable. col := index.Column(i) - if err != nil { - panic(err) - } - desc := col.Descending // DESC inverts the order of the index. diff --git a/pkg/sql/pgwire/conn.go b/pkg/sql/pgwire/conn.go index 0bc2d7349088..969d51a57f49 100644 --- a/pkg/sql/pgwire/conn.go +++ b/pkg/sql/pgwire/conn.go @@ -288,7 +288,8 @@ func (c *conn) serveImpl( // we only need the minimum to make pgx happy. var err error for param, value := range testingStatusReportParams { - if err := c.sendParamStatus(param, value); err != nil { + err = c.sendParamStatus(param, value) + if err != nil { break } } diff --git a/pkg/sql/sem/tree/create.go b/pkg/sql/sem/tree/create.go index 6df2e1aa909f..0eeaea135bf9 100644 --- a/pkg/sql/sem/tree/create.go +++ b/pkg/sql/sem/tree/create.go @@ -1656,10 +1656,6 @@ func (o KVOptions) ToRoleOptions( } } else { strFn, err := typeAsStringOrNull(ro.Value, op) - if err != nil { - return nil, err - } - if err != nil { return nil, err } diff --git a/pkg/storage/cloudimpl/workload_storage.go b/pkg/storage/cloudimpl/workload_storage.go index 13a481e26fa0..15865f0dbba6 100644 --- a/pkg/storage/cloudimpl/workload_storage.go +++ b/pkg/storage/cloudimpl/workload_storage.go @@ -76,7 +76,7 @@ func makeWorkloadStorage( } } if s.table.Name == `` { - return nil, errors.Wrapf(err, `unknown table %s for generator %s`, conf.Table, meta.Name) + return nil, errors.Errorf(`unknown table %s for generator %s`, conf.Table, meta.Name) } return s, nil } diff --git a/pkg/testutils/lint/lint_test.go b/pkg/testutils/lint/lint_test.go index c966875c9ccd..8b4fd2016d05 100644 --- a/pkg/testutils/lint/lint_test.go +++ b/pkg/testutils/lint/lint_test.go @@ -2048,6 +2048,8 @@ func TestLint(t *testing.T) { // roachtest is not collecting redactable logs so we don't care // about printf hygiene there as much. stream.GrepNot(`pkg/cmd/roachtest/log\.go:.*format argument is not a constant expression`), + // We purposefully produce nil dereferences in this file to test crash conditions + stream.GrepNot(`pkg/util/log/logcrash/crash_reporting_test\.go:.*nil dereference in type assertion`), } const vetTool = "roachvet" diff --git a/pkg/testutils/lint/passes/nilness/BUILD.bazel b/pkg/testutils/lint/passes/nilness/BUILD.bazel new file mode 100644 index 000000000000..728a4e56e181 --- /dev/null +++ b/pkg/testutils/lint/passes/nilness/BUILD.bazel @@ -0,0 +1,24 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "nilness", + srcs = ["nilness.go"], + importpath = "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/nilness", + visibility = ["//visibility:public"], + deps = [ + "@org_golang_x_tools//go/analysis", + "@org_golang_x_tools//go/analysis/passes/buildssa", + "@org_golang_x_tools//go/ssa", + ], +) + +go_test( + name = "nilness_test", + srcs = ["nilness_test.go"], + data = glob(["testdata/**"]), + tags = ["broken_in_bazel"], + deps = [ + ":nilness", + "@org_golang_x_tools//go/analysis/analysistest", + ], +) diff --git a/pkg/testutils/lint/passes/nilness/nilness.go b/pkg/testutils/lint/passes/nilness/nilness.go new file mode 100644 index 000000000000..3dafd905489f --- /dev/null +++ b/pkg/testutils/lint/passes/nilness/nilness.go @@ -0,0 +1,441 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Portions of this file are additionally subject to the following +// license and copyright. +// +// Copyright 2021 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 nilness inspects the control-flow graph of an SSA function +// and reports errors such as nil pointer dereferences and degenerate +// nil pointer comparisons. +package nilness + +import ( + "fmt" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/buildssa" + "golang.org/x/tools/go/ssa" +) + +const doc = `check for redundant or impossible nil comparisons + +The nilness checker inspects the control-flow graph of each function in +a package and reports nil pointer dereferences, degenerate nil +pointers, and panics with nil values. A degenerate comparison is of the form +x==nil or x!=nil where x is statically known to be nil or non-nil. These are +often a mistake, especially in control flow related to errors. Panics with nil +values are checked because they are not detectable by + + if r := recover(); r != nil { + +This check reports conditions such as: + + if f == nil { // impossible condition (f is a function) + } + +and: + + p := &v + ... + if p != nil { // tautological condition + } + +and: + + if p == nil { + print(*p) // nil dereference + } + +and: + + if p == nil { + panic(p) + } +` + +type argExtractor func(c *ssa.CallCommon) ssa.Value + +type analyzerConfig struct { + // reportDegenerateConditions controls the reporting of "impossible" + // and "tautological" comparisons. While many of these are in error, + // it also catches nil checks where the current control flow graph + // can't produce nil but where we might want to guard against changes + // in the future. It also catches a number of compound conditionals + // where the "tautological" check produces more readable code. + reportDegenerateIfConditions bool + // argExtractors is a map from the full name of a function to + // a func that returns the argument from a Call that should + // not be passed provably nil values. + argExtractors map[string]argExtractor +} + +var crdbConfig = analyzerConfig{ + reportDegenerateIfConditions: false, + argExtractors: map[string]argExtractor{ + "github.com/cockroachdb/errors.Handled": firstArg, + "github.com/cockroachdb/errors.HandledWithMessage": firstArg, + "github.com/cockroachdb/errors.NewAssertionErrorWithWrappedErrf": firstArg, + "github.com/cockroachdb/errors.Mark": firstArg, + "github.com/cockroachdb/errors.WithContextTags": firstArg, + "github.com/cockroachdb/errors.WithDetail": firstArg, + "github.com/cockroachdb/errors.WithDetailf": firstArg, + "github.com/cockroachdb/errors.WithHint": firstArg, + "github.com/cockroachdb/errors.WithHintf": firstArg, + "github.com/cockroachdb/errors.WithMessage": firstArg, + "github.com/cockroachdb/errors.WithMessagef": firstArg, + "github.com/cockroachdb/errors.WithStack": firstArg, + "github.com/cockroachdb/errors.WithStackDepth": firstArg, + "github.com/cockroachdb/errors.Wrap": firstArg, + "github.com/cockroachdb/errors.Wrapf": firstArg, + "github.com/cockroachdb/errors.WrapWithDepth": firstArg, + "github.com/cockroachdb/errors.WrapWithDepthf": firstArg, + }, +} + +var defaultConfig = analyzerConfig{ + reportDegenerateIfConditions: true, +} + +// CRDBAnalyzer defines a pass that checks for uses of provably nil +// values that were likely in error with custom configuruation +// suitable for the CRDB codebase. +var CRDBAnalyzer = &analysis.Analyzer{ + Name: "nilness", + Doc: doc, + Run: crdbConfig.run, + Requires: []*analysis.Analyzer{buildssa.Analyzer}, +} + +// Analyzer defines a pass that checks for uses of provably nil values +// that were likely in error. +var Analyzer = &analysis.Analyzer{ + Name: "nilness", + Doc: doc, + Run: defaultConfig.run, + Requires: []*analysis.Analyzer{buildssa.Analyzer}, +} + +func (a *analyzerConfig) run(pass *analysis.Pass) (interface{}, error) { + ssainput := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA) + for _, fn := range ssainput.SrcFuncs { + a.runFunc(pass, fn) + } + return nil, nil +} + +func (a *analyzerConfig) runFunc(pass *analysis.Pass, fn *ssa.Function) { + reportf := func(category string, pos token.Pos, format string, args ...interface{}) { + pass.Report(analysis.Diagnostic{ + Pos: pos, + Category: category, + Message: fmt.Sprintf(format, args...), + }) + } + + // notNil reports an error if v is provably nil. + notNil := func(stack []fact, instr ssa.Instruction, v ssa.Value, descr string) { + if nilnessOf(stack, v) == isnil { + reportf("nilderef", instr.Pos(), "nil dereference in "+descr) + } + } + + // notNilArg reports an error if v is provably nil. + notNilArg := func(stack []fact, instr ssa.Instruction, v ssa.Value, descr string) { + if nilnessOf(stack, v) == isnil { + reportf("nilarg", instr.Pos(), "nil argument to "+descr) + } + } + + // visit visits reachable blocks of the CFG in dominance order, + // maintaining a stack of dominating nilness facts. + // + // By traversing the dom tree, we can pop facts off the stack as + // soon as we've visited a subtree. Had we traversed the CFG, + // we would need to retain the set of facts for each block. + seen := make([]bool, len(fn.Blocks)) // seen[i] means visit should ignore block i + var visit func(b *ssa.BasicBlock, stack []fact) + visit = func(b *ssa.BasicBlock, stack []fact) { + if seen[b.Index] { + return + } + seen[b.Index] = true + + // Report nil dereferences. + for _, instr := range b.Instrs { + switch instr := instr.(type) { + case ssa.CallInstruction: + call := instr.Common() + notNil(stack, instr, call.Value, call.Description()) + + switch f := call.Value.(type) { + case *ssa.Function: + if tf, ok := f.Object().(*types.Func); ok { + if extract, ok := a.argExtractors[tf.FullName()]; ok { + notNilArg(stack, instr, extract(call), tf.FullName()) + } + } + } + case *ssa.FieldAddr: + notNil(stack, instr, instr.X, "field selection") + case *ssa.IndexAddr: + notNil(stack, instr, instr.X, "index operation") + case *ssa.MapUpdate: + notNil(stack, instr, instr.Map, "map update") + case *ssa.Slice: + // A nilcheck occurs in ptr[:] iff ptr is a pointer to an array. + if _, ok := instr.X.Type().Underlying().(*types.Pointer); ok { + notNil(stack, instr, instr.X, "slice operation") + } + case *ssa.Store: + notNil(stack, instr, instr.Addr, "store") + case *ssa.TypeAssert: + if !instr.CommaOk { + notNil(stack, instr, instr.X, "type assertion") + } + case *ssa.UnOp: + if instr.Op == token.MUL { // *X + notNil(stack, instr, instr.X, "load") + } + } + } + + // Look for panics with nil value + for _, instr := range b.Instrs { + switch instr := instr.(type) { + case *ssa.Panic: + if nilnessOf(stack, instr.X) == isnil { + reportf("nilpanic", instr.Pos(), "panic with nil value") + } + } + } + + // For nil comparison blocks, report an error if the condition + // is degenerate, and push a nilness fact on the stack when + // visiting its true and false successor blocks. + if binop, tsucc, fsucc := eq(b); binop != nil { + xnil := nilnessOf(stack, binop.X) + ynil := nilnessOf(stack, binop.Y) + + if ynil != unknown && xnil != unknown && (xnil == isnil || ynil == isnil) { + // Degenerate condition: + // the nilness of both operands is known, + // and at least one of them is nil. + if a.reportDegenerateIfConditions { + var adj string + if (xnil == ynil) == (binop.Op == token.EQL) { + adj = "tautological" + } else { + adj = "impossible" + } + reportf("cond", binop.Pos(), "%s condition: %s %s %s", adj, xnil, binop.Op, ynil) + } + + // If tsucc's or fsucc's sole incoming edge is impossible, + // it is unreachable. Prune traversal of it and + // all the blocks it dominates. + // (We could be more precise with full dataflow + // analysis of control-flow joins.) + var skip *ssa.BasicBlock + if xnil == ynil { + skip = fsucc + } else { + skip = tsucc + } + for _, d := range b.Dominees() { + if d == skip && len(d.Preds) == 1 { + continue + } + visit(d, stack) + } + return + } + + // "if x == nil" or "if nil == y" condition; x, y are unknown. + if xnil == isnil || ynil == isnil { + var newFacts facts + if xnil == isnil { + // x is nil, y is unknown: + // t successor learns y is nil. + newFacts = expandFacts(fact{binop.Y, isnil}) + } else { + // x is nil, y is unknown: + // t successor learns x is nil. + newFacts = expandFacts(fact{binop.X, isnil}) + } + + for _, d := range b.Dominees() { + // Successor blocks learn a fact + // only at non-critical edges. + // (We could do be more precise with full dataflow + // analysis of control-flow joins.) + s := stack + if len(d.Preds) == 1 { + if d == tsucc { + s = append(s, newFacts...) + } else if d == fsucc { + s = append(s, newFacts.negate()...) + } + } + visit(d, s) + } + return + } + } + + for _, d := range b.Dominees() { + visit(d, stack) + } + } + + // Visit the entry block. No need to visit fn.Recover. + if fn.Blocks != nil { + visit(fn.Blocks[0], make([]fact, 0, 20)) // 20 is plenty + } +} + +// A fact records that a block is dominated +// by the condition v == nil or v != nil. +type fact struct { + value ssa.Value + nilness nilness +} + +func (f fact) negate() fact { return fact{f.value, -f.nilness} } + +type nilness int + +const ( + isnonnil = -1 + unknown nilness = 0 + isnil = 1 +) + +var nilnessStrings = []string{"non-nil", "unknown", "nil"} + +func (n nilness) String() string { return nilnessStrings[n+1] } + +// nilnessOf reports whether v is definitely nil, definitely not nil, +// or unknown given the dominating stack of facts. +func nilnessOf(stack []fact, v ssa.Value) nilness { + switch v := v.(type) { + // unwrap ChangeInterface values recursively, to detect if underlying + // values have any facts recorded or are otherwise known with regard to nilness. + // + // This work must be in addition to expanding facts about + // ChangeInterfaces during inference/fact gathering because this covers + // cases where the nilness of a value is intrinsic, rather than based + // on inferred facts, such as a zero value interface variable. That + // said, this work alone would only inform us when facts are about + // underlying values, rather than outer values, when the analysis is + // transitive in both directions. + case *ssa.ChangeInterface: + if underlying := nilnessOf(stack, v.X); underlying != unknown { + return underlying + } + } + + // Is value intrinsically nil or non-nil? + switch v := v.(type) { + case *ssa.Alloc, + *ssa.FieldAddr, + *ssa.FreeVar, + *ssa.Function, + *ssa.Global, + *ssa.IndexAddr, + *ssa.MakeChan, + *ssa.MakeClosure, + *ssa.MakeInterface, + *ssa.MakeMap, + *ssa.MakeSlice: + return isnonnil + case *ssa.Const: + if v.IsNil() { + return isnil + } + return isnonnil + } + + // Search dominating control-flow facts. + for _, f := range stack { + if f.value == v { + return f.nilness + } + } + return unknown +} + +// If b ends with an equality comparison, eq returns the operation and +// its true (equal) and false (not equal) successors. +func eq(b *ssa.BasicBlock) (op *ssa.BinOp, tsucc, fsucc *ssa.BasicBlock) { + if If, ok := b.Instrs[len(b.Instrs)-1].(*ssa.If); ok { + if binop, ok := If.Cond.(*ssa.BinOp); ok { + switch binop.Op { + case token.EQL: + return binop, b.Succs[0], b.Succs[1] + case token.NEQ: + return binop, b.Succs[1], b.Succs[0] + } + } + } + return nil, nil, nil +} + +// expandFacts takes a single fact and returns the set of facts that can be +// known about it or any of its related values. Some operations, like +// ChangeInterface, have transitive nilness, such that if you know the +// underlying value is nil, you also know the value itself is nil, and vice +// versa. This operation allows callers to match on any of the related values +// in analyzes, rather than just the one form of the value that happened to +// appear in a comparison. +// +// This work must be in addition to unwrapping values within nilnessOf because +// while this work helps give facts about transitively known values based on +// inferred facts, the recursive check within nilnessOf covers cases where +// nilness facts are intrinsic to the underlying value, such as a zero value +// interface variables. +// +// ChangeInterface is the only expansion currently supported, but others, like +// Slice, could be added. At this time, this tool does not check slice +// operations in a way this expansion could help. See +// https://play.golang.org/p/mGqXEp7w4fR for an example. +func expandFacts(f fact) []fact { + ff := []fact{f} + +Loop: + for { + switch v := f.value.(type) { + case *ssa.ChangeInterface: + f = fact{v.X, f.nilness} + ff = append(ff, f) + default: + break Loop + } + } + + return ff +} + +type facts []fact + +func (ff facts) negate() facts { + nn := make([]fact, len(ff)) + for i, f := range ff { + nn[i] = f.negate() + } + return nn +} + +var firstArg = func(c *ssa.CallCommon) ssa.Value { return c.Args[0] } diff --git a/pkg/testutils/lint/passes/nilness/nilness_test.go b/pkg/testutils/lint/passes/nilness/nilness_test.go new file mode 100644 index 000000000000..761063a3d7a1 --- /dev/null +++ b/pkg/testutils/lint/passes/nilness/nilness_test.go @@ -0,0 +1,30 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Portions of this file are additionally subject to the following +// license and copyright. +// +// Copyright 2021 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 nilness_test + +import ( + "testing" + + "github.com/cockroachdb/cockroach/pkg/testutils/lint/passes/nilness" + "golang.org/x/tools/go/analysis/analysistest" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, nilness.Analyzer, "a") +} diff --git a/pkg/testutils/lint/passes/nilness/testdata/src/a/a.go b/pkg/testutils/lint/passes/nilness/testdata/src/a/a.go new file mode 100644 index 000000000000..2af1336b0ccb --- /dev/null +++ b/pkg/testutils/lint/passes/nilness/testdata/src/a/a.go @@ -0,0 +1,177 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Portions of this file are additionally subject to the following +// license and copyright. +// +// Copyright 2021 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 + +type X struct{ f, g int } + +func f(x, y *X) { + if x == nil { + print(x.f) // want "nil dereference in field selection" + } else { + print(x.f) + } + + if x == nil { + if nil != y { + print(1) + panic(0) + } + x.f = 1 // want "nil dereference in field selection" + y.f = 1 // want "nil dereference in field selection" + } + + var f func() + if f == nil { // want "tautological condition: nil == nil" + go f() // want "nil dereference in dynamic function call" + } else { + // This block is unreachable, + // so we don't report an error for the + // nil dereference in the call. + defer f() + } +} + +func f2(ptr *[3]int, i interface{}) { + if ptr != nil { + print(ptr[:]) + *ptr = [3]int{} + print(*ptr) + } else { + print(ptr[:]) // want "nil dereference in slice operation" + *ptr = [3]int{} // want "nil dereference in store" + print(*ptr) // want "nil dereference in load" + + if ptr != nil { // want "impossible condition: nil != nil" + // Dominated by ptr==nil and ptr!=nil, + // this block is unreachable. + // We do not report errors within it. + print(*ptr) + } + } + + if i != nil { + print(i.(interface{ f() })) + } else { + print(i.(interface{ f() })) // want "nil dereference in type assertion" + } +} + +func g() error + +func f3() error { + err := g() + if err != nil { + return err + } + if err != nil && err.Error() == "foo" { // want "impossible condition: nil != nil" + print(0) + } + ch := make(chan int) + if ch == nil { // want "impossible condition: non-nil == nil" + print(0) + } + if ch != nil { // want "tautological condition: non-nil != nil" + print(0) + } + return nil +} + +func h(err error, b bool) { + if err != nil && b { + return + } else if err != nil { + panic(err) + } +} + +func i(*int) error { + for { + if err := g(); err != nil { + return err + } + } +} + +func f4(x *X) { + if x == nil { + panic(x) + } +} + +func f5(x *X) { + panic(nil) // want "panic with nil value" +} + +func f6(x *X) { + var err error + panic(err) // want "panic with nil value" +} + +func f7() { + x, err := bad() + if err != nil { + panic(0) + } + if x == nil { + panic(err) // want "panic with nil value" + } +} + +func bad() (*X, error) { + return nil, nil +} + +func f8() { + var e error + v, _ := e.(interface{}) + print(v) +} + +func f9(x interface { + a() + b() + c() +}) { + + x.b() // we don't catch this panic because we don't have any facts yet + xx := interface { + a() + b() + }(x) + if xx != nil { + return + } + x.c() // want "nil dereference in dynamic method call" + xx.b() // want "nil dereference in dynamic method call" + xxx := interface{ a() }(xx) + xxx.a() // want "nil dereference in dynamic method call" + + if unknown() { + panic(x) // want "panic with nil value" + } + if unknown() { + panic(xx) // want "panic with nil value" + } + if unknown() { + panic(xxx) // want "panic with nil value" + } +} + +func unknown() bool { + return false +} diff --git a/pkg/util/encoding/encoding.go b/pkg/util/encoding/encoding.go index e3b6c60bbaaa..1840db456d71 100644 --- a/pkg/util/encoding/encoding.go +++ b/pkg/util/encoding/encoding.go @@ -1196,9 +1196,6 @@ func EncodeGeoDescending(b []byte, curveIndex uint64, so *geopb.SpatialObject) ( } n := len(b) b = encodeBytesAscendingWithTerminator(b, data, ascendingGeoEscapes.escapedTerm) - if err != nil { - return nil, err - } onesComplement(b[n:]) return b, nil } diff --git a/pkg/util/json/encoded.go b/pkg/util/json/encoded.go index 5b65cce21eae..e9f316bb784a 100644 --- a/pkg/util/json/encoded.go +++ b/pkg/util/json/encoded.go @@ -428,18 +428,18 @@ func (j *jsonEncoded) FetchValKey(key string) (JSON, error) { // or maybe there's something fancier we could do if we know the locations // of the offsets by strategically positioning our binary search guesses to // land on them. - var err error + var searchErr error i := sort.Search(j.containerLen, func(idx int) bool { data, _, err := j.objectGetNthDataRange(idx) if err != nil { + searchErr = err return false } return string(data) >= key }) - if err != nil { - return nil, err + if searchErr != nil { + return nil, searchErr } - // The sort.Search API implies that we have to double-check if the key we // landed on is the one we were searching for in the first place. if i >= j.containerLen { diff --git a/vendor b/vendor index 23f0fdc8662c..239554abe5b7 160000 --- a/vendor +++ b/vendor @@ -1 +1 @@ -Subproject commit 23f0fdc8662c5dc3fbb3a5c73bb7408bd71a7be6 +Subproject commit 239554abe5b72fedd92c2b3c6e37d512288efa8b