Consider allowing more linters #1579

mtrmac · 2023-04-19T18:42:52Z

~~#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 · 2023-04-19T18:51:37Z

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 · 2023-04-20T23:56:22Z

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

mtrmac · 2023-04-21T21:24:05Z

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 · 2023-04-24T20:31:31Z

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 · 2023-05-25T16:14:22Z

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

mtrmac · 2024-07-10T17:59:02Z

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 · 2024-07-10T18:36:35Z

I will create PR.

Honny1 · 2024-07-11T11:48:34Z

@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 · 2024-07-11T12:34:43Z

I am fine with adding these linters.

@giuseppe @mtrmac @nalind @saschagrunert WDYT?

mtrmac · 2024-07-11T14:23:21Z

@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 · 2024-07-12T09:59:28Z

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 · 2024-07-12T10:20:04Z

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 · 2024-07-12T16:44:48Z

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 · 2024-07-15T09:09:32Z

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 · 2024-07-15T14:44:11Z

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

mtrmac mentioned this issue Sep 19, 2023

overlay, composefs: use data-only lower layers #1713

Merged

mtrmac mentioned this issue Jan 9, 2024

chore: add non-empty .golangci.yml; fix whitespace lint issues containers/image#2244

Closed

mtrmac mentioned this issue Jun 7, 2024

chunked format string issues? #1955

Closed

Honny1 self-assigned this Jun 25, 2024

This was referenced Jun 27, 2024

Enable staticcheck linter #1991

Merged

Solve errcheck warnings (part 0) #1993

Merged

Solve errcheck warnings (part 1) #1998

Merged

Solve errcheck warnings (part 2) #1999

Merged

This was referenced Jul 8, 2024

Solve errcheck warnings (part 3) #2002

Merged

Solve errcheck warnings (part 4) #2003

Merged

Solve errcheck warnings (part 5) #2006

Merged

Honny1 mentioned this issue Jul 11, 2024

Enable errcheck linter #2011

Merged

This was referenced Jul 15, 2024

Enable dupl linter #2026

Open

Enable stylecheck linter #2027

Open

Enable unparam linter #2028

Open

rhatdan closed this as completed Jul 15, 2024

mtrmac mentioned this issue Aug 15, 2024

Use gofumpt containers/image#2500

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider allowing more linters #1579

Consider allowing more linters #1579

mtrmac commented Apr 19, 2023 •

edited

Loading

mtrmac commented Apr 19, 2023 •

edited

Loading

rhatdan commented Apr 20, 2023

mtrmac commented Apr 21, 2023

rhatdan commented Apr 24, 2023

mtrmac commented May 25, 2023

mtrmac commented Jul 10, 2024

Honny1 commented Jul 10, 2024

Honny1 commented Jul 11, 2024 •

edited

Loading

rhatdan commented Jul 11, 2024

mtrmac commented Jul 11, 2024 •

edited

Loading

Honny1 commented Jul 12, 2024

Honny1 commented Jul 12, 2024

mtrmac commented Jul 12, 2024

Honny1 commented Jul 15, 2024 •

edited

Loading

Honny1 commented Jul 15, 2024

Consider allowing more linters #1579

Consider allowing more linters #1579

Comments

mtrmac commented Apr 19, 2023 • edited Loading

mtrmac commented Apr 19, 2023 • edited Loading

rhatdan commented Apr 20, 2023

mtrmac commented Apr 21, 2023

rhatdan commented Apr 24, 2023

mtrmac commented May 25, 2023

mtrmac commented Jul 10, 2024

Honny1 commented Jul 10, 2024

Honny1 commented Jul 11, 2024 • edited Loading

rhatdan commented Jul 11, 2024

mtrmac commented Jul 11, 2024 • edited Loading

Honny1 commented Jul 12, 2024

Honny1 commented Jul 12, 2024

mtrmac commented Jul 12, 2024

Honny1 commented Jul 15, 2024 • edited Loading

Honny1 commented Jul 15, 2024

mtrmac commented Apr 19, 2023 •

edited

Loading

mtrmac commented Apr 19, 2023 •

edited

Loading

Honny1 commented Jul 11, 2024 •

edited

Loading

mtrmac commented Jul 11, 2024 •

edited

Loading

Honny1 commented Jul 15, 2024 •

edited

Loading