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 +}