-
-
Notifications
You must be signed in to change notification settings - Fork 652
Closed
Labels
Description
Summary
Elements yielded from RangeStmt's have the same memory address. Not realizing this and storing these addresses for later use is an unsafe behavior.
Steps to reproduce the behavior
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 foo() (int, *string, string) {
for _, item := range vector {
return 0, &item, item
}
}
func main() {
for _, item := range []string{"A", "B", "C"} {
appendVector(&item)
}
printVector()
zero, c_star, c := foo()
fmt.Printf("%d %v %s", zero, c_start, c)
}
Expected behavior
foo's behavior shouldn't cause any issues, but appendVector(&item)
will.
I propose adding the following rule:
func (r *implicitAliasing) Match(n ast.Node, c *gosec.Context) (*gosec.Issue, error) {
if rangeStmt, ok := n.(*ast.RangeStmt); ok && rangeStmt.Value != nil && rangeStmt.Tok.String() == ":=" {
if ident, ok := rangeStmt.Value.(*ast.Ident); ok {
name := ident.Name
var issue *gosec.Issue
var fun func(n ast.Node) bool
fun = func(n ast.Node) bool {
if ret, ok := n.(*ast.ReturnStmt); ok {
for _, element := range ret.Results {
if isDereferenceOf(element, name) {
continue
}
ast.Inspect(element, fun)
if issue != nil {
return false
}
}
return false
}
if isDereferenceOf(n, name) {
issue = gosec.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence)
return false
}
return issue == nil
}
ast.Inspect(rangeStmt.Body, fun)
return issue, nil
}
}
return nil, nil
}
It does have false positives but seems like it is a good stab of what can be a nasty bug. I haven't participated in much open source software so please advise on how I should go about this; I can open a PR with the work I have done if that is desired. Thanks.