serum-errors / go-serum-analyzer

Other
7 stars 2 forks source link

Panic: Found a way to reach an "unreachable" path. #2

Closed TripleDogDare closed 1 year ago

TripleDogDare commented 1 year ago

Reproduction: https://github.com/serum-errors/go-demo-app-with-serum/pull/1

 ~/repos/misc/go-demo-app-with-serum (induce-panic) $ go-serum-analyzer -strict ./...
panic: should be unreachable: destructirung assignment should only originate from a call expression.

goroutine 765 [running]:
github.com/serum-errors/go-serum-analyzer/analysis.findErrorCodesFromIdentTaint(0xc00bdcd140, 0xc0000ece78?, 0x40a996?, 0xc00bdbd140)
    /home/cjb/repos/warpforge/go-serum-analyzer/analysis/analyse.go:784 +0x5bd
github.com/serum-errors/go-serum-analyzer/analysis.findErrorCodesInExpression(0xc0000ecfc8?, 0x4e094e?, {0x78a720?, 0xc005be4340?}, 0xc005be4360?)
    /home/cjb/repos/warpforge/go-serum-analyzer/analysis/analyse.go:589 +0x190
github.com/serum-errors/go-serum-analyzer/analysis.findErrorCodesInReturnStmt(0x0?, 0xc005be4360?, 0xc0000ecfa8?, 0x48d7be?)
    /home/cjb/repos/warpforge/go-serum-analyzer/analysis/analyse.go:570 +0x93
github.com/serum-errors/go-serum-analyzer/analysis.findErrorCodesInFunctionReturnStmts.func1({0x789a18?, 0xc005be4360})
    /home/cjb/repos/warpforge/go-serum-analyzer/analysis/analyse.go:508 +0x16f
go/ast.inspector.Visit(0xc00bdcd560, {0x789a18?, 0xc005be4360?})
    /opt/go/go1.18.3/go/src/go/ast/walk.go:387 +0x31
go/ast.Walk({0x788ca0?, 0xc00bdcd560?}, {0x789a18?, 0xc005be4360?})
    /opt/go/go1.18.3/go/src/go/ast/walk.go:52 +0x62
go/ast.walkStmtList({0x788ca0, 0xc00bdcd560}, {0xc005bcd610?, 0x1, 0xc0001239e0?})
    /opt/go/go1.18.3/go/src/go/ast/walk.go:32 +0x91
go/ast.Walk({0x788ca0?, 0xc00bdcd560?}, {0x7893d8?, 0xc0057ed8c0?})
    /opt/go/go1.18.3/go/src/go/ast/walk.go:235 +0x10e5
go/ast.Walk({0x788ca0?, 0xc00bdcd560?}, {0x789838?, 0xc001fea200?})
    /opt/go/go1.18.3/go/src/go/ast/walk.go:242 +0x122a
go/ast.walkStmtList({0x788ca0, 0xc00bdcd560}, {0xc005be4440?, 0x2, 0x403609?})
    /opt/go/go1.18.3/go/src/go/ast/walk.go:32 +0x91
go/ast.Walk({0x788ca0?, 0xc00bdcd560?}, {0x7893d8?, 0xc0057ed920?})
    /opt/go/go1.18.3/go/src/go/ast/walk.go:235 +0x10e5
go/ast.Walk({0x788ca0?, 0xc00bdcd560?}, {0x789838?, 0xc001fea280?})
    /opt/go/go1.18.3/go/src/go/ast/walk.go:242 +0x122a
go/ast.walkStmtList({0x788ca0, 0xc00bdcd560}, {0xc005be44a0?, 0x2, 0xc00bdcd560?})
    /opt/go/go1.18.3/go/src/go/ast/walk.go:32 +0x91
go/ast.Walk({0x788ca0?, 0xc00bdcd560?}, {0x7893d8?, 0xc0057ed950?})
    /opt/go/go1.18.3/go/src/go/ast/walk.go:235 +0x10e5
go/ast.Inspect(...)
    /opt/go/go1.18.3/go/src/go/ast/walk.go:399
github.com/serum-errors/go-serum-analyzer/analysis.findErrorCodesInFunctionReturnStmts(0xc00bdcd140?, 0xc00bdcd4a0?, 0xc00bdbd140?)
    /home/cjb/repos/warpforge/go-serum-analyzer/analysis/analyse.go:497 +0x125
github.com/serum-errors/go-serum-analyzer/analysis.findErrorCodesInFunc(0xc00bdcd140, 0xc00bdbd140)
    /home/cjb/repos/warpforge/go-serum-analyzer/analysis/analyse.go:476 +0x21f
github.com/serum-errors/go-serum-analyzer/analysis.runVerify(0xc0086f0410)
    /home/cjb/repos/warpforge/go-serum-analyzer/analysis/analyse.go:149 +0x305
golang.org/x/tools/go/analysis/internal/checker.(*action).execOnce(0xc005c63540)
    /home/cjb/gopath/pkg/mod/golang.org/x/tools@v0.1.5/go/analysis/internal/checker/checker.go:691 +0x9fe
sync.(*Once).doSlow(0x6720e9?, 0xc0001381c0?)
    /opt/go/go1.18.3/go/src/sync/once.go:68 +0xc2
sync.(*Once).Do(...)
    /opt/go/go1.18.3/go/src/sync/once.go:59
golang.org/x/tools/go/analysis/internal/checker.(*action).exec(0xc0059800f0?)
    /home/cjb/gopath/pkg/mod/golang.org/x/tools@v0.1.5/go/analysis/internal/checker/checker.go:579 +0x3d
golang.org/x/tools/go/analysis/internal/checker.execAll.func1(0x0?)
    /home/cjb/gopath/pkg/mod/golang.org/x/tools@v0.1.5/go/analysis/internal/checker/checker.go:567 +0x25
created by golang.org/x/tools/go/analysis/internal/checker.execAll
    /home/cjb/gopath/pkg/mod/golang.org/x/tools@v0.1.5/go/analysis/internal/checker/checker.go:573 +0x165

Serum analyzer built from current master branch

$ git log -1 --format=oneline
d28fd4b26d2969f1f1fd829f4cbb45ea8d3fc686 (HEAD -> master, origin/master, origin/HEAD) does it blend?
warpfork commented 1 year ago

Definitely a missing feature.

The code that needs work is indeed right where the stack trace points -- around analyse.go:784 plus or minus about 10 lines.

I think the syntax that will trigger this panic is something like x, y := z, w, or x, y = z, w. The code we have right now only supports x, y := f(...)

The line that's attempting to cast .(*ast.CallExpr) should be converted to a switch statement, I think, and also handle ... at least *ast.AssignStmt. That might be the only additional thing needed (but I haven't written the code yet and am not 100% sure).

TripleDogDare commented 1 year ago

I have run across complaints about assign statements a few times.

TripleDogDare commented 1 year ago

AFAICT, the go1.18+ fix #5 resolves the panic. However, the feature is still unimplemented and returns a panic. I'm going to close this issue but will keep working on the PR.