Bugs fixes

[TheRealMal]

This PR fixes 2 bugs:

Fixes #18085 Memory leak
Fixes #18084 Dereference of nil

[k8s-ci-robot]

Hi @TheRealMal. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[henrybear327]

/ok-to-test

[tjungblu]

it sounds like an easy fix, but how would the rename operation below work on an open file?

[tjungblu]

TIL this actually works with the rename syscall on linux. It will fail on Windows with the classic "the process cannot access the file because it is being used by another process". Which is Support Tier 3, but the fix is simple: just wrap the file operations before the rename in a function and the defer within.

[TheRealMal]

What do you mean by "rename operation"? I just moved f.Close() to defer, because file should be closed even if any error occured after it being opened. I can't see any logic changes.

[tjungblu]

This operation:

etcd/client/v3/snapshot/v3_snapshot.go

Lines 95 to 97 in 7bad55f

	if err = os.Rename(partpath, dbPath); err != nil {
	return resp.Version, fmt.Errorf("could not rename %s to %s (%v)", partpath, dbPath, err)
	}

will fail on Windows, because defer closes the file after the function exits and you can't rename a still opened file.

MRE if you want to try this out: https://gist.github.com/tjungblu/7d4ff9bc88afe86004bf292fd9032966

[TheRealMal]

Well, you are right

[tjungblu]

turned out to be slightly more verbose than I thought, but that could work:

	err = func() (err error) {
		var f *os.File
		f, err = os.OpenFile(partpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fileutil.PrivateFileMode)
		if err != nil {
			return fmt.Errorf("could not open %s (%v)", partpath, err)
		}
		defer func() {
			err = f.Close()
		}()
		
		lg.Info("created temporary db file", zap.String("path", partpath))

		var rd io.ReadCloser
		rd, err = cli.Snapshot(ctx)
		if err != nil {
			return
		}
		lg.Info("fetching snapshot", zap.String("endpoint", cfg.Endpoints[0]))
		var size int64
		size, err = io.Copy(f, rd)
		if err != nil {
			return
		}
		if !hasChecksum(size) {
			return fmt.Errorf("sha256 checksum not found [bytes: %d]", size)
		}
		if err = fileutil.Fsync(f); err != nil {
			return
		}

		lg.Info("fetched snapshot",
			zap.String("endpoint", cfg.Endpoints[0]),
			zap.String("size", humanize.Bytes(uint64(size))),
			zap.String("took", humanize.Time(now)),
		)

		return
	}()

	if err != nil {
		return err
	}

[TheRealMal]

But err = f.Close() will rewrite any error which comes after defer func() {err = f.Close()}() inside that block, won't it?

[henrybear327]

Joining the errors should work :)

Ref: https://pkg.go.dev/errors#Join

[jmhbnz]

Hi @TheRealMal - Thanks for raising these fixes. It looks like your two commits are not signed. Can you please take a look and ensure all commits are signed so the developer certificate of origin check passes? i.e.

git rebase --signoff HEAD~2

[jmhbnz]

Discussed during sig-etcd triage meeting - hey @TheRealMal do you have capacity to continue this pr?