search

zalando / go-keyring

Cross-platform keyring interface for Go
MIT License
882 stars 86 forks source link

Fix suspected race condition in prompt handling #105

Closed williammartin closed 8 months ago

williammartin commented 8 months ago

Description

Fixes https://github.com/zalando/go-keyring/issues/104

The current approach to handling prompts involves calling for a prompt and then registering a signal handler for a response that indicates the prompt had completed. In most cases, if the user were to take action, this would be quite slow. However, in the case of automatic responses to the prompt, the signal response may appear before the signal handler is configured, resulting in blocking forever.

This commit moves the call for a prompt after the signal handling has been configured, closing the race window.

Reviewer Notes

I'll admit that I'm not very familiar with all the nuances of dbus or this library so it's entirely possible I've missed something. We've had three reports over on https://github.com/cli/cli/issues/8802 that this has resolved the issue they were facing but I can't say for sure that there's not some unintended side effects.

szuecs commented 8 months ago

@williammartin thanks for the PR, the description and the detailed issue, really great work! Reviewing the timestamps of the message, I would say you are right to call it a race condition.

szuecs commented 8 months ago

:+1:

mikkeloscar commented 8 months ago

:+1:

williammartin commented 8 months ago

Thanks so much for the quick review and merge! Will you create a new release with this or should I pin to the sha in the meantime?

thanks for the PR, the description and the detailed issue, really great work!

OSS maintainers have to stick together 😉

mikkeloscar commented 8 months ago

I have created a new release! https://github.com/zalando/go-keyring/releases/tag/v0.2.4