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.

Sign up for GitHub

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

Jump to bottom

Fix panic that could occur after a listener fails to startup #8174

Merged
merged 1 commit into from
Jan 17, 2020
Merged

Fix panic that could occur after a listener fails to startup #8174

merged 1 commit into from
Jan 17, 2020

Conversation

briankassouf
Copy link
Contributor

Fixes a panic that was introduced by #8173. It can occur after a TCP listener fails to startup:

2020-01-17T02:09:23.827Z [INFO]  core.cluster-listener.tcp: starting listener: listener_address=127.0.0.1:35767
2020-01-17T02:09:23.827Z [ERROR] core.cluster-listener.tcp: error starting listener: error="listen tcp 127.0.0.1:35767: bind: address already in use"
2020-01-17T02:09:23.827Z [INFO]  TestBackend_E2E_Initialize: cleaning up vault cluster
--- FAIL: TestBackend_E2E_Initialize (3.06s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x764082]

goroutine 8 [running]:
testing.tRunner.func1(0xc000228400)
	/usr/local/go/src/testing/testing.go:830 +0x392
panic(0x151f8a0, 0x285aaf0)
	/usr/local/go/src/runtime/panic.go:522 +0x1b5
crypto/tls.(*listener).Addr(0xc0004d0dc0, 0xc0004d0d01, 0x0)
	<autogenerated>:1 +0x32
github.com/hashicorp/vault/vault/cluster.(*Listener).Run(0xc000226100, 0x1ac6140, 0xc0000a8010, 0x1ada280, 0xc0002969c0)
	/go/src/github.com/hashicorp/vault/vault/cluster/cluster.go:257 +0x2fb
github.com/hashicorp/vault/vault.(*Core).startClusterListener(0xc00024fb80, 0x1ac6140, 0xc0000a8010, 0xc0000a49f0, 0x20)
	/go/src/github.com/hashicorp/vault/vault/cluster.go:312 +0x4df
github.com/hashicorp/vault/vault.(*Core).unsealInternal(0xc00024fb80, 0x1ac6140, 0xc0000a8010, 0xc0000a49f0, 0x20, 0x21, 0x0, 0x0, 0x0)
	/go/src/github.com/hashicorp/vault/vault/core.go:1372 +0x121
github.com/hashicorp/vault/vault.(*Core).unseal(0xc00024fb80, 0xc0000a4990, 0x21, 0x21, 0x187ce00, 0x27d7900, 0x0, 0x0)
	/go/src/github.com/hashicorp/vault/vault/core.go:1056 +0x5e7
github.com/hashicorp/vault/vault.(*Core).Unseal(...)
	/go/src/github.com/hashicorp/vault/vault/core.go:966
github.com/hashicorp/vault/vault.(*TestCluster).UnsealCoresWithError.func1(0xc00024fb80, 0xc000347580, 0xc0000cbe28)
	/go/src/github.com/hashicorp/vault/vault/testing.go:842 +0xdf
github.com/hashicorp/vault/vault.(*TestCluster).UnsealCoresWithError(0xc00022cfc0, 0xc000010000, 0x0, 0x0)
	/go/src/github.com/hashicorp/vault/vault/testing.go:855 +0xa9
github.com/hashicorp/vault/vault.(*TestCluster).UnsealCores(0xc00022cfc0, 0x1ad8e00, 0xc000228400)
	/go/src/github.com/hashicorp/vault/vault/testing.go:834 +0x48
github.com/hashicorp/vault/builtin/credential/aws.TestBackend_E2E_Initialize(0xc000228400)
	/go/src/github.com/hashicorp/vault/builtin/credential/aws/backend_e2e_test.go:89 +0x94e
testing.tRunner(0xc000228400, 0x17b97c0)
	/usr/local/go/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:916 +0x35a
FAIL	github.com/hashicorp/vault/builtin/credential/aws	3.075s

@briankassouf briankassouf added this to the 1.4 milestone Jan 17, 2020
@briankassouf briankassouf changed the title Fix panic that could occur listener fails to startup Fix panic that could occur after a listener fails to startup Jan 17, 2020
michelvocks
michelvocks reviewed Jan 17, 2020
View reviewed changes
Copy link
Contributor

@michelvocks michelvocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you maybe explain a bit more why this panic is happening? It seems like the panic is happening on https://github.com/hashicorp/vault/blob/master/vault/cluster/cluster.go#L257 which seems to not reach your fix. What am I missing here?

@ncabatoff
Copy link
Collaborator

Could you maybe explain a bit more why this panic is happening? It seems like the panic is happening on https://github.com/hashicorp/vault/blob/master/vault/cluster/cluster.go#L257 which seems to not reach your fix. What am I missing here?

With the existing code, TCPLayer.listeners may contain nil elements, so when we iterate over that slice in the loop you link to, ln will be nil, and thus tlsLn.Addr() will panic. He's changed that so listeners contains no nil values.

michelvocks
michelvocks approved these changes Jan 17, 2020
View reviewed changes
Copy link
Contributor

@michelvocks michelvocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Thanks for the explanation, @ncabatoff!

@briankassouf briankassouf merged commit 981a856 into master Jan 17, 2020
@briankassouf briankassouf deleted the tcp-listener-panic branch January 17, 2020 17:07
catsby added a commit that referenced this pull request Jan 21, 2020
@catsby
Merge branch 'master' into b-website-fix-k8s-config-link
78ac661 
* master: (34 commits)
  Use Shamir as KeK when migrating from auto-seal to shamir (#8172)
  changelog++
  ssh backend: support at character in role name (#8038)
  Fix typo in YAML markup (#8194)
  Fix typo (#8192)
  Fix k8s injector examples (#8179)
  update dependencies, patch nextjs config (#8184)
  Strip unnecessary payload in AD root cred rotation example (#8160)
  New Website! (#8154)
  Update CHANGELOG.md
  changelog++
  Fix panic when listener fails to startup (#8174)
  Create network layer abstraction to allow in-memory cluster traffic (#8173)
  Update test var name and tidy
  Factor out mysqlhelper so we can create mysql docker containers in other tests. (#8167)
  changelog++
  changelog++
  Pull wrapping creation to a var (#8137)
  ldap, okta: fix renewal when login policies are empty (#8072)
  Update CHANGELOG.md
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncabatoff ncabatoff ncabatoff approved these changes

@michelvocks michelvocks michelvocks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.4
Development

Successfully merging this pull request may close these issues.
3 participants
@briankassouf @ncabatoff @michelvocks