Skip to content

G307: Unsafe defer call of a method returning an error for defer file.Close() ? #512

Closed
@koddr

Description

@koddr

Summary

After update to latest Golang & gosec version, I see this security error:

[...] - G307 (CWE-703): Deferring unsafe method "Close" on type "*os.File" (Confidence: HIGH, Severity: MEDIUM)
    49:         }
  > 50:         defer file.Close()
    51: 

Summary:
   Files: 6
   Lines: 231
   Nosec: 0
   Issues: 1

My function never changes and looks like:

// ...

// NewConfig returns a new decoded Config struct
func NewConfig(configPath string) (*Config, error) {
	// Validate config path
	if err := ValidateConfigPath(configPath); err != nil {
		return nil, err
	}

	// Create config structure
	config := &Config{}

	// Open config file
	file, err := os.Open(filepath.Clean(configPath))
	if err != nil {
		return nil, err
	}
	defer file.Close() // <-- error G307 (CWE-703) on this line

	// Init new YAML decode
	d := yaml.NewDecoder(file)

	// Start YAML decoding from file
	if err := d.Decode(&config); err != nil {
		return nil, err
	}

	return config, nil
}

// ...

Steps to reproduce the behavior

  1. Create function with open/close file
  2. Run gosec

gosec version

$ gosec

VERSION: 2.4.0
GIT TAG: v2.4.0
BUILD DATE: 2020-07-24T07:54:54Z

Go version (output of 'go version')

go version go1.15 linux/amd64

Operating system / Environment

$ uname -a

Linux vic-linux-pc 5.8.0-2-MANJARO #1 SMP PREEMPT Sat Aug 8 17:55:27 UTC 2020 x86_64 GNU/Linux

Expected behavior

No errors, or solve this error.

Actual behavior

CI (GitHub Actions) send warnings and skip my code to master branch (but this code wasn't changed and works fine at lower version).

Activity

ccojocar

ccojocar commented on Aug 17, 2020

@ccojocar
Member

This is because https://golang.org/pkg/os/#File.Close returns and error which is not checked when calling defer. This was recently introduced to catch this kind of situation which could lead to a crash.

We should maybe rethink this rule since is a common pattern which might generate more headaches than catching security issues.

koddr

koddr commented on Aug 18, 2020

@koddr
Author

@ccojocar hi! Thanks for reply.

I think, better way to make it works, is skip defer ... lines from security checking. At this time, I (and other developers with the same problem) will still add a gosec comment to reset this line, so why not do it right away?

Because I have no idea, how to covering error with this language construction.

ccojocar

ccojocar commented on Aug 18, 2020

@ccojocar
Member

This is a solution.

You can use this code snippet to avoid the warning:

defer func() {
    if err := file.Close(); err != nil { 
         logger.Printf("Error closing file: %s\n", err)
    }
}()
added a commit that references this issue on Aug 18, 2020
koddr

koddr commented on Aug 18, 2020

@koddr
Author

Thanks, that works! 👍

24 remaining items

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @ccojocar@koddr

        Issue actions

          G307: Unsafe defer call of a method returning an error for `defer file.Close()` ? · Issue #512 · securego/gosec