Add several linters to CI and fix found issues (#542)

* fix a bunch of error strings

* add linters to makefile

* import sort

* renamed .template/ files to .tmpl to prevent goimports from failing to parse

* fix fatcontext errors

* fix prealloc lints

* ineffassign lints

* unused, not touching the lib/ dir

* fix perfsprint errors

* fixed or silenced static check errors

* last prealloc

* fix govet errors

* errcheck

* don't lint lib/ dir

* lint

* add golangci-yml lint and imports check to lint CI

* bump for tests

* don't error when hitting EOF in copy in http

* fix but in err handling in utility

* fix error handling in ReadAvailableWithOptions

* fix mssql error where we abort a scan if we can't get the tlsconn, we should proceed because we can still extract version and other info without a TLS conn

* pr review

Author: phillip-stephens

.github/workflows/linter.yml

@@ -50,4 +50,34 @@ jobs:
         uses: actions/upload-artifact@v4
         with:
           name: formatted-files
-          path: /tmp/gofmt
+          path: /tmp/gofmt
+  goimports-and-golangci-lint:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+      - name: Setup Go
+        uses: actions/setup-go@v5
+        with:
+          go-version: "1.23"
+      - name: Other lint
+        run: |
+          go install golang.org/x/tools/cmd/goimports@latest
+          output=$(goimports -d -local "github.com/zmap/zdns" ./)
+          if [ -n "$output" ]; then
+            echo "goimports found issues:"
+            echo "$output"
+            exit 1
+          else
+            echo "No issues found by goimports."
+          fi
+          output=$(gofmt -d .)
+            if [ -n "$output" ]; then
+                echo "gofmt found issues:"
+                echo "$output"
+                exit 1
+            else
+                echo "No issues found by gofmt."
+            fi
+      - name: golangci-lint
+        uses: golangci/golangci-lint-action@v8.0.0

.golangci.yml

@@ -0,0 +1,50 @@
+# This code is licensed under the terms of the MIT license https://opensource.org/license/mit
+# Copyright (c) 2021 Marat Reymers
+
+version: "2"
+linters:
+  default: none
+  enable:
+    - errcheck
+    - fatcontext
+    - govet
+    - ineffassign
+    - perfsprint
+    - prealloc
+    - staticcheck
+    - unused
+  settings:
+    errcheck:
+      check-type-assertions: true
+    govet:
+      disable:
+        - fieldalignment
+      enable-all: true
+      settings:
+        shadow:
+          strict: false
+  exclusions:
+    generated: lax
+    presets:
+      - comments
+      - common-false-positives
+      - legacy
+      - std-error-handling
+    rules:
+      - linters:
+          - errcheck
+        path: _test\.go
+    paths:
+      - third_party$
+      - builtin$
+      - examples$
+      - lib/*
+issues:
+  max-issues-per-linter: 50
+formatters:
+  exclusions:
+    generated: lax
+    paths:
+      - third_party$
+      - builtin$
+      - examples$

Makefile

@@ -20,6 +20,8 @@ test:
 
 lint:
 	gofmt -s -w $(shell find . -type f -name '*.go'| grep -v "/.template/")
+	goimports -w -local "github.com/zmap/zgrab2" ./
+	golangci-lint run
 	black .
 
 zgrab2: $(GO_FILES)

bin/bin.go

@@ -4,10 +4,10 @@ import (
 	"encoding/json"
 	"os"
 	"runtime/pprof"
+	"strconv"
 	"sync"
 	"time"
 
-	"fmt"
 	"runtime"
 	"strings"
 
@@ -35,9 +35,9 @@ func getCPUProfileFile() string {
 // YYYYMMDDhhmmss, and {NANOS} as the decimal nanosecond offset.
 func getFormattedFile(formatString string, when time.Time) string {
 	timestamp := when.Format("20060102150405")
-	nanos := fmt.Sprintf("%d", when.Nanosecond())
-	ret := strings.Replace(formatString, "{TIMESTAMP}", timestamp, -1)
-	ret = strings.Replace(ret, "{NANOS}", nanos, -1)
+	nanos := strconv.Itoa(when.Nanosecond())
+	ret := strings.ReplaceAll(formatString, "{TIMESTAMP}", timestamp)
+	ret = strings.ReplaceAll(ret, "{NANOS}", nanos)
 	return ret
 }
 
@@ -128,16 +128,24 @@ func ZGrab2Main() {
 			log.Fatalf("error parsing flags")
 		}
 		for i, fl := range flagsReturned {
-			f, _ := fl.(zgrab2.ScanFlags)
+			f, ok := fl.(zgrab2.ScanFlags)
+			if !ok {
+				log.Fatalf("error parsing flags as ScanFlags")
+			}
 			mod := zgrab2.GetModule(modTypes[i])
 			s := mod.NewScanner()
-			s.Init(f)
+
+			if err = s.Init(f); err != nil {
+				log.Panicf("could not initialize multiple scanner: %v", err)
+			}
 			zgrab2.RegisterScan(s.GetName(), s)
 		}
 	} else {
 		mod := zgrab2.GetModule(moduleType)
 		s := mod.NewScanner()
-		s.Init(flag)
+		if err = s.Init(flag); err != nil {
+			log.Panicf("could not initialize scanner %s: %v", moduleType, err)
+		}
 		zgrab2.RegisterScan(moduleType, s)
 	}
 	wg := sync.WaitGroup{}

config.go

@@ -40,7 +40,6 @@ type Config struct {
 	inputTargets          InputTargetsFunc
 	outputResults         OutputResultsFunc
 	customDNSNameservers  []string // will be non-empty if user specified custom DNS, we'll check these are reachable before populating
-	localAddr             *net.TCPAddr
 	localAddrs            []net.IP // will be non-empty if user specified local addresses
 	localPorts            []uint16 // will be non-empty if user specified local ports
 	useIPv4               bool     // true if zgrab should use IPv4 addresses after resolving domains
@@ -102,7 +101,7 @@ func validateFrameworkConfiguration() {
 	} else if len(config.MetaFileName) > 0 {
 		var err error
 		if config.metaFile, err = os.Create(config.MetaFileName); err != nil {
-			log.Fatal(fmt.Errorf("error creating meta file: %s", err))
+			log.Fatal(fmt.Errorf("error creating meta file: %w", err))
 		}
 	}
 
@@ -111,7 +110,7 @@ func validateFrameworkConfiguration() {
 	} else if len(config.StatusUpdatesFileName) > 0 {
 		var err error
 		if config.statusUpdatesFile, err = os.Create(config.StatusUpdatesFileName); err != nil {
-			log.Fatal(fmt.Errorf("error creating status updates file: %v", err))
+			log.Fatal(fmt.Errorf("error creating status updates file: %w", err))
 		}
 	}
 
@@ -295,11 +294,11 @@ func extractPorts(portString string) ([]uint16, error) {
 			}
 			startPort, err := parsePortString(parts[0])
 			if err != nil {
-				return nil, fmt.Errorf("invalid start port %s of port range: %v", parts[0], err)
+				return nil, fmt.Errorf("invalid start port %s of port range: %w", parts[0], err)
 			}
 			endPort, err := parsePortString(parts[1])
 			if err != nil {
-				return nil, fmt.Errorf("invalid end port %s of port range: %v", parts[1], err)
+				return nil, fmt.Errorf("invalid end port %s of port range: %w", parts[1], err)
 			}
 			if startPort >= endPort {
 				return nil, fmt.Errorf("start port %d must be less than end port %d", startPort, endPort)
@@ -312,7 +311,7 @@ func extractPorts(portString string) ([]uint16, error) {
 			// single port
 			port, err := parsePortString(portStr)
 			if err != nil {
-				return nil, fmt.Errorf("invalid port %s: %v", portStr, err)
+				return nil, fmt.Errorf("invalid port %s: %w", portStr, err)
 			}
 			portMap[port] = struct{}{}
 		}

conn.go

@@ -70,7 +70,7 @@ func (c *TimeoutConnection) SaturateTimeoutsToReadAndWriteTimeouts() {
 	// Get the minimum of the context deadline and the timeout
 	minDeadline := int64(math.MaxInt64)
 	if ctxDeadline, ok := c.ctx.Deadline(); ok {
-		minDeadline = int64(ctxDeadline.Sub(time.Now()))
+		minDeadline = int64(time.Until(ctxDeadline))
 	}
 	if c.SessionTimeout > 0 {
 		minDeadline = min(minDeadline, int64(c.SessionTimeout))
@@ -93,7 +93,7 @@ func (c *TimeoutConnection) SaturateTimeoutsToReadAndWriteTimeouts() {
 
 // TimeoutConnection.Read calls Read() on the underlying connection, using any configured deadlines
 func (c *TimeoutConnection) Read(b []byte) (n int, err error) {
-	if err := c.checkContext(); err != nil {
+	if err = c.checkContext(); err != nil {
 		return 0, err
 	}
 	origSize := len(b)
@@ -125,7 +125,7 @@ func (c *TimeoutConnection) Read(b []byte) (n int, err error) {
 
 // TimeoutConnection.Write calls Write() on the underlying connection, using any configured deadlines.
 func (c *TimeoutConnection) Write(b []byte) (n int, err error) {
-	if err := c.checkContext(); err != nil {
+	if err = c.checkContext(); err != nil {
 		return 0, err
 	}
 	c.SaturateTimeoutsToReadAndWriteTimeouts()
@@ -144,7 +144,7 @@ func (c *TimeoutConnection) SetReadDeadline(deadline time.Time) error {
 		return err
 	}
 	if !deadline.IsZero() {
-		c.ReadTimeout = deadline.Sub(time.Now())
+		c.ReadTimeout = time.Until(deadline)
 	}
 	return nil
 }
@@ -156,7 +156,7 @@ func (c *TimeoutConnection) SetWriteDeadline(deadline time.Time) error {
 		return err
 	}
 	if !deadline.IsZero() {
-		c.WriteTimeout = deadline.Sub(time.Now())
+		c.WriteTimeout = time.Until(deadline)
 	}
 	return nil
 }
@@ -291,7 +291,7 @@ func (d *Dialer) SetDefaults() *Dialer {
 			d.Dialer = &net.Dialer{}
 			// this may be a single IP address or a comma-separated list of IP addresses
 			ns := config.customDNSNameservers[rand.Intn(len(config.customDNSNameservers))]
-			d.Dialer.Resolver = &net.Resolver{
+			d.Resolver = &net.Resolver{
 				PreferGo: true,
 				Dial: func(ctx context.Context, network, address string) (net.Conn, error) {
 					return d.Dialer.DialContext(ctx, network, ns)

conn_bytelimit_test.go

@@ -11,17 +11,20 @@ import (
 )
 
 // Start a local echo server on port.
-func runEchoServer(t *testing.T, port int) {
+func runEchoServer(t *testing.T, port int) <-chan error {
 	endpoint := fmt.Sprintf("127.0.0.1:%d", port)
 	listener, err := net.Listen("tcp", endpoint)
 	if err != nil {
 		t.Fatal(err)
 	}
+	errChan := make(chan error)
 	go func() {
+		defer close(errChan)
 		defer listener.Close()
 		sock, err := listener.Accept()
 		if err != nil {
-			t.Fatal(err)
+			errChan <- err
+			return
 		}
 		defer sock.Close()
 
@@ -30,12 +33,12 @@ func runEchoServer(t *testing.T, port int) {
 			n, err := sock.Read(buf)
 			if err != nil {
 				if err != io.EOF && !strings.Contains(err.Error(), "connection reset") {
-					t.Fatal(err)
+					errChan <- err
 				}
 				return
 			}
 			sock.SetWriteDeadline(time.Now().Add(time.Millisecond * 250))
-			n, err = sock.Write(buf[0:n])
+			_, err = sock.Write(buf[0:n])
 			if err != nil {
 				if err != io.EOF && !strings.Contains(err.Error(), "connection reset") && !strings.Contains(err.Error(), "broken pipe") {
 					t.Logf("Unexpected error writing to client: %v", err)
@@ -44,6 +47,7 @@ func runEchoServer(t *testing.T, port int) {
 			}
 		}
 	}()
+	return errChan
 }
 
 // Interface for getting a TimeoutConnection; we want to test both the dialer and the direct Dial functions.
@@ -185,7 +189,7 @@ func getTestBuffer(size int) []byte {
 
 // Check that buf is of the type returned by getTestBuffer.
 func checkTestBuffer(buf []byte) bool {
-	if buf == nil || len(buf) == 0 {
+	if len(buf) == 0 {
 		return false
 	}
 	for i, v := range buf {
@@ -344,7 +348,7 @@ func runBytesReadLimitTrial(t *testing.T, connector timeoutConnector, idx int, m
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	port := 0x1234 + idx
-	runEchoServer(t, port)
+	errChan := runEchoServer(t, port)
 	conn, err := connector.connect(ctx, t, port, idx)
 	if err != nil {
 		t.Fatalf("Error dialing: %v", err)
@@ -360,7 +364,19 @@ func runBytesReadLimitTrial(t *testing.T, connector timeoutConnector, idx int, m
 		}
 	}()
 	defer conn.Close()
-	return method(cfg, t, conn, idx)
+	err = method(cfg, t, conn, idx)
+	if err != nil {
+		return err
+	}
+	// Check if the server has any errors
+	select {
+	case err := <-errChan:
+		if err != nil {
+			t.Fatalf("Error in server: %v", err)
+		}
+	default:
+	}
+	return nil
 }
 
 // Run a full set of trials on the connector -- ten with a single send, and ten with multiple sends.

conn_timeout_test.go

@@ -133,7 +133,7 @@ func (cfg *connTimeoutTestConfig) runServer(t *testing.T, stopServer <-chan stru
 		}
 		defer sock.Close()
 		time.Sleep(cfg.serverWriteDelay)
-		if err := _write(sock, cfg.serverToClientPayload); err != nil {
+		if err = _write(sock, cfg.serverToClientPayload); err != nil {
 			errorChan <- serverError(clientTestStepRead, err)
 			return
 		}
@@ -152,7 +152,6 @@ func (cfg *connTimeoutTestConfig) runServer(t *testing.T, stopServer <-chan stru
 			t.Errorf("%s: clientToServerPayload mismatch", cfg.name)
 		}
 		<-stopServer // wait for client to indicate server can exit
-		return
 	}()
 	return errorChan
 }

fake_resolver.go

@@ -4,9 +4,10 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"golang.org/x/net/dns/dnsmessage"
 	"net"
 	"time"
+
+	"golang.org/x/net/dns/dnsmessage"
 )
 
 // Fake DNS Resolver, to force a DNS lookup to return a pinned address
@@ -19,7 +20,7 @@ import (
 func NewFakeResolver(ipstr string) (*net.Resolver, error) {
 	ip := net.ParseIP(ipstr)
 	if len(ip) < 4 {
-		return nil, fmt.Errorf("Fake resolver can't use non-IP '%s'", ipstr)
+		return nil, fmt.Errorf("fake resolver can't use non-IP '%s'", ipstr)
 	}
 	fDNS := FakeDNSServer{
 		IP: ip,
@@ -86,7 +87,7 @@ func (f *FakeDNSServer) fakeDNS(s string, dmsg dnsmessage.Message) (r dnsmessage
 			},
 		}
 	default:
-		r.Header.RCode = dnsmessage.RCodeNameError
+		r.RCode = dnsmessage.RCodeNameError
 	}
 
 	return r, nil
@@ -124,7 +125,7 @@ func (fc *fakeDNSConn) Read(b []byte) (int, error) {
 	bb := make([]byte, 2, 514)
 	bb, err = resp.AppendPack(bb)
 	if err != nil {
-		return 0, fmt.Errorf("cannot marshal DNS message: %v", err)
+		return 0, fmt.Errorf("cannot marshal DNS message: %w", err)
 	}
 
 	bb = bb[2:]