panic in windowsKeychain.Get

[0x53A]

ref kopia/kopia#732

I got a panic in Kopia, which uses go-keyring:

> "kopia.exe" snapshot create --description "abcdefg" "C:\some\path"
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x38 pc=0x136cc7a]
goroutine 1 [running]:
github.com/zalando/go-keyring.windowsKeychain.Get(0xc000167860, 0x22, 0xc00042f2ee, 0x8, 0x1a4eba0, 0xc00042f2e0, 0x1a3b740, 0xc00050f880)
	/home/travis/gopath/pkg/mod/github.com/zalando/go-keyring@v0.1.0/keyring_windows.go:20 +0x11a
github.com/zalando/go-keyring.Get(...)
	/home/travis/gopath/pkg/mod/github.com/zalando/go-keyring@v0.1.0/keyring.go:32
github.com/kopia/kopia/repo.GetPersistedPassword(0x1d8fbc0, 0xc000534120, 0xc0004be200, 0x39, 0xc00050f7e0, 0x0, 0xc000534150)
	/home/travis/gopath/src/github.com/kopia/kopia/repo/password.go:26 +0x375
github.com/kopia/kopia/cli.getPasswordFromFlags(0x1d8fbc0, 0xc000534120, 0x1d70100, 0xc0000720d0, 0x0, 0x0, 0x1a4cd60)
	/home/travis/gopath/src/github.com/kopia/kopia/cli/password.go:56 +0x145
github.com/kopia/kopia/cli.openRepository(0x1d8fbc0, 0xc000534120, 0x0, 0x1, 0x0, 0x0, 0x0, 0x0)
	/home/travis/gopath/src/github.com/kopia/kopia/cli/config.go:60 +0x165
github.com/kopia/kopia/cli.maybeRepositoryAction.func1.1(0x0, 0x0)
	/home/travis/gopath/src/github.com/kopia/kopia/cli/app.go:141 +0xf0
github.com/kopia/kopia/cli.withProfiling(...)
	/home/travis/gopath/src/github.com/kopia/kopia/cli/profile_disabled.go:7
github.com/kopia/kopia/cli.maybeRepositoryAction.func1(0xc0001a2d80, 0xe8bcaa, 0x1ac45e0)
	/home/travis/gopath/src/github.com/kopia/kopia/cli/app.go:125 +0x64
gopkg.in/alecthomas/kingpin%2ev2.(*actionMixin).applyActions(0xc0004f9518, 0xc0001a2d80, 0x0, 0x0)
	/home/travis/gopath/pkg/mod/gopkg.in/alecthomas/kingpin.v2@v2.2.6/actions.go:28 +0x74
gopkg.in/alecthomas/kingpin%2ev2.(*Application).applyActions(0xc000152870, 0xc0001a2d80, 0x0, 0x0)
	/home/travis/gopath/pkg/mod/gopkg.in/alecthomas/kingpin.v2@v2.2.6/app.go:557 +0xe3
gopkg.in/alecthomas/kingpin%2ev2.(*Application).execute(0xc000152870, 0xc0001a2d80, 0xc00050f620, 0x2, 0x2, 0x0, 0x0, 0x0, 0xc000007dd8)
	/home/travis/gopath/pkg/mod/gopkg.in/alecthomas/kingpin.v2@v2.2.6/app.go:390 +0xa5
gopkg.in/alecthomas/kingpin%2ev2.(*Application).Parse(0xc000152870, 0xc000130010, 0x5, 0x7, 0x1, 0xc000007de0, 0x0, 0x1)
	/home/travis/gopath/pkg/mod/gopkg.in/alecthomas/kingpin.v2@v2.2.6/app.go:222 +0x213
main.main()
	/home/travis/gopath/src/github.com/kopia/kopia/main.go:77 +0x188

It crashes here in line 20:

go-keyring/keyring_windows.go

Lines 11 to 20 in 6905df4

	func (k windowsKeychain) Get(service, username string) (string, error) {
	cred, err := wincred.GetGenericCredential(k.credName(service, username))
	if err != nil {
	if err == syscall.ERROR_NOT_FOUND {
	return "", ErrNotFound
	}
	return "", err
	}
	return string(cred.CredentialBlob), nil

Now, I don't know much about golang, but one thing that might be related is that go-keyring checks whether err != nil, whereas wincred checks if cred != nil.

In case err was nil and ret was 0 here https://github.com/danieljoos/wincred/blob/78f93c1f8b99b0c2f6e7f3d2bdc4993cf87bddff/sys.go#L70, I believe that it would cause nil, nil to be returned.

Another way nil, nil might be returned was if cred is null:
https://github.com/danieljoos/wincred/blob/78f93c1f8b99b0c2f6e7f3d2bdc4993cf87bddff/conversion.go#L91-L94

See danieljoos/wincred#5 (comment).

So I'd change the check from if err != nil to if cred == nil.
That wouldn't solve the underlying issue, but might convert the panic into a normal, handleable error.

---

Update:

I took a look at the logs, and, running on two different computers, I had two errors out of 290 runs (0.69%).
Each time it was executing a bunch of times successfully in sequence and then failed.

Ref danieljoos/wincred#5 (comment), the CreadRead API seems to be a bit flakey, do you think it might make sense to check if cred, err are nil, nil, and then try a second time, or would that be better handled at the calling application (Kopia in this case)?

---

Update 2:

4 failures in 439 runs (0.91%)

[mislav]

This is being investigated in danieljoos/wincred#32

[williambrode]

Thanks @mislav for the update, I just hit this issue too!