Merge pull request moby#48825 from thaJeztah/update_linting

golangci: enable all govet linters, run gosec on tests as well

.golangci.yml

@@ -83,6 +83,11 @@ linters-settings:
       - G307 # G307: Deferring unsafe method "*os.File" on type "Close" (also EXC0008); (TODO: evaluate these and fix where needed: G307: Deferring unsafe method "*os.File" on type "Close")
       - G504 # G504: Blocklisted import net/http/cgi: Go versions < 1.6.3 are vulnerable to Httpoxy attack: (CVE-2016-5386); (only affects go < 1.6.3)
 
+  govet:
+    enable-all: true
+    disable:
+      - fieldalignment # TODO: evaluate which ones should be updated.
+
   importas:
     # Do not allow unaliased imports of aliased packages.
     no-unaliased: true
@@ -131,6 +136,10 @@ issues:
     - path: _test\.go
       linters:
         - errcheck
+
+    - text: "G404: Use of weak random number generator"
+      path: _test\.go
+      linters:
         - gosec
 
     # Suppress golint complaining about generated types in api/types/
@@ -153,6 +162,14 @@ issues:
       linters:
         - staticcheck
 
+    - text: '^shadow: declaration of "(ctx|err|ok)" shadows declaration'
+      linters:
+        - govet
+    - text: '^shadow: declaration of "(out)" shadows declaration'
+      path: _test\.go
+      linters:
+        - govet
+
   # Maximum issues count per one linter. Set to 0 to disable. Default is 50.
   max-issues-per-linter: 0
 

builder/builder-next/builder.go

