Rule which detects aliasing of values in RangeStmt #443
Conversation
Codecov Report
@@ Coverage Diff @@
## master #443 +/- ##
=======================================
Coverage 71.42% 71.42%
=======================================
Files 9 9
Lines 651 651
=======================================
Hits 465 465
Misses 164 164
Partials 22 22 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for this contribution! This is good work but I would rework a bit the rule to avoid traversing multiple times the tree. You can use a cache and reference back the already parsed elements. See the IntegerOverflowCheck rule. I am looking forward to seeing an improved implementation such that we can merge it. Thanks!
rules/implicit_aliasing.go
Outdated
|
|
||
| var issue *gosec.Issue | ||
| var fun func(n ast.Node) bool | ||
| fun = func(n ast.Node) bool { |
There was a problem hiding this comment.
I think that you can compact these two lines by just:
fun := func(n ast.Node) bool {
}
rules/implicit_aliasing.go
Outdated
| continue | ||
| } | ||
|
|
||
| ast.Inspect(element, fun) |
There was a problem hiding this comment.
We try to avoid Inspect in the rules, because will effectively traverse again the tree which is not very efficient. Maybe have a look at this rule which uses a cache to traverse only once the tree
gosec/rules/integer_overflow.go
Line 33 in a305f10
|
Totally makes sense -- I'm looking into refactoring this to be less big O. |
|
@ccojocar , is there a way to tell when GoSec leaves an AST node? The issue I'm having is the following: for _, item := range []string{"A", "B", "C"} {
foo(item) // This is safe.
}
item := "D"
bar(&item) // Also safe but problematic.For ease of discussion, let "bad identifier" be an identifier yielded by a range statement which should not be referenced (found as the expression in a unary op with the '&' operator). I'm trying to collect all bad identifiers and verify they are not referened within the body of the range loop. With the func (r *implicitAliasing) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) {
if n == nil {
r.pop()
return nil, nil
}
r.push([]string{})
switch node := n.(type) {
case *ast.RangeStmt:
if node.Value != nil && node.Tok.String() == ":=" {
if ident, ok := node.Value.(*ast.Ident); ok {
r.add(ident.Name)
}
}
case *ast.UnaryExpr:
if ident, ok := node.X.(*ast.Ident); ok && node.Op.String() == "&" {
if r.has(ident.Name) {
return gosec.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil
}
}
return nil, nil
}From what I can tell, GoSec doesn't have a way for a rule to visit all the An alternative would be to carry a list of the bad IDs and invalidate them when they are rebound (filter on assignment statements) but this seems like a less desirable solution since the implementation (based on the names last use) in no way represents the problem it is attempting to solve (scope). I'd also feel less confident that this would catch all the ways that a name could be recycled and it would also be strange that this is invalid Go code but theoretically could get flagged: for _, item := range []string{"A", "B", "C"} {
foo(item) // This is safe.
}
bar(&item) // Not valid.I doubt it makes sense to start passing all the |
|
@caccavale I believe we can catch the issue inside of the package main
import "fmt"
var vector []*string
func appendVector(s *string) {
vector = append(vector, s)
}
func printVector() {
for _, item := range vector {
fmt.Printf("%s", *item)
}
fmt.Println()
}
func main() {
for _, item := range []string{"A", "B", "C"} {
fmt.Printf("%p\n", &item)
appendVector(&item)
}
printVector()
}I run We can have something like this:
I think we can optimise this a bit by storing also the position of the On the first From that point on, we don't check anymore |
|
@caccavale Do you need any additional help to be able to move forward with this rule improvement? Thanks |
|
Sorry for the delay! Hows it look? Thanks for the detailed explanation/walkthrough of the AST. I didn't realize the strength in the |
fixes #438