Merge pull request #681 from owen-mc/new-query/wrapped-error-always-nil

Add query "Wrapped error always nil"

Author: owen-mc

ql/lib/change-notes/2022-02-10-wrapped-error-always-nil.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `go/unexpected-nil-value`, to finds calls to `Wrap` from `pkg/errors` where the error argument is always nil.

ql/src/InconsistentCode/WrappedErrorAlwaysNil.go

@@ -0,0 +1,30 @@
+package main
+
+import (
+	"github.com/pkg/errors"
+)
+
+func f1(input string) error {
+	if input == "1" {
+		return errors.Errorf("error in f1")
+	}
+	return nil
+}
+
+func f2(input string) (bool, error) {
+	if input == "2" {
+		return false, errors.Errorf("error in f2")
+	}
+	return true, nil
+}
+
+func test1(input string) error {
+	err := f1(input)
+	if err != nil {
+		return errors.Wrap(err, "input is the first non-negative integer")
+	}
+	if ok2, _ := f2(input); !ok2 {
+		return errors.Wrap(err, "input is the second non-negative integer")
+	}
+	return nil
+}

ql/src/InconsistentCode/WrappedErrorAlwaysNil.qhelp

@@ -0,0 +1,54 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+  The <code>pkg.errors</code> package provides the <code>errors.Wrap</code>
+  function for annotating an error with a stack trace. When passed
+  <code>nil</code>, this function returns <code>nil</code>.
+
+  When the first parameter to <code>errors.Wrap</code> is <em>always</em>
+  <code>nil</code>, the function call has no effect and likely indicates a
+  programming mistake.
+
+  A common example of this is when an <code>errors.Wrap(err, "message")</code>
+  call is copied from an earlier error-handling block in the same function and
+  used in a subsequent error-handling block that does not check
+  <code>err</code> in its guard. In this case the return of a <code>nil</code>
+  value to the caller indicates by convention that the operation succeeded, and
+  so the failure is masked.
+</p>
+</overview>
+
+<recommendation>
+<p>
+  Usually an <code>err</code> value is being referenced outside its intended
+  scope. The problem can be fixed by removing that reference, for example by
+  changing a call of the form <code>errors.Wrap(err, "message")</code> to
+  <code>errors.New("message")</code>.
+</p>
+</recommendation>
+
+<example>
+<p>
+  The example below shows an example where the <code>err</code> value returned
+  from the call to <code>f1</code> is reused in a later call, when it is known
+  to be <code>nil</code>:
+</p>
+
+<sample src="WrappedErrorAlwaysNil.go" />
+
+<p>
+  One way of fixing this is to create a new error value with
+  <code>errors.New</code>:
+</p>
+
+<sample src="WrappedErrorAlwaysNilGood.go" />
+</example>
+
+<references>
+<li>Go errors, github.com/pkg/errors: <a href="https://pkg.go.dev/github.com/pkg/errors#Wrap">errors.Wrap</a></li>
+</references>
+</qhelp>

ql/src/InconsistentCode/WrappedErrorAlwaysNil.ql

@@ -0,0 +1,68 @@
+/**
+ * @name Wrapped error is always nil
+ * @description Finds calls to `Wrap` from `pkg/errors` where the error argument is always nil.
+ * @kind problem
+ * @problem.severity warning
+ * @id go/unexpected-nil-value
+ * @tags reliability
+ *       correctness
+ *       logic
+ * @precision high
+ */
+
+import go
+
+/** Gets package for `github.com/pkg/errors`. */
+string packagePath() { result = package("github.com/pkg/errors", "") }
+
+/**
+ * An equality test which guarantees that an expression is always `nil`.
+ */
+class NilTestGuard extends DataFlow::BarrierGuard, DataFlow::EqualityTestNode {
+  DataFlow::Node otherNode;
+
+  NilTestGuard() {
+    this.getAnOperand() = Builtin::nil().getARead() and
+    otherNode = this.getAnOperand() and
+    not otherNode = Builtin::nil().getARead()
+  }
+
+  override predicate checks(Expr e, boolean outcome) {
+    e = otherNode.asExpr() and
+    outcome = this.getPolarity()
+  }
+}
+
+/** Gets a use of a local variable that has the value `nil`. */
+DataFlow::ExprNode getNilFromLocalVariable() {
+  exists(SsaVariable ssa, Write w |
+    w.definesSsaVariable(ssa, Builtin::nil().getARead()) and
+    result.asInstruction() = ssa.getAUse()
+  )
+}
+
+from DataFlow::Node n
+where
+  n = any(Function f | f.hasQualifiedName(packagePath(), "Wrap")).getACall().getArgument(0) and
+  (
+    // ```go
+    // errors.Wrap(nil, "")
+    // ```
+    n = Builtin::nil().getARead()
+    or
+    // ```go
+    // var localVar error = nil
+    // errors.Wrap(localVar, "")
+    // ```
+    n = getNilFromLocalVariable()
+    or
+    // ```go
+    // if err != nil {
+    // 	return ...
+    // }
+    // if ok2, _ := f2(input); !ok2 {
+    // 	return errors.Wrap(err, "")
+    // }
+    n = any(NilTestGuard ntg).getAGuardedNode()
+  )
+select n, "The first argument to 'errors.Wrap' is always nil"

