[FIXED] New staticcheck fixes (#5826)

@neilalexander I do believe this uncovered two off by one conditions in node48 under stree. Could you take a look, kept that change separate. --------- Signed-off-by: Derek Collison <[email protected]> Signed-off-by: Neil Twigg <[email protected]> Co-authored-by: Neil Twigg <[email protected]>
nats-io · Aug 25, 2024 · 3fd298e · 3fd298e
1 parent c4ee477
commit 3fd298e
Show file tree

Hide file tree

Showing 14 changed files with 51 additions and 39 deletions.
diff --git a/internal/ldap/dn_test.go b/internal/ldap/dn_test.go
@@ -1,5 +1,7 @@
 // Copyright (c) 2011-2015 Michael Mitton ([email protected])
 // Portions copyright (c) 2015-2016 go-ldap Authors
+// Static-Check Fixes Copyright 2024 The NATS Authors
+
 package ldap
 
 import (
@@ -53,7 +55,7 @@ func TestSuccessfulDNParsing(t *testing.T) {
 	for test, answer := range testcases {
 		dn, err := ParseDN(test)
 		if err != nil {
-			t.Errorf(err.Error())
+			t.Error(err.Error())
 			continue
 		}
 		if !reflect.DeepEqual(dn, &answer) {

diff --git a/server/certidp/certidp.go b/server/certidp/certidp.go
@@ -1,4 +1,4 @@
-// Copyright 2023 The NATS Authors
+// Copyright 2023-2024 The NATS Authors
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
@@ -222,7 +222,7 @@ func CertOCSPEligible(link *ChainLink) bool {
 	if link == nil || link.Leaf.Raw == nil || len(link.Leaf.Raw) == 0 {
 		return false
 	}
-	if link.Leaf.OCSPServer == nil || len(link.Leaf.OCSPServer) == 0 {
+	if len(link.Leaf.OCSPServer) == 0 {
 		return false
 	}
 	urls := getWebEndpoints(link.Leaf.OCSPServer)

diff --git a/server/certidp/ocsp_responder.go b/server/certidp/ocsp_responder.go
@@ -1,4 +1,4 @@
-// Copyright 2023 The NATS Authors
+// Copyright 2023-2024 The NATS Authors
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
@@ -15,6 +15,7 @@ package certidp
 
 import (
 	"encoding/base64"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -26,7 +27,7 @@ import (
 
 func FetchOCSPResponse(link *ChainLink, opts *OCSPPeerConfig, log *Log) ([]byte, error) {
 	if link == nil || link.Leaf == nil || link.Issuer == nil || opts == nil || log == nil {
-		return nil, fmt.Errorf(ErrInvalidChainlink)
+		return nil, errors.New(ErrInvalidChainlink)
 	}
 
 	timeout := time.Duration(opts.Timeout * float64(time.Second))
@@ -59,7 +60,7 @@ func FetchOCSPResponse(link *ChainLink, opts *OCSPPeerConfig, log *Log) ([]byte,
 	responders := *link.OCSPWebEndpoints
 
 	if len(responders) == 0 {
-		return nil, fmt.Errorf(ErrNoAvailOCSPServers)
+		return nil, errors.New(ErrNoAvailOCSPServers)
 	}
 
 	var raw []byte

diff --git a/server/client.go b/server/client.go
@@ -3017,7 +3017,7 @@ func (c *client) addShadowSub(sub *subscription, ime *ime, enact bool) (*subscri
 	if err := im.acc.sl.Insert(&nsub); err != nil {
 		errs := fmt.Sprintf("Could not add shadow import subscription for account %q", im.acc.Name)
 		c.Debugf(errs)
-		return nil, fmt.Errorf(errs)
+		return nil, errors.New(errs)
 	}
 
 	// Update our route map here. But only if we are not a leaf node or a hub leafnode.

diff --git a/server/gateway_test.go b/server/gateway_test.go
@@ -1,4 +1,4 @@
-// Copyright 2018-2020 The NATS Authors
+// Copyright 2018-2024 The NATS Authors
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
@@ -211,7 +211,7 @@ func waitCh(t *testing.T, ch chan bool, errTxt string) {
 	case <-ch:
 		return
 	case <-time.After(5 * time.Second):
-		t.Fatalf(errTxt)
+		t.Fatal(errTxt)
 	}
 }
 
@@ -5055,7 +5055,7 @@ func TestGatewayMapReplyOnlyForRecentSub(t *testing.T) {
 	select {
 	case e := <-errCh:
 		if e != nil {
-			t.Fatalf(e.Error())
+			t.Fatal(e.Error())
 		}
 	case <-time.After(time.Second):
 		t.Fatalf("Did not get replies")

diff --git a/server/jetstream_cluster_3_test.go b/server/jetstream_cluster_3_test.go
@@ -5857,15 +5857,15 @@ func TestJetStreamClusterRestartThenScaleStreamReplicas(t *testing.T) {
 			select {
 			case dl := <-loggers[0].dbgCh:
 				if strings.Contains(dl, condition) {
-					errCh <- fmt.Errorf(condition)
+					errCh <- errors.New(condition)
 				}
 			case dl := <-loggers[1].dbgCh:
 				if strings.Contains(dl, condition) {
-					errCh <- fmt.Errorf(condition)
+					errCh <- errors.New(condition)
 				}
 			case dl := <-loggers[2].dbgCh:
 				if strings.Contains(dl, condition) {
-					errCh <- fmt.Errorf(condition)
+					errCh <- errors.New(condition)
 				}
 			case <-ctx.Done():
 				return

diff --git a/server/stree/node48.go b/server/stree/node48.go
@@ -51,9 +51,9 @@ func (n *node48) isFull() bool { return n.size >= 48 }
 
 func (n *node48) grow() node {
 	nn := newNode256(n.prefix)
-	for c := byte(0); c < 255; c++ {
-		if i := n.key[c]; i > 0 {
-			nn.addChild(c, n.child[i-1])
+	for c := 0; c < len(n.key); c++ {
+		if i := n.key[byte(c)]; i > 0 {
+			nn.addChild(byte(c), n.child[i-1])
 		}
 	}
 	return nn
@@ -69,9 +69,9 @@ func (n *node48) deleteChild(c byte) {
 	last := byte(n.size - 1)
 	if i < last {
 		n.child[i] = n.child[last]
-		for c := byte(0); c <= 255; c++ {
-			if n.key[c] == last+1 {
-				n.key[c] = i + 1
+		for ic := 0; ic < len(n.key); ic++ {
+			if n.key[byte(ic)] == last+1 {
+				n.key[byte(ic)] = i + 1
 				break
 			}
 		}
@@ -87,9 +87,9 @@ func (n *node48) shrink() node {
 		return nil
 	}
 	nn := newNode16(nil)
-	for c := byte(0); c < 255; c++ {
-		if i := n.key[c]; i > 0 {
-			nn.addChild(c, n.child[i-1])
+	for c := 0; c < len(n.key); c++ {
+		if i := n.key[byte(c)]; i > 0 {
+			nn.addChild(byte(c), n.child[i-1])
 		}
 	}
 	return nn

diff --git a/server/stree/stree_test.go b/server/stree/stree_test.go
@@ -760,6 +760,15 @@ func TestSubjectTreeNode48(t *testing.T) {
 	require_Equal(t, iterations, 2)
 	require_True(t, gotB)
 	require_True(t, gotC)
+
+	// Check for off-by-one on byte 255 as found by staticcheck, see
+	// https://github.com/nats-io/nats-server/pull/5826.
+	n.addChild(255, &c)
+	require_Equal(t, n.key[255], 3)
+	grown := n.grow().(*node256)
+	require_True(t, grown.findChild(255) != nil)
+	shrunk := n.shrink().(*node16)
+	require_True(t, shrunk.findChild(255) != nil)
 }
 
 func TestSubjectTreeMatchNoCallbackDupe(t *testing.T) {

diff --git a/server/sublist_test.go b/server/sublist_test.go
@@ -1,4 +1,4 @@
-// Copyright 2016-2023 The NATS Authors
+// Copyright 2016-2024 The NATS Authors
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
@@ -984,7 +984,7 @@ func TestSublistRaceOnMatch(t *testing.T) {
 	wg.Wait()
 	select {
 	case e := <-errCh:
-		t.Fatalf(e.Error())
+		t.Fatal(e.Error())
 	default:
 	}
 }

diff --git a/server/websocket.go b/server/websocket.go
@@ -671,7 +671,7 @@ func (c *client) wsHandleProtocolError(message string) error {
 	buf := wsCreateCloseMessage(wsCloseStatusProtocolError, message)
 	c.wsEnqueueControlMessage(wsCloseMessage, buf)
 	nbPoolPut(buf) // wsEnqueueControlMessage has taken a copy.
-	return fmt.Errorf(message)
+	return errors.New(message)
 }
 
 // Create a close message with the given `status` and `body`.

diff --git a/test/client_cluster_test.go b/test/client_cluster_test.go
@@ -1,4 +1,4 @@
-// Copyright 2013-2019 The NATS Authors
+// Copyright 2013-2024 The NATS Authors
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
@@ -321,7 +321,7 @@ func TestRequestsAcrossRoutes(t *testing.T) {
 	nc1.Flush()
 
 	if err := checkExpectedSubs(1, srvA, srvB); err != nil {
-		t.Fatalf(err.Error())
+		t.Fatal(err.Error())
 	}
 
 	var resp string
@@ -372,7 +372,7 @@ func TestRequestsAcrossRoutesToQueues(t *testing.T) {
 	})
 
 	if err := checkExpectedSubs(2, srvA, srvB); err != nil {
-		t.Fatalf(err.Error())
+		t.Fatal(err.Error())
 	}
 
 	var resp string

diff --git a/test/leafnode_test.go b/test/leafnode_test.go
@@ -1,4 +1,4 @@
-// Copyright 2019-2020 The NATS Authors
+// Copyright 2019-2024 The NATS Authors
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
@@ -4376,13 +4376,13 @@ func TestLeafnodeHeaders(t *testing.T) {
 
 	snc, err := nats.Connect(srv.ClientURL())
 	if err != nil {
-		t.Fatalf(err.Error())
+		t.Fatal(err.Error())
 	}
 	defer snc.Close()
 
 	lnc, err := nats.Connect(leaf.ClientURL())
 	if err != nil {
-		t.Fatalf(err.Error())
+		t.Fatal(err.Error())
 	}
 	defer lnc.Close()
 
@@ -4407,7 +4407,7 @@ func TestLeafnodeHeaders(t *testing.T) {
 	}
 	err = snc.PublishMsg(msg)
 	if err != nil {
-		t.Fatalf(err.Error())
+		t.Fatal(err.Error())
 	}
 
 	smsg, err := ssub.NextMsg(time.Second)

diff --git a/test/new_routes_test.go b/test/new_routes_test.go
@@ -892,7 +892,7 @@ func TestNewRouteSinglePublishOnNewAccount(t *testing.T) {
 	expectA(pongRe)
 
 	if err := checkExpectedSubs(1, srvB); err != nil {
-		t.Fatalf(err.Error())
+		t.Fatal(err.Error())
 	}
 
 	clientB := createClientConn(t, optsB.Host, optsB.Port)
@@ -1097,7 +1097,7 @@ func testNewRouteStreamImport(t *testing.T, duplicateSub bool) {
 	// a subscription on "foo" for account $foo due to import.
 	// So total of 2 subs.
 	if err := checkExpectedSubs(2, srvA); err != nil {
-		t.Fatalf(err.Error())
+		t.Fatal(err.Error())
 	}
 
 	// Send on clientA
@@ -1122,7 +1122,7 @@ func testNewRouteStreamImport(t *testing.T, duplicateSub bool) {
 	expectB(pongRe)
 
 	if err := checkExpectedSubs(0, srvA); err != nil {
-		t.Fatalf(err.Error())
+		t.Fatal(err.Error())
 	}
 
 	sendA("PUB foo 2\r\nok\r\nPING\r\n")

diff --git a/test/norace_test.go b/test/norace_test.go
@@ -1,4 +1,4 @@
-// Copyright 2019-2020 The NATS Authors
+// Copyright 2019-2024 The NATS Authors
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
@@ -96,7 +96,7 @@ func TestNoRaceRouteSendSubs(t *testing.T) {
 	clientBExpect(pongRe)
 
 	if err := checkExpectedSubs(totalPerServer, srvA, srvB); err != nil {
-		t.Fatalf(err.Error())
+		t.Fatal(err.Error())
 	}
 
 	routes := fmt.Sprintf(`
@@ -113,7 +113,7 @@ func TestNoRaceRouteSendSubs(t *testing.T) {
 
 	checkClusterFormed(t, srvA, srvB)
 	if err := checkExpectedSubs(2*totalPerServer, srvA, srvB); err != nil {
-		t.Fatalf(err.Error())
+		t.Fatal(err.Error())
 	}
 
 	checkSlowConsumers := func(t *testing.T) {
@@ -166,7 +166,7 @@ func TestNoRaceRouteSendSubs(t *testing.T) {
 	defer requestorOnB.Close()
 
 	if err := checkExpectedSubs(2*totalPerServer+2, srvA, srvB); err != nil {
-		t.Fatalf(err.Error())
+		t.Fatal(err.Error())
 	}
 
 	totalReplies := 120000