From f4a24b035375edb153503a69a1165330c4d2b64c Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 26 Nov 2019 16:19:17 +0000 Subject: [PATCH 1/3] Improve type information for tuple elements. We would previously rely on the type information of the target variable into which the element is stored, but that could be a more general type. For example, in the assignment ```go x, y := f() ``` the type of `x` might be an interface while the type of `f()[0]` is a concrete type implementing that interface. --- ql/src/semmle/go/controlflow/IR.qll | 70 +++++++++++++++++++---------- 1 file changed, 46 insertions(+), 24 deletions(-) diff --git a/ql/src/semmle/go/controlflow/IR.qll b/ql/src/semmle/go/controlflow/IR.qll index a8d498fb016..7de15bb3b1d 100644 --- a/ql/src/semmle/go/controlflow/IR.qll +++ b/ql/src/semmle/go/controlflow/IR.qll @@ -552,42 +552,64 @@ module IR { ExtractTupleElementInstruction() { this = MkExtractNode(s, i) } - /** Holds if this extracts the `idx`th value of the result of `base`. */ - predicate extractsElement(Instruction base, int idx) { + Instruction getBase() { exists(Expr baseExpr | baseExpr = s.(Assignment).getRhs() or baseExpr = s.(ValueSpec).getInit() | - base = evalExprInstruction(baseExpr) and - idx = i + result = evalExprInstruction(baseExpr) ) or - base = MkNextNode(s) and - idx = i + result = MkNextNode(s) or - base = evalExprInstruction(s.(ReturnStmt).getExpr()) and - idx = i + result = evalExprInstruction(s.(ReturnStmt).getExpr()) or - base = evalExprInstruction(s.(CallExpr).getArgument(0)) and - idx = i + result = evalExprInstruction(s.(CallExpr).getArgument(0).stripParens()) + } + + /** Holds if this extracts the `idx`th value of the result of `base`. */ + predicate extractsElement(Instruction base, int idx) { + base = getBase() and idx = i } override Type getResultType() { - result = s.(Assignment).getLhs(i).getType() + exists(CallExpr c | getBase() = evalExprInstruction(c) | + result = c.getTarget().getResultType(i) + ) or - result = s.(ValueSpec).getNameExpr(i).getType() - or - i = 0 and - result = s.(RangeStmt).getKey().getType() - or - i = 1 and - result = s.(RangeStmt).getValue().getType() - or - result = s.(ReturnStmt).getEnclosingFunction().getType().getResultType(i) - or - exists(CallExpr inner | - inner = s.(CallExpr).getArgument(0).stripParens() and - result = inner.getTarget().getResultType(i) + exists(Type rangeType | rangeType = s.(RangeStmt).getDomain().getType().getUnderlyingType() | + exists(Type baseType | + baseType = rangeType.(ArrayType).getElementType() or + baseType = rangeType.(PointerType).getBaseType().getUnderlyingType().(ArrayType).getElementType() or + baseType = rangeType.(SliceType).getElementType() | + i = 0 and + result instanceof IntType + or + i = 1 and + result = baseType + ) + or + rangeType instanceof StringType and + ( + i = 0 and + result instanceof IntType + or + result = Builtin::rune().getType() + ) + or + exists(MapType map | map = rangeType | + i = 0 and + result = map.getKeyType() + or + i = 1 and + result = map.getValueType() + ) + or + i = 0 and + result = rangeType.(RecvChanType).getElementType() + or + i = 0 and + result = rangeType.(SendRecvChanType).getElementType() ) } From e5a12e97381c8170bb0b377c5f934dab4243f198 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 26 Nov 2019 16:22:32 +0000 Subject: [PATCH 2/3] Add new query ImpossibleInterfaceNilCheck. --- change-notes/1.24/analysis-go.md | 1 + .../ImpossibleInterfaceNilCheck.go | 16 ++++++ .../ImpossibleInterfaceNilCheck.qhelp | 48 ++++++++++++++++++ .../ImpossibleInterfaceNilCheck.ql | 49 +++++++++++++++++++ .../ImpossibleInterfaceNilCheckGood.go | 12 +++++ .../ImpossibleInterfaceNilCheck.expected | 2 + .../ImpossibleInterfaceNilCheck.go | 14 ++++++ .../ImpossibleInterfaceNilCheck.qlref | 1 + .../ImpossibleInterfaceNilCheckGood.go | 12 +++++ .../ImpossibleInterfaceNilCheck/lib.go | 11 +++++ .../ImpossibleInterfaceNilCheck/tst.go | 27 ++++++++++ 11 files changed, 193 insertions(+) create mode 100644 ql/src/RedundantCode/ImpossibleInterfaceNilCheck.go create mode 100644 ql/src/RedundantCode/ImpossibleInterfaceNilCheck.qhelp create mode 100644 ql/src/RedundantCode/ImpossibleInterfaceNilCheck.ql create mode 100644 ql/src/RedundantCode/ImpossibleInterfaceNilCheckGood.go create mode 100644 ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.expected create mode 100644 ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.go create mode 100644 ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.qlref create mode 100644 ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheckGood.go create mode 100644 ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/lib.go create mode 100644 ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/tst.go diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index 1e9b8c40bd0..5a8c9570cdb 100644 --- a/change-notes/1.24/analysis-go.md +++ b/change-notes/1.24/analysis-go.md @@ -5,6 +5,7 @@ | **Query** | **Tags** | **Purpose** | |---------------------------------------------------------------------------|----------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------| | Constant length comparison (`go/constant-length-comparison`) | correctness | Highlights code that checks the length of an array or slice against a constant before indexing it using a variable, suggesting a logic error. Results are shown on LGTM by default. | +| Impossible interface nil check (`go/impossible-interface-nil-check`) | correctness | Highlights code that compares an interface value that cannot be `nil` to `nil`, suggesting a logic error. Results are shown on LGTM by default. | ## Changes to existing queries diff --git a/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.go b/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.go new file mode 100644 index 00000000000..04ed671a6fc --- /dev/null +++ b/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.go @@ -0,0 +1,16 @@ +package main + +import "fmt" + +func fetch(url string) (string, *RequestError) + +func niceFetch(url string) { + var s string + var e error + s, e = fetch(url) + if e != nil { + fmt.Printf("Unable to fetch URL: %v\n", e) + } else { + fmt.Printf("URL contents: %s\n", s) + } +} diff --git a/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.qhelp b/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.qhelp new file mode 100644 index 00000000000..7a08f4ed14a --- /dev/null +++ b/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.qhelp @@ -0,0 +1,48 @@ + + + + +

+Interface values in Go are type tagged, that is, they are essentially pairs of the form +(value, type), where value is a non-interface value with the given +type. Such a pair is never nil, even if value is +nil. +

+

+In particular, if a non-interface value v is assigned to a variable x +whose type is an interface, then x will never be nil, regardless of +v. Comparing x to nil is pointless, and may indicate +a misunderstanding of Go interface values or some other underlying bug. +

+ + + +

+Carefully inspect the comparison to ensure it is not a symptom of a bug. +

+ + + +

+The following example shows a declaration of a function fetch which fetches the +contents of a URL, returning either the contents or an error value, which is a pointer to a custom +error type RequestError (not shown). The function niceFetch wraps this +function, printing out either the URL contents or an error message. +

+ +

+However, because e is declared to be of type error, which is an +interface, the nil check will never succeed, since e can never be +nil. +

+

+In this case, the problem can be solved by using a short variable declaration using operator +:=, which will automatically infer the more precise non-interface type +*ResourceError for e, making the nil check behave as +expected. +

+ + + diff --git a/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.ql b/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.ql new file mode 100644 index 00000000000..0f6153c34cf --- /dev/null +++ b/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.ql @@ -0,0 +1,49 @@ +/** + * @name Impossible interface nil check + * @description Comparing a non-nil interface value to nil may indicate a logic error. + * @kind problem + * @problem.severity warning + * @id go/impossible-interface-nil-check + * @tags correctness + * @precision high + */ + +import go + +/** + * Holds if `eq` compares interface value `nd` to `nil`. + */ +predicate interfaceNilCheck(DataFlow::EqualityTestNode eq, DataFlow::Node nd) { + nd = eq.getAnOperand() and + nd.getType().getUnderlyingType() instanceof InterfaceType and + eq.getAnOperand().getType() instanceof NilLiteralType +} + +/** + * Holds if the result of `nd` may flow to an interface `nil` check. + */ +predicate flowsToInterfaceNilCheck(DataFlow::Node nd) { + interfaceNilCheck(_, nd) or + flowsToInterfaceNilCheck(nd.getASuccessor()) +} + +/** + * Holds if `nd` is a non-nil interface value. + */ +predicate nonNilWrapper(DataFlow::Node nd) { + flowsToInterfaceNilCheck(nd) and + forex(DataFlow::Node pred | pred = nd.getAPredecessor() | + exists(Type predtp | predtp = pred.getType().getUnderlyingType() | + not predtp instanceof InterfaceType and + not predtp instanceof NilLiteralType + ) + or + nonNilWrapper(pred) + ) +} + +from DataFlow::EqualityTestNode eq, DataFlow::Node nd +where + interfaceNilCheck(eq, nd) and + nonNilWrapper(nd) +select nd, "This value can never be nil, since it is a wrapped interface value." diff --git a/ql/src/RedundantCode/ImpossibleInterfaceNilCheckGood.go b/ql/src/RedundantCode/ImpossibleInterfaceNilCheckGood.go new file mode 100644 index 00000000000..f0b74568e6b --- /dev/null +++ b/ql/src/RedundantCode/ImpossibleInterfaceNilCheckGood.go @@ -0,0 +1,12 @@ +package main + +import "fmt" + +func niceFetchGood(url string) { + s, e := fetch(url) + if e != nil { + fmt.Printf("Unable to fetch URL: %v\n", e) + } else { + fmt.Printf("URL contents: %s\n", s) + } +} diff --git a/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.expected b/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.expected new file mode 100644 index 00000000000..e1294f702e2 --- /dev/null +++ b/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.expected @@ -0,0 +1,2 @@ +| ImpossibleInterfaceNilCheck.go:9:5:9:5 | e | This value can never be nil, since it is a wrapped interface value. | +| tst.go:10:14:10:14 | y | This value can never be nil, since it is a wrapped interface value. | diff --git a/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.go b/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.go new file mode 100644 index 00000000000..00b015d3814 --- /dev/null +++ b/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.go @@ -0,0 +1,14 @@ +package main + +import "fmt" + +func niceFetch(url string) { + var s string + var e error + s, e = fetch(url) + if e != nil { + fmt.Printf("Unable to fetch URL: %v\n", e) + } else { + fmt.Printf("URL contents: %s\n", s) + } +} diff --git a/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.qlref b/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.qlref new file mode 100644 index 00000000000..d858724be57 --- /dev/null +++ b/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheck.qlref @@ -0,0 +1 @@ +RedundantCode/ImpossibleInterfaceNilCheck.ql diff --git a/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheckGood.go b/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheckGood.go new file mode 100644 index 00000000000..f0b74568e6b --- /dev/null +++ b/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/ImpossibleInterfaceNilCheckGood.go @@ -0,0 +1,12 @@ +package main + +import "fmt" + +func niceFetchGood(url string) { + s, e := fetch(url) + if e != nil { + fmt.Printf("Unable to fetch URL: %v\n", e) + } else { + fmt.Printf("URL contents: %s\n", s) + } +} diff --git a/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/lib.go b/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/lib.go new file mode 100644 index 00000000000..e38679df10b --- /dev/null +++ b/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/lib.go @@ -0,0 +1,11 @@ +package main + +type RequestError string + +func (e RequestError) Error() string { + return string(e) +} + +func fetch(url string) (string, *RequestError) { + return "stuff", nil +} diff --git a/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/tst.go b/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/tst.go new file mode 100644 index 00000000000..81584045c13 --- /dev/null +++ b/ql/test/query-tests/RedundantCode/ImpossibleInterfaceNilCheck/tst.go @@ -0,0 +1,27 @@ +package main + +import "fmt" + +func test1() { + var x *int = nil + var y interface{} = x + fmt.Println(x == nil) + fmt.Println(x == y) + fmt.Println(y == nil) // NOT OK +} + +func test2() { + var x *int = nil + test3(x) +} + +func test3(y interface{}) { + // we don't want to flag this one, even though we might be able to + // inter-procedurally establish that y cannot be nil + fmt.Println(y == nil) // OK +} + +func test4() { + var y interface{} + fmt.Println(y == nil) // OK +} From ba54cde86e623e4b3c8741206c2db4095ce09d92 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 27 Nov 2019 10:47:42 +0000 Subject: [PATCH 3/3] Add two references. --- ql/src/RedundantCode/ImpossibleInterfaceNilCheck.qhelp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.qhelp b/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.qhelp index 7a08f4ed14a..b4305e99298 100644 --- a/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.qhelp +++ b/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.qhelp @@ -45,4 +45,9 @@ expected.

+ + +
  • Jordan Orelli: How to use interfaces in Go.
    • +
  • Go Frequently Asked Questions: Why is my nil error value not equal to nil?.
    • +