Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rules/sdk: G705 should allow map copying #24

Closed
kirbyquerby opened this issue Jun 9, 2022 · 1 comment
Closed

rules/sdk: G705 should allow map copying #24

kirbyquerby opened this issue Jun 9, 2022 · 1 comment

Comments

@kirbyquerby
Copy link
Collaborator

Summary

Copying a map is a safe operation and shouldn't be flagged by the static analyzer. We should allow code like:

for k, v := range m1 {
	m2[k] = v
}

and probably:

for k := range m1 {
	m2[k] = m1[v]
}

Steps to reproduce the behavior

make test

gosec version

Go version (output of 'go version')

N/A

Operating system / Environment

N/A

Expected behavior

Don't raise a warning for copying a map.

Actual behavior

[/home/nathan/Documents/gosec/analyzer.go:359-361] - G705 (CWE-): the value in the range statement should be nil: want: for key := range m (Confidence: MEDIUM, Severity: HIGH)
    358:        if len(gosec.context.Ignores) > 0 {
  > 359:                for k, v := range gosec.context.Ignores[0] {
  > 360:                        ignores[k] = v
  > 361:                }
    362:        }
@kirbyquerby kirbyquerby mentioned this issue Jun 9, 2022
Merged
odeke-em pushed a commit that referenced this issue Jun 9, 2022
@kirbyquerby
rules/sdk: use ctx.Info.TypeOf() to get types (#25)
f6f1167 
This lets us determine types much more reliably without having to worry about adding more cases to switch statements and pulling out the debugger when we panic on a new AST structure we haven't handled yet. It's always nice to half the number of lines in a file as well :)

And of course this change causes the rule to notice more map statements that it previously mixed, so I've also fixed those.

I also discovered a new case that this rule incorrectly flags -- map copying is safe to do directly. I've filed #24 and suppressed the rule for the map copy in analyzer.go.

With this change, I'm able to run gosec on cosmos/cosmos-sdk without it crashing.

Updates cosmos/cosmos-sdk#10572
@kirbyquerby kirbyquerby mentioned this issue Jun 22, 2022
Merged
odeke-em pushed a commit that referenced this issue Jun 23, 2022
@kirbyquerby
rules/sdk: permit map copying in G705 (#28)
6d93b60 
This change permits map copies of the form:
```go
for k, v := range from {
    to[k] = v
}
```

It doesn't currently permit the alternative, which trickier to check for and doesn't really have any benefits:
```go
for k := range from {
  to[k] = from[k]
}
```


Updates #24
@kirbyquerby kirbyquerby mentioned this issue Jul 1, 2022
Merged
@kirbyquerby
Copy link
Collaborator Author

This is fixed as of #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@kirbyquerby