Skip to content

Commit

Permalink
checkers: fix sqlQuery panic
Browse files Browse the repository at this point in the history
If astcast to SelectorExpr failed, funcExpr.Sel will be nil.

Bug reported by @un000

Also move "_" lhs check up, since it's cheaper.

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>
  • Loading branch information
quasilyte committed May 26, 2020
1 parent 33d100b commit 35e1b83
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 6 deletions.
15 changes: 9 additions & 6 deletions checkers/sqlQuery_checker.go
Expand Up @@ -36,19 +36,19 @@ func (c *sqlQueryChecker) VisitStmt(stmt ast.Stmt) {
return
}

call := astcast.ToCallExpr(assign.Rhs[0])
funcExpr := astcast.ToSelectorExpr(call.Fun)
if !c.funcIsQuery(funcExpr) {
return
}

// If Query() is called, but first return value is ignored,
// there is no way to close/read the returned rows.
// This can cause a connection leak.
if id, ok := assign.Lhs[0].(*ast.Ident); ok && id.Name != "_" {
return
}

call := astcast.ToCallExpr(assign.Rhs[0])
funcExpr := astcast.ToSelectorExpr(call.Fun)
if !c.funcIsQuery(funcExpr) {
return
}

if c.typeHasExecMethod(c.ctx.TypesInfo.TypeOf(funcExpr.X)) {
c.warnAndSuggestExec(funcExpr)
} else {
Expand All @@ -57,6 +57,9 @@ func (c *sqlQueryChecker) VisitStmt(stmt ast.Stmt) {
}

func (c *sqlQueryChecker) funcIsQuery(funcExpr *ast.SelectorExpr) bool {
if funcExpr.Sel == nil {
return false
}
switch funcExpr.Sel.Name {
case "Query", "QueryContext":
// Stdlib and friends.
Expand Down
6 changes: 6 additions & 0 deletions checkers/testdata/sqlQuery/negative_tests.go
Expand Up @@ -4,6 +4,12 @@ import (
"database/sql"
)

func foo() (int, int) { return 0, 0 }

func notQueryCall() {
_, _ = foo()
}

func queryResultIsUsed(db *sql.DB, qe QueryExecer, mydb *myDatabase) {
const queryString = "SELECT * FROM users"

Expand Down

0 comments on commit 35e1b83

Please sign in to comment.