Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crashes in keychain Get & Remove #83

Merged
merged 2 commits into from
Aug 20, 2016
Merged

Conversation

dgoodlad
Copy link
Collaborator

@dgoodlad dgoodlad commented Aug 20, 2016

These lines were causing serviceRef to be released twice. It looks like some copy/paste shenanigans from the blocks just above where I've made these changes, e.g. keychain.go:59. I think it's pretty safe to assume from the context that these lines were meant to release accountRef not serviceRef.

These were causing a crash on macOS 10.12 beta that looked like:

SIGTRAP: trace trap
PC=0x7fff86a8b985 m=5
signal arrived during cgo execution

goroutine 1 [syscall, locked to thread]:
runtime.cgocall(0x442a000, 0xc8200dfe20, 0xc800000000)
        /usr/local/Cellar/go/1.5.3/libexec/src/runtime/cgocall.go:120 +0x11b fp=0xc8200dfdf0 sp=0xc8200dfdc0
github.com/99designs/aws-vault/keyring._Cfunc_CFRelease(0x4f00000)
        ??:0 +0x31 fp=0xc8200dfe20 sp=0xc8200dfdf0
github.com/99designs/aws-vault/keyring.(*keychain).Get(0xc8200e5380, 0x7fff5fbffa77, 0xa, 0x7fff5fbffa77, 0xa, 0xc820017050, 0x87, 0x87, 0xc820
013800, 0x16, ...)
        /Users/lachlan/Projects/99designs/go/src/github.com/99designs/aws-vault/keyring/keychain.go:117 +0x97c fp=0xc8200dffb8 sp=0xc8200dfe20
main.(*KeyringProvider).Retrieve(0xc820013740, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)

This line was causing `serviceRef` to be released twice, which causes a
crash on macOS 10.12 beta.
@dgoodlad dgoodlad changed the title Fix crash in keychain Get Fix crashes in keychain Get & Remove Aug 20, 2016
@lox
Copy link
Collaborator

lox commented Aug 20, 2016

Ugh. Thanks, good catch. Should really prioritize #58. I'm waiting for upstream to merge it, but perhaps we should just use https://github.com/99designs/go-keychain.

@dgoodlad
Copy link
Collaborator Author

@lox yeah there's a lot of work that seems to happen in this repo to support the keychain module and bugs therein; I did follow #58 through to keybase/go-keychain#4 - reckon it just needs another ping and review there...

@dgoodlad dgoodlad merged commit 398f39a into master Aug 20, 2016
@dgoodlad dgoodlad deleted the fix-crash-in-keychain-get branch August 20, 2016 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants