Skip to content

Conversation

@henry118
Copy link
Member

Fixes: #6584

gosec linter is able to identify issues described in #6584

e.g.

$ git revert 54e95e6b880912e489529be120d4ecead80dbd90
[gosec dfc8ca1ec] Revert "fix Implicit memory aliasing in for loop"
 2 files changed, 2 deletions(-)

$ make check
+ proto-fmt
+ check
GOGC=75 golangci-lint run
containerstore.go:192:54: G601: Implicit memory aliasing in for loop. (gosec)
		containers = append(containers, containerFromProto(&container))
		                                                   ^
image_store.go:132:42: G601: Implicit memory aliasing in for loop. (gosec)
		images = append(images, imageFromProto(&image))
		                                       ^
make: *** [check] Error 1

I also disabled following two settings which prevents the linter to show a complete list of issues.

  • max-issues-per-linter (default 50)
  • max-same-issues (default 3)

Furthermore enabling gosec reveals many other issues. For now I blacklisted the ones except G601. Maybe we can create tasks to address them one by one moving forward?

Signed-off-by: Henry Wang [email protected]

@k8s-ci-robot
Copy link

Hi @henry118. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 12, 2022

Build succeeded.

disable:
- errcheck

issues:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@henry118 henry118 Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I made suggested changes in the new version.

As to the excluded issues, they are not related to G601: Implicit memory aliasing in for loop.

Can we cut new tickets to address them separately one by one? as there are many of them. I am happy to take the ownership of it :)

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 14, 2022

Build succeeded.

@henry118 henry118 requested review from kzys March 14, 2022 20:00
Copy link
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Can you include the pull request description (or its excerpt) in the commit message itself?

https://github.com/containerd/project/blob/main/CONTRIBUTING.md#commit-messages

`gosec` linter is able to identify issues described in containerd#6584

e.g.

$ git revert 54e95e6
[gosec dfc8ca1ec] Revert "fix Implicit memory aliasing in for loop"
 2 files changed, 2 deletions(-)

$ make check
+ proto-fmt
+ check
GOGC=75 golangci-lint run
containerstore.go:192:54: G601: Implicit memory aliasing in for loop. (gosec)
		containers = append(containers, containerFromProto(&container))
		                                                   ^
image_store.go:132:42: G601: Implicit memory aliasing in for loop. (gosec)
		images = append(images, imageFromProto(&image))
		                                       ^
make: *** [check] Error 1

I also disabled following two settings which prevent the linter to show a complete list of issues.

* max-issues-per-linter (default 50)
* max-same-issues (default 3)

Furthermore enabling gosec revealed many other issues. For now I blacklisted the ones except G601.

Will create separate tasks to address them one by one moving next.

Signed-off-by: Henry Wang <[email protected]>
@henry118
Copy link
Member Author

Looks good to me. Can you include the pull request description (or its excerpt) in the commit message itself?

Updated

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 14, 2022

Build succeeded.

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable linters to catch issues like #6331

4 participants