search

dweymouth / supersonic

A lightweight and full-featured cross-platform desktop client for self-hosted music servers
GNU General Public License v3.0
670 stars 26 forks source link

Check if server is nil before passing pointer to go-keyring #322

Closed Laevos closed 5 months ago

Laevos commented 5 months ago

I'm running into an issue that appears to be related to https://github.com/zalando/go-keyring/issues/88, which I describe in https://github.com/zalando/go-keyring/issues/88#issuecomment-1914505694. The gist of the issue is that an incompatibility between go-keyring and KeepassXC causes a segfault in the SetServerPassword method as the ServerConfig isn't properly initialized.

2024/01/29 03:11:41 error getting password from keyring: org.freedesktop.Secret.Error.IsLocked
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x5e33a2]

goroutine 79 [running]:
github.com/zalando/go-keyring/secret_service.(*SecretService).handlePrompt(0xc0091ef050, {0xc000462380?, 0x2c?})
    github.com/zalando/go-keyring@v0.2.1/secret_service/secret_service.go:197 +0x122
github.com/zalando/go-keyring/secret_service.(*SecretService).CreateItem(0xd69990?, {0xe6cdb0, 0xc0089842d0}, {0xc009084780, 0x43}, 0xc00a337620, {{0xc008e7d1d0, 0x41}, {0x1b1af00, 0x0, ...}, ...})
    github.com/zalando/go-keyring@v0.2.1/secret_service/secret_service.go:175 +0x370
github.com/zalando/go-keyring.secretServiceProvider.Set({}, {0xd1127f, 0xa}, {0xc0089b61b0, 0x24}, {0xc008402c80, 0x40})
    github.com/zalando/go-keyring@v0.2.1/keyring_unix.go:43 +0x3be
github.com/zalando/go-keyring.Set(...)
    github.com/zalando/go-keyring@v0.2.1/keyring.go:27
github.com/dweymouth/supersonic/backend.(*ServerManager).SetServerPassword(0xc0000fb900, 0xc008813eb0?, {0xc008402c80, 0x40})
    github.com/dweymouth/supersonic/backend/servermanager.go:159 +0xb6
github.com/dweymouth/supersonic/ui/controller.(*Controller).trySetPasswordAndConnectToServer(0xc000582300, 0x8?, {0xc008402c80, 0x40})
    github.com/dweymouth/supersonic/ui/controller/controller.go:544 +0x3c
github.com/dweymouth/supersonic/ui/controller.(*Controller).PromptForLoginAndConnect.func1.1()
    github.com/dweymouth/supersonic/ui/controller/controller.go:376 +0x178
created by github.com/dweymouth/supersonic/ui/controller.(*Controller).PromptForLoginAndConnect.func1 in goroutine 84
    github.com/dweymouth/supersonic/ui/controller/controller.go:368 +0x105

As there's currently no ETA for the upstream issue being fixed, I propose the following solution to prevent a segfault in the event that this or any other issue crops up which causes the ServerConfig to be nil. Since trySetPasswordAndConnectToServer() can gracefully handle an error in SetServerPassword(), this seems like it should be a safe fix since we can just store the password in-memory.

Laevos commented 5 months ago

Ah, just realized this doesn't fix the issue; it's not that the ServerConfig is nil, otherwise we wouldn't even get into the keyring-go code before we hit a nil dereference, so unfortunately the fix seems like it'll have to be upstream, apologies :pensive: