Update make lint and fix issues (#85)

Co-authored-by: James Otting <[email protected]>

Author: ale7714

Makefile

@@ -46,12 +46,13 @@ clean:
 	rm -rf bin/
 
 bin/golangci-lint: Makefile
-	GOBIN=`pwd`/bin go install github.com/golangci/golangci-lint/cmd/[email protected]
+	GOOS='' GOBIN=`pwd`/bin go install github.com/golangci/golangci-lint/cmd/[email protected]
 
 .PHONY: lint
 lint: bin/golangci-lint
 	go mod tidy
-	bin/golangci-lint run -v --fix
+	GOOS='linux' bin/golangci-lint run -v --fix
+	GOOS='windows' bin/golangci-lint run -v --fix
 
 .PHONY: test
 test:

cmd/viam-agent/main.go

@@ -29,7 +29,7 @@ var (
 
 	// only changed/set at startup, so no mutex.
 	globalLogger = logging.NewLogger("viam-agent")
-	globalCancel func() //nolint:unused
+	globalCancel context.CancelFunc
 )
 
 //nolint:lll
@@ -46,11 +46,11 @@ type agentOpts struct {
 
 //nolint:gocognit
 func commonMain() {
-	ctx, cancel := setupExitSignalHandling()
-	globalCancel = cancel
+	var ctx context.Context
+	ctx, globalCancel = setupExitSignalHandling()
 
 	defer func() {
-		cancel()
+		globalCancel()
 		activeBackgroundWorkers.Wait()
 	}()
 

cmd/viam-agent/main_windows.go

@@ -8,13 +8,16 @@ import (
 	"github.com/viamrobotics/agent"
 	"github.com/viamrobotics/agent/utils"
 	"go.viam.com/rdk/logging"
+	goutils "go.viam.com/utils"
 	"golang.org/x/sys/windows/svc"
 	"golang.org/x/sys/windows/svc/debug"
 	"golang.org/x/sys/windows/svc/eventlog"
 )
 
-var elog debug.Log
-var _ svc.Handler = (*agentService)(nil)
+var (
+	elog debug.Log
+	_    svc.Handler = (*agentService)(nil)
+)
 
 const serviceName = "viam-agent"
 
@@ -27,10 +30,10 @@ func (*agentService) Execute(args []string, r <-chan svc.ChangeRequest, changes
 	for {
 		c := <-r
 		if c.Cmd == svc.Stop || c.Cmd == svc.Shutdown {
-			elog.Info(1, fmt.Sprintf("%s service stopping", serviceName))
+			goutils.UncheckedError(elog.Info(1, fmt.Sprintf("%s service stopping", serviceName)))
 			break
 		} else {
-			elog.Error(1, fmt.Sprintf("unexpected control request #%d", c))
+			goutils.UncheckedError(elog.Error(1, fmt.Sprintf("unexpected control request #%d", c)))
 		}
 	}
 	changes <- svc.Status{State: svc.StopPending}
@@ -41,7 +44,7 @@ func main() {
 	if inService, err := svc.IsWindowsService(); err != nil {
 		panic(err)
 	} else if !inService {
-		println("no service detected -- running as normal process")
+		globalLogger.Info("no service detected -- running as normal process")
 		commonMain()
 		return
 	}
@@ -51,29 +54,31 @@ func main() {
 	if err != nil {
 		return
 	}
-	defer elog.Close()
+	defer func() {
+		goutils.UncheckedError(elog.Close())
+	}()
 
-	elog.Info(1, fmt.Sprintf("starting %s service", serviceName))
+	goutils.UncheckedError(elog.Info(1, fmt.Sprintf("starting %s service", serviceName)))
 	go commonMain()
 	// note: svc.Run hangs until windows terminates the service. Then we manually call
 	// globalCancel, which stops the go commonMain goroutine, then we wait for the waitgroup.
 	err = svc.Run(serviceName, &agentService{})
 	if err != nil {
-		elog.Error(1, fmt.Sprintf("%s service failed: %v", serviceName, err))
+		goutils.UncheckedError(elog.Error(1, fmt.Sprintf("%s service failed: %v", serviceName, err)))
 		return
 	}
 	if globalCancel == nil {
-		elog.Error(1, "globalCancel is nil, shutdown will be unclean")
+		goutils.UncheckedError(elog.Error(1, "globalCancel is nil, shutdown will be unclean"))
 	} else {
 		globalCancel()
 	}
 	// wait first so viam-server doesn't try to restart
 	activeBackgroundWorkers.Wait()
 	// KillTree to catch any stragglers
 	if err := utils.KillTree(-1); err != nil {
-		elog.Error(1, fmt.Sprintf("error killing subtree %s", err))
+		goutils.UncheckedError(elog.Error(1, fmt.Sprintf("error killing subtree %s", err)))
 	}
-	elog.Info(1, fmt.Sprintf("%s service stopped", serviceName))
+	goutils.UncheckedError(elog.Info(1, fmt.Sprintf("%s service stopped", serviceName)))
 }
 
 func ignoredSignal(_ os.Signal) bool {

utils/utils_windows.go

@@ -10,6 +10,7 @@ import (
 	"syscall"
 
 	"go.viam.com/rdk/logging"
+	goutils "go.viam.com/utils"
 	"golang.org/x/sys/windows/svc/eventlog"
 )
 
@@ -28,7 +29,9 @@ func SyncFS(syncPath string) error {
 	if err != nil {
 		return err
 	}
-	defer syscall.CloseHandle(handle)
+	defer func() {
+		goutils.UncheckedError(syscall.CloseHandle(handle))
+	}()
 	err = syscall.Fsync(handle)
 	if err != nil {
 		return err
@@ -38,21 +41,26 @@ func SyncFS(syncPath string) error {
 
 // KillTree kills the process tree on windows (because other signaling doesn't work).
 func KillTree(pid int) error {
-	elog, _ := eventlog.Open("viam-agent")
-	// note: we're ignoring the log error because we want this to work
+	elog, err := eventlog.Open("viam-agent")
+	if err != nil {
+		// Check error but continue since we want this to work
+		elog = nil
+	}
+
 	if pid == -1 {
 		pid = os.Getpid()
 	}
+
+	// Use a fixed command string to prevent injection
+	//nolint:gosec // WMIC.exe is a fixed command
 	cmd := exec.Command("WMIC.exe", "process", "where", fmt.Sprintf("ParentProcessId=%d", pid), "get", "ProcessId")
 	output, err := cmd.Output()
 	if err != nil {
 		return err
-		// elog.Error(1, fmt.Sprintf("error executing %s %s", cmd.Path, cmd.Args))
-		// elog.Error(1, fmt.Sprintf("error getting child process for #%d, #%s", pid, err))
 	}
 	lines := strings.Split(string(output), "\r\n")
 	if elog != nil {
-		elog.Info(1, fmt.Sprintf("KillTree stopping %d children of pid %d", len(lines), pid))
+		goutils.UncheckedError(elog.Info(1, fmt.Sprintf("KillTree stopping %d children of pid %d", len(lines), pid)))
 	}
 	for _, line := range lines[1:] {
 		if line == "" {
@@ -62,22 +70,24 @@ func KillTree(pid int) error {
 		_, err := fmt.Sscan(line, &childPID)
 		if err != nil {
 			if elog != nil {
-				elog.Error(1, fmt.Sprintf("not a valid childProcess line %q, #%s", line, err))
+				goutils.UncheckedError(elog.Error(1, fmt.Sprintf("not a valid childProcess line %q, #%s", line, err)))
 			}
 			continue
 		}
+
+		//nolint:gosec // taskkill is a fixed command
 		cmd = exec.Command("taskkill", "/F", "/T", "/PID", strconv.Itoa(childPID))
 		err = cmd.Run()
 		if elog != nil {
 			if err != nil {
-				elog.Error(1, fmt.Sprintf("error running taskkill pid %d: #%s", childPID, err))
+				goutils.UncheckedError(elog.Error(1, fmt.Sprintf("error running taskkill pid %d: #%s", childPID, err)))
 			} else {
-				elog.Info(1, fmt.Sprintf("killed pid %d", childPID))
+				goutils.UncheckedError(elog.Info(1, fmt.Sprintf("killed pid %d", childPID)))
 			}
 		}
 	}
 	if elog != nil {
-		elog.Info(1, "KillTree finished")
+		goutils.UncheckedError(elog.Info(1, "KillTree finished"))
 	}
 	return nil
 }