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.

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

Fix some implicit memory aliasing in for loops #395

Merged
merged 9 commits into from
May 5, 2023

Conversation

jtraglia
Copy link
Contributor

@jtraglia jtraglia commented May 4, 2023

This PR fixes some implicit memory aliasing in for loops, which weren't being caught by golangci-lint (gosec).

It should probably be noted that golangci-lint does not run on generated files.

@jtraglia jtraglia marked this pull request as draft May 4, 2023 20:04
@gbotrel
Copy link
Collaborator

gbotrel commented May 4, 2023

It should probably be noted that golangci-lint does not run on generated files.

!!! really? 😮

@jtraglia
Copy link
Contributor Author

jtraglia commented May 4, 2023

Yes, I can't find a good document that states this, but here's the code:

// isGenerated reports whether the source file is generated code.
// Using a bit laxer rules than https://go.dev/s/generatedcode to
// match more generated code. See #48 and #72.
func isGeneratedFileByComment(doc string) bool {
	const (
		genCodeGenerated = "code generated"
		genDoNotEdit     = "do not edit"
		genAutoFile      = "autogenerated file" // easyjson
	)

If the file contains any of these strings, which the generated files do, it will skip it.

@jtraglia
Copy link
Contributor Author

jtraglia commented May 4, 2023

And sorry, I realized that I changed the generated files instead of the generator, and that there's probably more than three instances of this problem. It's going take a little while to do it right. Marking it as a draft in the mean time.

@jtraglia
Copy link
Contributor Author

jtraglia commented May 4, 2023

So, I believe this is fixed now. Alternatively, we could replace this:

for _, s := range testValues {
    s := s // prevent memory aliasing
    myFunc(&s)
}

with something like:

for i := range testValues {
    myFunc(&testValues[i])
}

I think I prefer the second option, but the first option requires fewer changes.

@jtraglia jtraglia marked this pull request as ready for review May 4, 2023 20:55
@gbotrel gbotrel merged commit 8fa7e7c into Consensys:develop May 5, 2023
6 checks passed
@jtraglia jtraglia deleted the implicit-memory-aliasing branch May 5, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants