Issues found by static analysis

[joonas-fi]

Tasks:

• Send PR for most clear issues
• Have the above PR be merged
• Agree on a lint rule set
• Fix the rest of the issues found by agreed-on lint rule set
• Set up GitHub action so there will be no new regressions

I ran golangci-lint, with configuration I've settled on for my projects. Here's the current issues it found:

Lint report

pkg/rsyncd/rsyncd.go:76:6: `sumBuf` is unused (deadcode)
type sumBuf struct {
     ^
pkg/maincmd/unprivileged_linux.go:10:6: `runAsUnprivilegedUser` is unused (deadcode)
func runAsUnprivilegedUser(cmd *exec.Cmd) {
     ^
pkg/rsyncchecksum/rsyncchecksum.go:52:14: Error return value of `binary.Write` is not checked (errcheck)
	binary.Write(h, binary.LittleEndian, seed)
	            ^
pkg/rsyncwire/wire.go:96:14: Error return value of `binary.Write` is not checked (errcheck)
	binary.Write(&b.buf, binary.LittleEndian, data)
	            ^
pkg/rsyncwire/wire.go:100:14: Error return value of `binary.Write` is not checked (errcheck)
	binary.Write(&b.buf, binary.LittleEndian, data)
	            ^
pkg/rsyncwire/wire.go:115:16: Error return value of `io.WriteString` is not checked (errcheck)
	io.WriteString(&b.buf, data)
	              ^
pkg/anonssh/anonssh.go:166:14: Error return value of `req.Reply` is not checked (errcheck)
				req.Reply(false, errmsg)
				         ^
pkg/anonssh/anonssh.go:167:18: Error return value of `channel.Write` is not checked (errcheck)
				channel.Write(errmsg)
				             ^
pkg/anonssh/anonssh.go:180:17: Error return value of `newChan.Reject` is not checked (errcheck)
		newChan.Reject(ssh.UnknownChannelType, fmt.Sprintf("unknown channel type: %q", t))
		              ^
pkg/receivermaincmd/receiver.go:98:19: Error return value of `out.Cleanup` is not checked (errcheck)
	defer out.Cleanup()
	                 ^
pkg/rsyncd/rsyncd.go:198:17: Error return value of `io.WriteString` is not checked (errcheck)
		io.WriteString(cwr, s.formatModuleList())
		              ^
pkg/rsyncd/rsyncd.go:199:17: Error return value of `io.WriteString` is not checked (errcheck)
		io.WriteString(cwr, "@RSYNCD: EXIT\n")
		              ^
pkg/rsyncd/rsyncd.go:253:15: Error return value of `mpx.WriteMsg` is not checked (errcheck)
		mpx.WriteMsg(rsyncwire.MsgError, []byte(fmt.Sprintf("gokr-rsync [sender]: %v\n", err)))
		            ^
pkg/rsyncd/rsyncd.go:312:16: Error return value of `mpx.WriteMsg` is not checked (errcheck)
			mpx.WriteMsg(rsyncwire.MsgError, []byte(fmt.Sprintf("gokr-rsync [sender]: %v\n", err)))
			            ^
pkg/rsynctest/rsynctest.go:109:15: Error return value of `srv.Serve` is not checked (errcheck)
		go srv.Serve(context.Background(), ts.listener)
		            ^
pkg/anonssh/anonssh.go:60:3: commentFormatting: put a space between `//` and comment text (gocritic)
		//s.env = append(s.env, fmt.Sprintf("%s=%s", r.VariableName, r.VariableValue))
		^
pkg/receivermaincmd/receivermaincmd.go:169:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
		} else {
		       ^
pkg/rsyncd/match.go:126:6: commentFormatting: put a space between `//` and comment text (gocritic)
					//falseAlarms++
					^
pkg/rsyncd/match.go:181:3: commentFormatting: put a space between `//` and comment text (gocritic)
		//log.Printf("null_tag, k=%d", k)
		^
cmd/gokr-rsyncd/rsyncd.go:25:3: exitAfterDefer: log.Fatal will exit, and `defer cancel()` will not run (gocritic)
		log.Fatal(err)
		^
cmd/libuser/main.go:23:3: exitAfterDefer: log.Fatal will exit, and `defer cancel()` will not run (gocritic)
		log.Fatal(err)
		^
pkg/rsyncchecksum/checksum_test.go:28:12: G306: Expect WriteFile permissions to be 0600 or less (gosec)
	if err := ioutil.WriteFile(large, content, 0644); err != nil {
	          ^
pkg/receivermaincmd/receivermaincmd.go:277:9: G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
	ssh := exec.Command(args[0], args[1:]...)
	       ^
pkg/maincmd/maincmd.go:18:2: G108: Profiling endpoint is automatically exposed on /debug/pprof (gosec)
	_ "net/http/pprof"
	^
pkg/maincmd/writetest.go:12:12: G306: Expect WriteFile permissions to be 0600 or less (gosec)
	if err := ioutil.WriteFile(fn, []byte("gokr-rsyncd creates this file to prevent misconfigurations. if you see this file, it means gokr-rsyncd unexpectedly was started with too many privileges"), 0644); err == nil {
	          ^
pkg/rsynctest/rsynctest.go:251:12: G306: Expect WriteFile permissions to be 0600 or less (gosec)
	if err := ioutil.WriteFile(large, content, 0644); err != nil {
	          ^
pkg/anonssh/anonssh.go:43:2: `env` is unused (structcheck)
	env     []string
	^
pkg/anonssh/anonssh.go:44:2: `ptyf` is unused (structcheck)
	ptyf    *os.File
	^
pkg/anonssh/anonssh.go:45:2: `ttyf` is unused (structcheck)
	ttyf    *os.File
	^
pkg/anonssh/anonssh.go:27:2: `cfg` is unused (structcheck)
	cfg  *config.Config
	^
pkg/rsyncd/rsyncd.go:80:2: `sum1` is unused (structcheck)
	sum1   uint32
	^
pkg/rsyncd/rsyncd.go:81:2: `sum2` is unused (structcheck)
	sum2   [16]byte
	^
pkg/rsyncd/rsyncd.go:77:2: `offset` is unused (structcheck)
	offset int64
	^
pkg/rsyncd/rsyncd.go:78:2: `len` is unused (structcheck)
	len    int64
	^
pkg/rsyncd/rsyncd.go:79:2: `index` is unused (structcheck)
	index  int32
	^
pkg/rsyncd/token.go:39:36: unnecessary conversion (unconvert)
		return st.conn.WriteInt32(-(int32(token) + 1))
		                                 ^
pkg/anonssh/anonssh.go:50:27: `(*session).request` - `ctx` is unused (unparam)
func (s *session) request(ctx context.Context, req *ssh.Request) error {
                          ^
pkg/receivermaincmd/generator.go:46:66: (*recvTransfer).skipFile - result 1 (error) is always nil (unparam)
func (rt *recvTransfer) skipFile(f *file, st os.FileInfo) (bool, error) {
                                                                 ^
pkg/receivermaincmd/receivermaincmd.go:239:12: `doCmd` - `osenv` is unused (unparam)
func doCmd(osenv osenv, opts *Opts, machine, user, path string, daemonConnection int) (io.ReadCloser, io.WriteCloser, error) {
           ^
pkg/rsyncd/flist.go:17:38: `(*sendTransfer).sendFileList` - `c` is unused (unparam)
func (st *sendTransfer) sendFileList(c *rsyncwire.Conn, mod config.Module, opts *Opts, paths []string) (*fileList, error) {
                                     ^
pkg/rsyncd/token.go:45:79: `(*sendTransfer).sendToken` - `toklen` is unused (unparam)
func (st *sendTransfer) sendToken(f *os.File, i int32, offset int64, n int64, toklen int64) error {
                                                                              ^
pkg/rsyncd/match.go:160:5: unreachable: unreachable code (govet)
				offset += head.Sums[i].Len - 1
				^
pkg/rsyncd/match.go:182:3: unreachable: unreachable code (govet)
		readk := k + 1
		^
pkg/rsyncchecksum/rsyncchecksum.go:26:2: variable len has same name as predeclared identifier (predeclared)
	len := len(buf)
	^
pkg/rsynccommon/rsynccommon.go:14:21: param len has same name as predeclared identifier (predeclared)
func SumSizesSqroot(len int64) rsync.SumHead {
                    ^
pkg/receivermaincmd/generator.go:263:58: param len has same name as predeclared identifier (predeclared)
func (rt *recvTransfer) generateAndSendSums(in *os.File, len int64) error {
                                                         ^
pkg/receivermaincmd/receiver.go:124:3: variable len has same name as predeclared identifier (predeclared)
		len := sh.BlockLength
		^
pkg/maincmd/maincmd.go:44:3: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)
		fmt.Fprintf(stderr, opt.Help())
		^
pkg/receivermaincmd/generator.go:182:2: SA9003: empty branch (staticcheck)
	if rt.opts.PreserveHardlinks {
	^
pkg/receivermaincmd/generator.go:89:2: SA4006: this value of `st` is never used (staticcheck)
	st, err = rt.setUid(f, local, st)
	^

Some of these are non-issues, but some seem clear bugs like this one:

pkg/maincmd/maincmd.go:44:3: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)
		fmt.Fprintf(stderr, opt.Help())

Would you like me to submit a PR to fix all or some of these complaints?

[stapelberg]

The unused and predeclared are too aggressive, I think.

For the others, can you send a PR to fix (or exempt them where appropriate) them all and set up this static analyzer in GitHub Actions so that the code doesn’t regress please?

[joonas-fi]

Sure, no problem. But I'd like to get the previous stuff resolved first in #12

(Too much parallel work brings overhead due to expected merge conflicts that the "internal -> something else" rename will bring)

[joonas-fi]

I updated the issue text to contain list of tasks.

[joonas-fi]

There seems to be ready GitHub action one can use: https://github.com/golangci/golangci-lint-action