gosec: G307: gosec starts detecting G307 (CWE-703) even with proposed way to safely handle errors

@danieljmt new version released https://github.com/securego/gosec/releases/tag/v2.9.3

@lootek It is stated in the docs. In fact the docs were specifically updated because of this pattern. See: https://github.com/golang/go/issues/32427

// Close closes the File, rendering it unusable for I/O.
// On files that support SetDeadline, any pending I/O operations will
// be canceled and return immediately with an error.
// Close will return an error if it has already been called.
func (f *File) Close() error {

This is how I typically open and close files. It gets flagged even though I thought it was idiomatic. However I am new to Go, so I’m open to being told this is poor coding style.

        f, err := os.Create("foo.txt")
        if err != nil {
                return fmt.Errorf("bad foo: %w", err)
        }
        defer f.Close()

        _, err = f.Write([]byte("foo"))
        if err != nil {
                return fmt.Errorf("bad foo: %w", err)
        }

        err = f.Close()
        if err != nil {
                return fmt.Errorf("bad foo: %w", err)
        }

The defer f.Close() is used to close the file even if there is a panic before directly calling f.Close(). It is apparently safe to call Close() twice. If the code is already panicking then the close error is ignored.

More details:

$ go version
go version go1.17 darwin/amd64

$ go install github.com/securego/gosec/v2/cmd/gosec@latest
go: downloading github.com/securego/gosec v0.0.0-20200401082031-e946c8c39989
go: downloading github.com/securego/gosec/v2 v2.9.1
go: downloading github.com/gookit/color v1.4.2
go: downloading gopkg.in/yaml.v2 v2.4.0
go: downloading golang.org/x/tools v0.1.7
go: downloading github.com/google/uuid v1.3.0
go: downloading github.com/nbutton23/zxcvbn-go v0.0.0-20210217022336-fa2cb2858354
go: downloading github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778

Example source:

package main

import (
        "fmt"
        "os"
)

func main() {
        err := foo()
        fmt.Println(err)
}

func foo() error {
        f, err := os.Create("foo.txt")
        if err != nil {
                return fmt.Errorf("bad foo: %w", err)
        }
        defer f.Close()

        _, err = f.Write([]byte("foo"))
        if err != nil {
                return fmt.Errorf("bad foo: %w", err)
        }

        err = f.Close()
        if err != nil {
                return fmt.Errorf("bad foo: %w", err)
        }

        return nil
}

Result:

[/Users/greg/go/src/foo/main.go:18] - G307 (CWE-703): Deferring unsafe method "Close" on type "*os.File" (Confidence: HIGH, Severity: MEDIUM)
    17: 	}
  > 18: 	defer f.Close()
    19: 

@elgohr I merged your pull request.

Do you want to reopen the old PR?

You can reopen it if you want to rework it.

To my point of view a solution would be to revert the change and start again with the initial PR (including the failing false-positive-test)

I guess I already mentioned this https://github.com/securego/gosec/pull/661#issuecomment-889913968

This is not necessarily a false positive: https://www.joeshaw.org/dont-defer-close-on-writable-files/

Though gosec does not accept even the named parameters workaround so this issue here should be addressed anyway

We hit the same issue, we use the Github Action with the @master version. And I belive we can ignore this check first by add /* #nosec G307 */ (only in the situation below, you already check the error return by file close function)

// Github Action version of Gosec
 - name: Run Gosec Security Scanner
        uses: securego/gosec@master
        with:
          args: ./... -exclude=G301,G304,G107,G101,G110

The error

[/github/workspace/linkerd/install.go:274-278] - G307 (CWE-703): Deferring unsafe method "Close" on type "*os.File" (Confidence: HIGH, Severity: MEDIUM)
42
    273: 	}
43
  > 274: 	defer func() {
44
  > 275: 		if err := out.Close(); err != nil {
45
  > 276: 			fmt.Println(err)
46
  > 277: 		}
47
  > 278: 	}()
48
    279: 
49

I am seeing this as well. I would expect a defer func() to pass gosec without issue, because I am checking the error. This was working before, not sure what version.

G307 (CWE-703): Deferring unsafe method "Close" on type "*os.File" (Confidence: HIGH, Severity: MEDIUM)
    191: 			}
  > 192: 			defer func() {
  > 193: 				if err := f.Close(); err != nil {
  > 194: 					logger.Errorw("Failed to close file", zap.Error(err))
  > 195: 				}
  > 196: 			}()