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

chore: enable go-critic deferInLoop lint #2825

Merged
merged 1 commit into from May 1, 2024

Conversation

willmurphyscode
Copy link
Contributor

The goal here is to be more careful about leaking file handles and other resources.

Issue #2819 alerted us that we are not closing file handles aggressively enough.

This lint is not on by default, because it is still tagged as experimental, see https://go-critic.com/overview.html#deferinloop

Manual testing:

Introduce the following diff locally (note defer reader.Close() in the loop):

git diff -W | head -n 20
diff --git a/syft/pkg/cataloger/binary/elf_package_cataloger.go b/syft/pkg/cataloger/binary/elf_package_cataloger.go
index 94f65640..7a384fb5 100644
--- a/syft/pkg/cataloger/binary/elf_package_cataloger.go
+++ b/syft/pkg/cataloger/binary/elf_package_cataloger.go
@@ -51,56 +51,57 @@ func (c *elfPackageCataloger) Name() string {
 func (c *elfPackageCataloger) Catalog(_ context.Context, resolver file.Resolver) ([]pkg.Package, []artifact.Relationship, error) {
        locations, err := resolver.FilesByMIMEType(mimetype.ExecutableMIMETypeSet.List()...)
        if err != nil {
                return nil, nil, fmt.Errorf("unable to get binary files by mime type: %w", err)
        }
 
        // first find all ELF binaries that have notes
        var notesByLocation = make(map[elfPackageKey][]elfBinaryPackageNotes)
        for _, location := range locations {
                reader, err := resolver.FileContentsByLocation(location)
                if err != nil {
                        return nil, nil, fmt.Errorf("unable to get binary contents %q: %w", location.Path(), err)
                }
+               defer reader.Close()

On main, make lint with this diff exits 0.

On this branch, make lint fails like this:

make lint                                             
task: [lint] .tool/golangci-lint run --tests=false
syft/pkg/cataloger/binary/elf_package_cataloger.go:64:3: deferInLoop: Possible resource leak, 'defer' is called in the 'for' loop (gocritic)
                defer reader.Close()
                ^
task: Failed to run task "lint": exit status 1
make: *** [lint] Error 201

Also, the following command was used to confirm that enabling additional lints for gocritic appends to the set of enabled lints, rather than overwriting it (note that this is the set of gocritic lints, not the whole set of golangci-lint lints):

$ GL_DEBUG=gocritic ./.tool/golangci-lint run --enable=gocritic 1>/dev/null 2>&1 1>&2 | grep "Final used checks"
level=debug msg="[gocritic] Final used checks (35): [appendAssign argOrder assignOp badCall 
badCond captLocal caseOrder codegenComment commentFormatting defaultCaseOrder 
deferInLoop <<< this one is new 
deprecatedComment dupArg dupBranchBody dupCase dupSubExpr elseif exitAfterDefer flagDeref 
flagName ifElseChain mapKey newDeref offBy1 regexpMust singleCaseSwitch sloppyLen 
sloppyTypeAssert switchTrue typeSwitchVar underef unlambda unslice valSwap wrapperFunc]"

The goal here is to be more careful about leaking file handles and other
resources.

Signed-off-by: Will Murphy <will.murphy@anchore.com>
@willmurphyscode willmurphyscode merged commit ed40833 into main May 1, 2024
11 checks passed
@willmurphyscode willmurphyscode deleted the chore-enable-defer-in-loop-gocritic branch May 1, 2024 16:59
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