@@ -291,8 +291,6 @@ func (b *Builder) Build(ctx context.Context, opt backend.BuildConfig) (*builder.
 
 	var out builder.Result
 
-	id := identity.NewID()
-
 	frontendAttrs := map[string]string{}
 
 	if opt.Options.Target != "" {
@@ -405,6 +403,7 @@ func (b *Builder) Build(ctx context.Context, opt backend.BuildConfig) (*builder.
 		}
 	}
 
+	id := identity.NewID()
 	req := &controlapi.SolveRequest{
 		Ref: id,
 		Exporters: []*controlapi.Exporter{
@@ -432,12 +431,12 @@ func (b *Builder) Build(ctx context.Context, opt backend.BuildConfig) (*builder.
 		if exporterName != exporter.Moby && exporterName != client.ExporterImage {
 			return nil
 		}
-		id, ok := resp.ExporterResponse["containerimage.digest"]
+		imgID, ok := resp.ExporterResponse["containerimage.digest"]
 		if !ok {
 			return errors.Errorf("missing image id")
 		}
-		out.ImageID = id
-		return aux.Emit("moby.image.id", types.BuildResult{ID: id})
+		out.ImageID = imgID
+		return aux.Emit("moby.image.id", types.BuildResult{ID: imgID})
 	})
 
 	ch := make(chan *controlapi.StatusResponse)

builder/builder-next/exporter/mobyexporter/writer.go

@@ -80,7 +80,7 @@ func patchImageConfig(dt []byte, dps []digest.Digest, history []ocispec.History,
 	}
 
 	if cache != nil {
-		dt, err := json.Marshal(cache.Data)
+		dt, err = json.Marshal(cache.Data)
 		if err != nil {
 			return nil, err
 		}

builder/dockerfile/builder.go

@@ -358,20 +358,20 @@ func BuildFromConfig(ctx context.Context, config *container.Config, changes []st
 		commands = append(commands, cmd)
 	}
 
-	dispatchRequest := newDispatchRequest(b, dockerfile.EscapeToken, nil, NewBuildArgs(b.options.BuildArgs), newStagesBuildResults())
+	req := newDispatchRequest(b, dockerfile.EscapeToken, nil, NewBuildArgs(b.options.BuildArgs), newStagesBuildResults())
 	// We make mutations to the configuration, ensure we have a copy
-	dispatchRequest.state.runConfig = copyRunConfig(config)
-	dispatchRequest.state.imageID = config.Image
-	dispatchRequest.state.operatingSystem = os
+	req.state.runConfig = copyRunConfig(config)
+	req.state.imageID = config.Image
+	req.state.operatingSystem = os
 	for _, cmd := range commands {
-		err := dispatch(ctx, dispatchRequest, cmd)
+		err := dispatch(ctx, req, cmd)
 		if err != nil {
 			return nil, errdefs.InvalidParameter(err)
 		}
-		dispatchRequest.state.updateRunConfig()
+		req.state.updateRunConfig()
 	}
 
-	return dispatchRequest.state.runConfig, nil
+	return req.state.runConfig, nil
 }
 
 func convertMapToEnvList(m map[string]string) []string {

builder/dockerfile/copy.go

@@ -214,15 +214,15 @@ func (o *copier) calcCopyInfo(origPath string, allowWildcards bool) ([]copyInfo,
 	}
 
 	// Deal with the single file case
-	copyInfo, err := copyInfoForFile(o.source, origPath)
+	info, err := copyInfoForFile(o.source, origPath)
 	switch {
 	case imageSource == nil && errors.Is(err, os.ErrNotExist):
 		return nil, errors.Wrapf(err, "file not found in build context or excluded by .dockerignore")
 	case err != nil:
 		return nil, err
-	case copyInfo.hash != "":
-		o.storeInPathCache(imageSource, origPath, copyInfo.hash)
-		return newCopyInfos(copyInfo), err
+	case info.hash != "":
+		o.storeInPathCache(imageSource, origPath, info.hash)
+		return newCopyInfos(info), err
 	}
 
 	// TODO: remove, handle dirs in Hash()

builder/dockerfile/dispatchers.go

@@ -91,17 +91,17 @@ func dispatchAdd(ctx context.Context, d dispatchRequest, c *instructions.AddComm
 		return errors.New("the --chmod option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled")
 	}
 	downloader := newRemoteSourceDownloader(d.builder.Output, d.builder.Stdout)
-	copier := copierFromDispatchRequest(d, downloader, nil)
-	defer copier.Cleanup()
+	cpr := copierFromDispatchRequest(d, downloader, nil)
+	defer cpr.Cleanup()
 
-	copyInstruction, err := copier.createCopyInstruction(c.SourcesAndDest, "ADD")
+	instruction, err := cpr.createCopyInstruction(c.SourcesAndDest, "ADD")
 	if err != nil {
 		return err
 	}
-	copyInstruction.chownStr = c.Chown
-	copyInstruction.allowLocalDecompression = true
+	instruction.chownStr = c.Chown
+	instruction.allowLocalDecompression = true
 
-	return d.builder.performCopy(ctx, d, copyInstruction)
+	return d.builder.performCopy(ctx, d, instruction)
 }
 
 // COPY foo /path
@@ -119,17 +119,17 @@ func dispatchCopy(ctx context.Context, d dispatchRequest, c *instructions.CopyCo
 			return errors.Wrapf(err, "invalid from flag value %s", c.From)
 		}
 	}
-	copier := copierFromDispatchRequest(d, errOnSourceDownload, im)
-	defer copier.Cleanup()
-	copyInstruction, err := copier.createCopyInstruction(c.SourcesAndDest, "COPY")
+	cpr := copierFromDispatchRequest(d, errOnSourceDownload, im)
+	defer cpr.Cleanup()
+	instruction, err := cpr.createCopyInstruction(c.SourcesAndDest, "COPY")
 	if err != nil {
 		return err
 	}
-	copyInstruction.chownStr = c.Chown
-	if c.From != "" && copyInstruction.chownStr == "" {
-		copyInstruction.preserveOwnership = true
+	instruction.chownStr = c.Chown
+	if c.From != "" && instruction.chownStr == "" {
+		instruction.preserveOwnership = true
 	}
-	return d.builder.performCopy(ctx, d, copyInstruction)
+	return d.builder.performCopy(ctx, d, instruction)
 }
 
 func (d *dispatchRequest) getImageMount(ctx context.Context, imageRefOrID string) (*imageMount, error) {
@@ -158,8 +158,8 @@ func initializeStage(ctx context.Context, d dispatchRequest, cmd *instructions.S
 	}
 
 	var platform *ocispec.Platform
-	if v := cmd.Platform; v != "" {
-		v, err := d.getExpandedString(d.shlex, v)
+	if val := cmd.Platform; val != "" {
+		v, err := d.getExpandedString(d.shlex, val)
 		if err != nil {
 			return errors.Wrapf(err, "failed to process arguments for platform %s", v)
 		}

client/hijack_test.go

@@ -9,6 +9,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"testing"
+	"time"
 
 	"github.com/docker/docker/api/server/httputils"
 	"github.com/docker/docker/api/types"
@@ -20,36 +21,39 @@ func TestTLSCloseWriter(t *testing.T) {
 	t.Parallel()
 
 	var chErr chan error
-	ts := &httptest.Server{Config: &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-		chErr = make(chan error, 1)
-		defer close(chErr)
-		if err := httputils.ParseForm(req); err != nil {
-			chErr <- errors.Wrap(err, "error parsing form")
-			http.Error(w, err.Error(), http.StatusInternalServerError)
-			return
-		}
-		r, rw, err := httputils.HijackConnection(w)
-		if err != nil {
-			chErr <- errors.Wrap(err, "error hijacking connection")
-			http.Error(w, err.Error(), http.StatusInternalServerError)
-			return
-		}
-		defer r.Close()
-
-		fmt.Fprint(rw, "HTTP/1.1 101 UPGRADED\r\nContent-Type: application/vnd.docker.raw-stream\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\n")
-
-		buf := make([]byte, 5)
-		_, err = r.Read(buf)
-		if err != nil {
-			chErr <- errors.Wrap(err, "error reading from client")
-			return
-		}
-		_, err = rw.Write(buf)
-		if err != nil {
-			chErr <- errors.Wrap(err, "error writing to client")
-			return
-		}
-	})}}
+	ts := &httptest.Server{Config: &http.Server{
+		ReadHeaderTimeout: 5 * time.Minute, // "G112: Potential Slowloris Attack (gosec)"; not a real concern for our use, so setting a long timeout.
+		Handler: http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+			chErr = make(chan error, 1)
+			defer close(chErr)
+			if err := httputils.ParseForm(req); err != nil {
+				chErr <- errors.Wrap(err, "error parsing form")
+				http.Error(w, err.Error(), http.StatusInternalServerError)
+				return
+			}
+			r, rw, err := httputils.HijackConnection(w)
+			if err != nil {
+				chErr <- errors.Wrap(err, "error hijacking connection")
+				http.Error(w, err.Error(), http.StatusInternalServerError)
+				return
+			}
+			defer r.Close()
+
+			fmt.Fprint(rw, "HTTP/1.1 101 UPGRADED\r\nContent-Type: application/vnd.docker.raw-stream\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\n")
+
+			buf := make([]byte, 5)
+			_, err = r.Read(buf)
+			if err != nil {
+				chErr <- errors.Wrap(err, "error reading from client")
+				return
+			}
+			_, err = rw.Write(buf)
+			if err != nil {
+				chErr <- errors.Wrap(err, "error writing to client")
+				return
+			}
+		}),
+	}}
 
 	var (
 		l   net.Listener

daemon/containerd/image_delete.go

@@ -375,13 +375,13 @@ func (i *ImageService) imageDeleteHelper(ctx context.Context, img images.Image,
 			return err
 		}
 		if len(children) > 0 {
-			img := images.Image{
+			_, err = i.images.Create(ctx, images.Image{
 				Name:      danglingImageName(img.Target.Digest),
 				Target:    img.Target,
 				CreatedAt: time.Now(),
 				Labels:    img.Labels,
-			}
-			if _, err = i.images.Create(ctx, img); err != nil && !cerrdefs.IsAlreadyExists(err) {
+			})
+			if err != nil && !cerrdefs.IsAlreadyExists(err) {
 				return fmt.Errorf("failed to create dangling image: %w", err)
 			}
 		}

daemon/containerd/image_import.go

@@ -367,9 +367,6 @@ func storeJson(ctx context.Context, cs content.Ingester, mt string, obj interfac
 		return ocispec.Descriptor{}, errdefs.InvalidParameter(err)
 	}
 	configDigest := digest.FromBytes(configData)
-	if err != nil {
-		return ocispec.Descriptor{}, errdefs.InvalidParameter(err)
-	}
 	desc := ocispec.Descriptor{
 		MediaType: mt,
 		Digest:    configDigest,

daemon/containerd/image_test.go

@@ -170,7 +170,7 @@ func TestLookup(t *testing.T) {
 	}
 
 	t.Run("fail-inconsistency", func(t *testing.T) {
-		service := &ImageService{
+		svc := &ImageService{
 			images: &mutateOnGetImageStore{
 				Store: service.images,
 				getMutations: []images.Image{
@@ -199,12 +199,12 @@ func TestLookup(t *testing.T) {
 			},
 		}
 
-		_, _, err := service.resolveAllReferences(ctx, "test/volatile:inconsistent")
+		_, _, err := svc.resolveAllReferences(ctx, "test/volatile:inconsistent")
 		assert.ErrorIs(t, err, errInconsistentData)
 	})
 
 	t.Run("retry-inconsistency", func(t *testing.T) {
-		service := &ImageService{
+		svc := &ImageService{
 			images: &mutateOnGetImageStore{
 				Store: service.images,
 				getMutations: []images.Image{
@@ -221,7 +221,7 @@ func TestLookup(t *testing.T) {
 			},
 		}
 
-		img, all, err := service.resolveAllReferences(ctx, "test/volatile:retried")
+		img, all, err := svc.resolveAllReferences(ctx, "test/volatile:retried")
 		assert.NilError(t, err)
 
 		assert.Assert(t, img != nil)

daemon/graphdriver/btrfs/btrfs.go

@@ -559,7 +559,7 @@ func (d *Driver) Create(id, parent string, opts *graphdriver.CreateOpts) error {
 func (d *Driver) parseStorageOpt(storageOpt map[string]string, driver *Driver) error {
 	// Read size to change the subvolume disk quota per container
 	for key, val := range storageOpt {
-		key := strings.ToLower(key)
+		key = strings.ToLower(key)
 		switch key {
 		case "size":
 			size, err := units.RAMInBytes(val)

daemon/graphdriver/overlay2/overlay.go

@@ -427,7 +427,7 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts) (retErr
 func (d *Driver) parseStorageOpt(storageOpt map[string]string, driver *Driver) error {
 	// Read size to set the disk project quota per container
 	for key, val := range storageOpt {
-		key := strings.ToLower(key)
+		key = strings.ToLower(key)
 		switch key {
 		case "size":
 			size, err := units.RAMInBytes(val)

daemon/images/image.go

@@ -265,9 +265,9 @@ func (m *onlyFallbackMatcher) Match(other ocispec.Platform) bool {
 		// If there is a variant then this fallback does not apply, and there is no match
 		return false
 	}
-	otherN := platforms.Normalize(other)
-	otherN.Variant = "" // normalization adds a default variant... which is the whole problem with `platforms.Only`
 
-	return m.p.OS == otherN.OS &&
-		m.p.Architecture == otherN.Architecture
+	// note that platforms.Normalize adds a default variant... which is the
+	// whole problem with [platforms.Only], so we can't match on that.
+	otherN := platforms.Normalize(other)
+	return m.p.OS == otherN.OS && m.p.Architecture == otherN.Architecture
 }

daemon/logger/awslogs/cloudwatchlogs_test.go

@@ -1649,6 +1649,7 @@ func TestNewAWSLogsClientCredentialEndpointDetect(t *testing.T) {
 	// required for the cloudwatchlogs client
 	t.Setenv("AWS_REGION", "us-west-2")
 
+	// #nosec G101 -- ignore potential hardcoded credentials
 	credsResp := `{
 		"AccessKeyId" :    "test-access-key-id",
 		"SecretAccessKey": "test-secret-access-key"

daemon/logger/splunk/splunkhecmock_test.go

@@ -10,6 +10,7 @@ import (
 	"net/http"
 	"sync"
 	"testing"
+	"time"
 )
 
 func (message *splunkMessage) EventAsString() (string, error) {
@@ -76,7 +77,12 @@ func (hec *HTTPEventCollectorMock) URL() string {
 }
 
 func (hec *HTTPEventCollectorMock) Serve() error {
-	return http.Serve(hec.tcpListener, hec)
+	srv := &http.Server{
+		Handler: hec,
+
+		ReadHeaderTimeout: 5 * time.Minute, // "G112: Potential Slowloris Attack (gosec)"; not a real concern for our use, so setting a long timeout.
+	}
+	return srv.Serve(hec.tcpListener)
 }
 
 func (hec *HTTPEventCollectorMock) Close() error {