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 race in PKI's runUnifiedTransfer #20701

Merged
merged 2 commits into from
May 22, 2023

Conversation

cipherboy
Copy link
Contributor

During this race, we'll sometimes start (or fail to start) an additional unified transfer if the updated last run timestamp was written at the same time as another thread was reading it.

Instead, delay this check until we're holding the CAS guard; this will occasionally result in more messages saying that an existing process is already running, but otherwise shouldn't impact the functionality at all.

This manifests in the race detector as:

2023-05-22T15:38:20.0251871Z WARNING: DATA RACE
2023-05-22T15:38:20.0252056Z Read at 0x00c003f633b8 by goroutine 33457:
2023-05-22T15:38:20.0252340Z   github.com/hashicorp/vault/builtin/logical/pki.runUnifiedTransfer()
2023-05-22T15:38:20.0252973Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/builtin/logical/pki/periodic.go:53 +0x387
2023-05-22T15:38:20.0253287Z   github.com/hashicorp/vault/builtin/logical/pki.(*backend).periodicFunc.func3()
2023-05-22T15:38:20.0253908Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/builtin/logical/pki/backend.go:606 +0x39
2023-05-22T15:38:20.0254605Z 2023-05-22T15:37:25.778Z [DEBUG] TestRevocationQueue.perf-sec2.core0.secrets.pki.pki_e7ac3c69: starting to process unified revocations
2023-05-22T15:38:20.0254620Z 
2023-05-22T15:38:20.0254824Z Previous write at 0x00c003f633b8 by goroutine 33365:
2023-05-22T15:38:20.0255102Z   github.com/hashicorp/vault/builtin/logical/pki.runUnifiedTransfer()
2023-05-22T15:38:20.0255718Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/builtin/logical/pki/periodic.go:101 +0xbed
2023-05-22T15:38:20.0256034Z   github.com/hashicorp/vault/builtin/logical/pki.(*backend).periodicFunc.func3()
2023-05-22T15:38:20.0256647Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/builtin/logical/pki/backend.go:606 +0x39
2023-05-22T15:38:20.0256656Z 
2023-05-22T15:38:20.0256812Z Goroutine 33457 (running) created at:
2023-05-22T15:38:20.0257111Z   github.com/hashicorp/vault/builtin/logical/pki.(*backend).periodicFunc()
2023-05-22T15:38:20.0257731Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/builtin/logical/pki/backend.go:606 +0x2e5
2023-05-22T15:38:20.0258112Z   github.com/hashicorp/vault/builtin/logical/pki.(*backend).periodicFunc-fm()
2023-05-22T15:38:20.0258299Z       <autogenerated>:1 +0x64
2023-05-22T15:38:20.0258593Z   github.com/hashicorp/vault/sdk/framework.(*Backend).handleRollback()
2023-05-22T15:38:20.0259189Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/sdk/framework/backend.go:609 +0xaa
2023-05-22T15:38:20.0259460Z   github.com/hashicorp/vault/sdk/framework.(*Backend).HandleRequest()
2023-05-22T15:38:20.0260062Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/sdk/framework/backend.go:206 +0x1a4
2023-05-22T15:38:20.0260585Z 2023-05-22T15:37:25.778Z [DEBUG] TestRevocationQueue.perf-sec2.core0.secrets.pki.pki_e7ac3c69: gathered 0 unified revocations entries
2023-05-22T15:38:20.0260867Z   github.com/hashicorp/vault/builtin/logical/pki.(*backend).HandleRequest()
2023-05-22T15:38:20.0261067Z       <autogenerated>:1 +0x6e
2023-05-22T15:38:20.0261316Z   github.com/hashicorp/vault/vault.(*Router).routeCommon()
2023-05-22T15:38:20.0261878Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/vault/router.go:764 +0x2005
2023-05-22T15:38:20.0262116Z   github.com/hashicorp/vault/vault.(*Router).Route()
2023-05-22T15:38:20.0262660Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/vault/router.go:544 +0xa9d
2023-05-22T15:38:20.0262940Z   github.com/hashicorp/vault/vault.(*RollbackManager).attemptRollback()
2023-05-22T15:38:20.0263519Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/vault/rollback.go:245 +0xa5d
2023-05-22T15:38:20.0263850Z   github.com/hashicorp/vault/vault.(*RollbackManager).startOrLookupRollback.func2()
2023-05-22T15:38:20.0264415Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/vault/rollback.go:179 +0x9b
2023-05-22T15:38:20.0264424Z 
2023-05-22T15:38:20.0264585Z Goroutine 33365 (finished) created at:
2023-05-22T15:38:20.0264892Z   github.com/hashicorp/vault/builtin/logical/pki.(*backend).periodicFunc()
2023-05-22T15:38:20.0265501Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/builtin/logical/pki/backend.go:606 +0x2e5
2023-05-22T15:38:20.0265874Z   github.com/hashicorp/vault/builtin/logical/pki.(*backend).periodicFunc-fm()
2023-05-22T15:38:20.0266262Z       <autogenerated>:1 +0x64
2023-05-22T15:38:20.0266538Z   github.com/hashicorp/vault/sdk/framework.(*Backend).handleRollback()
2023-05-22T15:38:20.0267129Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/sdk/framework/backend.go:609 +0xaa
2023-05-22T15:38:20.0267421Z   github.com/hashicorp/vault/sdk/framework.(*Backend).HandleRequest()
2023-05-22T15:38:20.0268004Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/sdk/framework/backend.go:206 +0x1a4
2023-05-22T15:38:20.0268285Z   github.com/hashicorp/vault/builtin/logical/pki.(*backend).HandleRequest()
2023-05-22T15:38:20.0268588Z       <autogenerated>:1 +0x6e
2023-05-22T15:38:20.0268913Z   github.com/hashicorp/vault/vault.(*Router).routeCommon()
2023-05-22T15:38:20.0269469Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/vault/router.go:764 +0x2005
2023-05-22T15:38:20.0269710Z   github.com/hashicorp/vault/vault.(*Router).Route()
2023-05-22T15:38:20.0270265Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/vault/router.go:544 +0xa9d
2023-05-22T15:38:20.0270546Z   github.com/hashicorp/vault/vault.(*RollbackManager).attemptRollback()
2023-05-22T15:38:20.0271106Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/vault/rollback.go:245 +0xa5d
2023-05-22T15:38:20.0271451Z   github.com/hashicorp/vault/vault.(*RollbackManager).startOrLookupRollback.func2()
2023-05-22T15:38:20.0272009Z       /home/runner/actions-runner/_work/vault-enterprise/vault-enterprise/vault/rollback.go:179 +0x9b

During this race, we'll sometimes start (or fail to start) an additional
unified transfer if the updated last run timestamp was written at the
same time as another thread was reading it.

Instead, delay this check until we're holding the CAS guard; this will
occasionally result in more messages saying that an existing process is
already running, but otherwise shouldn't impact the functionality at
all.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@cipherboy cipherboy added bug Used to indicate a potential bug secret/pki backport/1.13.x labels May 22, 2023
@cipherboy cipherboy added this to the 1.13.3 milestone May 22, 2023
@cipherboy cipherboy requested review from kitography, stevendpclark and a team May 22, 2023 16:37
Copy link
Contributor

@kitography kitography left a comment

Choose a reason for hiding this comment

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

LGTM, comments are super helpful here.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@cipherboy cipherboy enabled auto-merge (squash) May 22, 2023 18:00
@cipherboy
Copy link
Contributor Author

Thank you! Auto-Merging with changelog entry :-)

@cipherboy cipherboy merged commit 0ac2fa1 into main May 22, 2023
@cipherboy cipherboy deleted the cipherboy-fix-race-run-unified-transfer branch May 22, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to indicate a potential bug secret/pki
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants