Consider allowing more linters

[mtrmac]

https://github.com/containers/storage/pull/1522 has disabled 5 out of the 7 default golangci-lint linters. That seems excessive.

For the record, currently:

% golangci-lint version              
golangci-lint has version 1.52.2 built with go1.20.2 from da04413a on 2023-03-25T18:11:28Z
% golangci-lint run --no-config ./...
pkg/chunked/compressor/compressor.go:331:21: SA1019: hdr.Xattrs is deprecated: Use PAXRecords instead. (staticcheck)
		for k, v := range hdr.Xattrs {
		                  ^
check.go:289:14: Error return value of `io.Copy` is not checked (errcheck)
						io.Copy(io.Discard, diffReader)
						       ^
check.go:883:54: SA1019: tar.TypeRegA has been deprecated since Go 1.11 and an alternative has been available since Go 1.1: Use TypeReg instead. (staticcheck)
		if hdr.Typeflag == tar.TypeLink || hdr.Typeflag == tar.TypeRegA {
		                                                   ^
check.go:902:45: SA1019: tar.TypeRegA has been deprecated since Go 1.11 and an alternative has been available since Go 1.1: Use TypeReg instead. (staticcheck)
	if typeflag == tar.TypeLink || typeflag == tar.TypeRegA {
	                                           ^
lockfile_compat.go:8:15: SA1019: lockfile.Locker is deprecated: Refer directly to *LockFile, the provided implementation, instead. (staticcheck)
type Locker = lockfile.Locker //lint:ignore SA1019 // lockfile.Locker is deprecated
              ^
pkg/regexp/regexp.go:26:9: copylocks: return copies lock value: github.com/containers/storage/pkg/regexp.Regexp contains sync.Once contains sync.Mutex (govet)
	return re
	       ^
pkg/ioutils/fswriters.go:153:18: Error return value of `w.closeTempFile` is not checked (errcheck)
		w.closeTempFile()
		               ^
pkg/ioutils/readers_test.go:82:7: lostcancel: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak (govet)
	ctx, _ := context.WithTimeout(context.Background(), 100*time.Millisecond)
	     ^
pkg/archive/archive.go:402:5: SA1019: hdr.Xattrs has been deprecated since Go 1.10: Use PAXRecords instead. (staticcheck)
	if hdr.Xattrs == nil {
	   ^
pkg/archive/archive.go:403:3: SA1019: hdr.Xattrs has been deprecated since Go 1.10: Use PAXRecords instead. (staticcheck)
		hdr.Xattrs = make(map[string]string)
		^
pkg/archive/archive.go:411:4: SA1019: hdr.Xattrs has been deprecated since Go 1.10: Use PAXRecords instead. (staticcheck)
			hdr.Xattrs[xattr] = string(capability)
			^
pkg/archive/archive.go:646:20: SA1019: tar.TypeRegA has been deprecated since Go 1.11 and an alternative has been available since Go 1.1: Use TypeReg instead. (staticcheck)
	case tar.TypeReg, tar.TypeRegA:
	                  ^
drivers/chown.go:63:12: SA1019: pwalk.Walk is deprecated: use [github.com/opencontainers/selinux/pkg/pwalkdir.Walk] (staticcheck)
	if err := pwalk.Walk(".", chown); err != nil {
	          ^
drivers/vfs/driver.go:186:21: Error return value of `label.SetFileLabel` is not checked (errcheck)
		label.SetFileLabel(dir, mountLabel)
		                  ^
types/utils.go:210:25: Error return value is not checked (errcheck)
	ReloadConfigurationFile(configFile, storeOptions)

At least the “error return”, “return copies lock value”, “the cancel function returned by context.WithTimeout should be called” parts seem relevant and valuable.

[mtrmac]

I’m sorry, that attribution is incorrect; those linters have been disabled for years, and I don’t now care to track what was the default at which time.

My primary point is that we are now very much diverging from the current default, which seems unintuitive — linter developers probably don’t care about utility / comprehensive coverage of our chosen set of linters, and per the examples above, quite likely to be suboptimal for this project.

[rhatdan]

What are the list of linters, you believe we should enable?

[mtrmac]

I have no expertise in that at all; that’s partly why this is an issue and not a PR.

At this point my recommendation would be to either just switch to the default set (if we don’t have any opinion at all), or to at least enable the default set (because it’s likely enough to be useful, and if we want to be more strict, sure, why not).

[rhatdan]

I agree, I think we need to start turning on more of the linters and starting with the default set would be great. Getting to the point that podman, buildah, common, image and storage supported the same set should be the goal.

[mtrmac]

This was changed in #1615 , but the configuration still disables 4/7 default linters.

[mtrmac]

After @Honny1’s work I think we can now enable errcheck, and then all default linters would be enabled; this issue would now be resolved.

[Honny1]

I will create PR.

[Honny1]

@mtrmac I have created PR #2011, which enables the errcheck linter. I also have some suggestions for other liners that could expand the default list of liners. Some of the liners will need non-trivial fixes but would be beneficial.

Here's a list of linters that I think would be nice to have:

• stylecheck Stylecheck is a replacement for golint.
• unparam Reports unused function parameters.
• gosec Inspects source code for security problems.

These linters are stricter, but they would be beneficial:

• dupl Tool for code clone detection.
• gocyclo Computes and checks the cyclomatic complexity of functions.
• gocognit Computes and checks the cognitive complexity of functions.
• maintidx Maintidx measures the maintainability index of each function.

Which ones would be a good idea, created an issue for allowing?

Cc: @rhatdan

[rhatdan]

I am fine with adding these linters.

@giuseppe @mtrmac @nalind @saschagrunert WDYT?

[mtrmac]

@mtrmac I have created PR #2011, which enables the errcheck linter.

Thanks! As far as I care, just using all of the default is good enough to close the issue, but this is also a good place to concentrate discussion about non-default ones.

Most importantly, I’m not a c/storage maintainer and this is not my decision. Dedicated maintainers should choose what works best for the project.

---

• stylecheck Stylecheck is a replacement for golint.

• Reports capitalization failures (ST1003) which are all false positives
• ST1005: error strings should not be capitalized, fine

This one is attractive to me in the sense that VS Code seems to include it by default, so having it enforced would decrease noise.

• unparam Reports unused function parameters. … and return values.

I’m ambivalent: It does find useful opportunities for simplification, OTOH sometimes having a meaningful abstraction is better than satisfying this, especially in tests (“numberOfFiles always receives 10”).

• gosec Inspects source code for security problems.

It does find some bad test code at least, but otherwise the false-positive rate seems pretty abysmal. I’m tempted to be flippant and to say that if this were “solved” by uncommented unscoped //nolint comments, those nolint comments might be more dangerous for the code than the lines it is warning about.

These linters are stricter, but they would be beneficial:

• dupl Tool for code clone detection.

Hum. It does find large chunks of similar code, but it’s not always obvious how to structure it in a less redundant way (e.g. the startReading code); every abstraction has also a cost. (My rule of thumb is to build an abstraction when creating a third copy.)

• gocyclo Computes and checks the cyclomatic complexity of functions.
• gocognit Computes and checks the cognitive complexity of functions.
• maintidx Maintidx measures the maintainability index of each function.

This code base does have a bit of a tendency to have functions which should be separate files… OTOH I’ve seen PR contributions proposing to enable this then try to satisfy such linters by chopping functions in half in completely nonsensical places; and people “trying to get things done” just slap a //nolint on top of the function. Attractive in theory, negative value in practice, IMHO.

[Honny1]

• stylecheck Stylecheck is a replacement for golint.

• Reports capitalization failures (ST1003) which are all false positives
• ST1005: error strings should not be capitalized, fine

This one is attractive to me in the sense that VS Code seems to include it by default, so having it enforced would decrease noise.

We can configure linter to ignore some specific rules that we found false positives.

• unparam Reports unused function parameters. … and return values.

I’m ambivalent: It does find useful opportunities for simplification, OTOH sometimes having a meaningful abstraction is better than satisfying this, especially in tests (“numberOfFiles always receives 10”).

• gosec Inspects source code for security problems.

It does find some bad test code at least, but otherwise the false-positive rate seems pretty abysmal. I’m tempted to be flippant and to say that if this were “solved” by uncommented unscoped //nolint comments, those nolint comments might be more dangerous for the code than the lines it is warning about.

I agree with you. I think excluding tests from linting would be a good idea.

These linters are stricter, but they would be beneficial:

• dupl Tool for code clone detection.

Hum. It does find large chunks of similar code, but it’s not always obvious how to structure it in a less redundant way (e.g. the startReading code); every abstraction has also a cost. (My rule of thumb is to build an abstraction when creating a third copy.)

We can set the threshold to 200.

• gocyclo Computes and checks the cyclomatic complexity of functions.
• gocognit Computes and checks the cognitive complexity of functions.
• maintidx Maintidx measures the maintainability index of each function.

This code base does have a bit of a tendency to have functions which should be separate files… OTOH I’ve seen PR contributions proposing to enable this then try to satisfy such linters by chopping functions in half in completely nonsensical places; and people “trying to get things done” just slap a //nolint on top of the function. Attractive in theory, negative value in practice, IMHO.

I agree with you, but I would say this can force the reviewer of PR, but it will add time to merging PR.

[Honny1]

I have tried to configure linters. Config:

---
run:
  concurrency: 6
  timeout: 5m
  tests: false
linters:
  enable:
    - gofumpt
    - dupl
    - stylecheck
    - gosec
    - unparam
linters-settings:
  dupl:
    threshold: 200
  stylecheck:
    checks: ["all", "-ST1003"]

Results:

pkg/stringutils/stringutils.go:16:18: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
		b[i] = letters[rand.Intn(len(letters))]
		               ^
pkg/stringutils/stringutils.go:28:18: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
		res[i] = chars[rand.Intn(len(chars))]
		               ^
pkg/reexec/command_linux.go:22:9: G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
	cmd := exec.Command(Self())
	       ^
pkg/reexec/command_linux.go:32:9: G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
	cmd := exec.CommandContext(ctx, Self())
	       ^
pkg/stringid/stringid.go:99:8: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
	rng = rand.New(rand.NewSource(seed))
	      ^
pkg/chunked/dump/dump.go:262:55: G601: Implicit memory aliasing in for loop. (gosec)
			if err := dumpNode(w, added, links, verityDigests, &e); err != nil {
			                                                   ^
pkg/idtools/utils_unix.go:31:13: G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
	execCmd := exec.Command(cmd, strings.Split(args, " ")...)
	           ^
pkg/archive/archive.go:700:17: G305: File traversal when extracting zip/tar archive (gosec)
		targetPath := filepath.Join(extractDir, hdr.Linkname)
		              ^
pkg/archive/archive.go:712:17: G305: File traversal when extracting zip/tar archive (gosec)
		targetPath := filepath.Join(filepath.Dir(path), hdr.Linkname)
		              ^
pkg/archive/archive.go:1069:11: G305: File traversal when extracting zip/tar archive (gosec)
		path := filepath.Join(dest, hdr.Name)
		        ^
pkg/archive/archive.go:1527:17: G110: Potential DoS vulnerability via decompression bomb (gosec)
				if _, err = io.Copy(hasher, t); err != nil && err != io.EOF {
				            ^
pkg/archive/copy.go:342:16: G110: Potential DoS vulnerability via decompression bomb (gosec)
			if _, err = io.Copy(rebasedTar, srcTar); err != nil {
			            ^
pkg/chrootarchive/archive.go:131:17: G110: Potential DoS vulnerability via decompression bomb (gosec)
				if _, err = io.Copy(hasher, t); err != nil && err != io.EOF {
				            ^
drivers/graphtest/graphtest_unix.go:434:9: G306: Expect WriteFile permissions to be 0600 or less (gosec)
	return os.WriteFile(path, data, 0o700)
	       ^
drivers/graphtest/testutil.go:48:12: G306: Expect WriteFile permissions to be 0600 or less (gosec)
	if err := os.WriteFile(path.Join(root, "file-a"), randomContent(64, seed), 0o755); err != nil {
	          ^
drivers/graphtest/testutil.go:54:12: G306: Expect WriteFile permissions to be 0600 or less (gosec)
	if err := os.WriteFile(path.Join(root, "dir-b", "file-b"), randomContent(128, seed+1), 0o755); err != nil {
	          ^
pkg/chunked/storage_linux.go:772:79: G601: Implicit memory aliasing in for loop. (gosec)
			compression, err := c.prepareCompressedStreamToFile(partCompression, part, &mf)
			                                                                           ^
cmd/containers-storage/main.go:91:30: G601: Implicit memory aliasing in for loop. (gosec)
					command.addFlags(flags, &command)
					                        ^
types/utils.go:15:59: expandEnvPath - result 1 (error) is always nil (unparam)
func expandEnvPath(path string, rootlessUID int) (string, error) {
                                                          ^
drivers/overlay/overlay.go:690:35: `supportsOverlay` - `homeMagic` is unused (unparam)
func supportsOverlay(home string, homeMagic graphdriver.FsMagic, rootUID, rootGID int) (supportsDType bool, err error) {
                                  ^
userns.go:24:60: getAdditionalSubIDs - result 2 (error) is always nil (unparam)
func getAdditionalSubIDs(username string) (*idSet, *idSet, error) {
                                                           ^
pkg/chunked/cache_linux.go:379:60: appendTag - result 1 (error) is always nil (unparam)
func appendTag(digest []byte, offset, len uint64) ([]byte, error) {
                                                           ^
pkg/idtools/idtools.go:318:6: func `parseSubidFile` is unused (unused)
func parseSubidFile(path, username string) (ranges, error) {
     ^
check.go:1078:26: func `(*checkDirectory).names` is unused (unused)
func (c *checkDirectory) names() []string {
                         ^
idset.go:52:17: func `(*idSet).union` is unused (unused)
func (s *idSet) union(t *idSet) *idSet {

I think some of them are interesting. WDYT: @giuseppe @mtrmac @nalind @saschagrunert

[mtrmac]

The gosec false positive rate is just depressing — it’s possible there’s something there but I honestly gave up after seeing nonsense in most categories. The other two linters, sure, worth fixing.

[Honny1]

The gosec false positive rate is just depressing — it’s possible there’s something there but I honestly gave up after seeing nonsense in most categories.

@mtrmac You're right about gosec. I checked the rules that gosec checks and it looks like it's more for web applications. And it wouldn't help much.

The other two linters, sure, worth fixing.

Okay, I'll create issues for them and then we can close this issue.

[Honny1]

@mtrmac I have created issues for the other linters, can we close this issue?