ql/src/InconsistentCode/WrappedErrorAlwaysNilGood.go

@@ -0,0 +1,30 @@
+package main
+
+import (
+	"github.com/pkg/errors"
+)
+
+func f1(input string) error {
+	if input == "1" {
+		return errors.Errorf("error in f1")
+	}
+	return nil
+}
+
+func f2(input string) (bool, error) {
+	if input == "2" {
+		return false, errors.Errorf("error in f2")
+	}
+	return true, nil
+}
+
+func test1(input string) error {
+	err := f1(input)
+	if err != nil {
+		return errors.Wrap(err, "input is the first non-negative integer")
+	}
+	if ok2, _ := f2(input); !ok2 {
+		return errors.New("input is the second non-negative integer")
+	}
+	return nil
+}

ql/test/query-tests/InconsistentCode/WrappedErrorAlwaysNil/WrappedErrorAlwaysNil.expected

@@ -0,0 +1,4 @@
+| WrappedErrorAlwaysNil.go:31:22:31:24 | err | The first argument to 'errors.Wrap' is always nil |
+| WrappedErrorAlwaysNil.go:41:14:41:16 | nil | The first argument to 'errors.Wrap' is always nil |
+| WrappedErrorAlwaysNil.go:45:14:45:16 | err | The first argument to 'errors.Wrap' is always nil |
+| WrappedErrorAlwaysNil.go:49:14:49:21 | localErr | The first argument to 'errors.Wrap' is always nil |

ql/test/query-tests/InconsistentCode/WrappedErrorAlwaysNil/WrappedErrorAlwaysNil.go

@@ -0,0 +1,50 @@
+package main
+
+//go:generate depstubber -vendor github.com/pkg/errors "" Errorf,Wrap
+
+import (
+	"github.com/pkg/errors"
+)
+
+func f1(input string) error {
+	if input == "1" {
+		return errors.Errorf("error in f1")
+	}
+	return nil
+}
+
+func f2(input string) (bool, error) {
+	if input == "2" {
+		return false, errors.Errorf("error in f2")
+	}
+	return true, nil
+}
+
+func test1(input string) error {
+	err := f1(input)
+	if err != nil {
+		// GOOD: Wrapped error is always non-nil
+		return errors.Wrap(err, "")
+	}
+	if ok2, _ := f2(input); !ok2 {
+		// BAD: Wrapped error is always nil
+		return errors.Wrap(err, "")
+	}
+	return nil
+}
+
+func test2(err error) {
+	// GOOD: Wrapped error is not always nil
+	errors.Wrap(err, "")
+
+	// BAD: Wrapped error is always nil
+	errors.Wrap(nil, "")
+
+	err = nil
+	// BAD: Wrapped error is always nil
+	errors.Wrap(err, "")
+
+	var localErr error = nil
+	// BAD: Wrapped error is always nil
+	errors.Wrap(localErr, "")
+}

ql/test/query-tests/InconsistentCode/WrappedErrorAlwaysNil/WrappedErrorAlwaysNil.qlref

@@ -0,0 +1 @@
+InconsistentCode/WrappedErrorAlwaysNil.ql

ql/test/query-tests/InconsistentCode/WrappedErrorAlwaysNil/go.mod

@@ -0,0 +1,5 @@
+module main
+
+go 1.14
+
+require github.com/pkg/errors v0.9.1