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

Protect GO_EVP_CIPHER_CTX_PTR with sync.Mutex #145

Closed
wants to merge 1 commit into from

Conversation

lyoung-confluent
Copy link

@lyoung-confluent lyoung-confluent commented May 10, 2024

See golang-fips/go#187 for details on why this is necessary. Protecting these with a mutex is a brute-force approach and presumably has negative performance implications, another option could be to go back to creating a GO_EVP_CIPHER_CTX_PTR within each encrypt/decrypt operation instead of caching the context.

@derekparker
Copy link
Contributor

Thanks for sending this fix!

There's definitely going to be a performance hit when trying to run these operations in parallel. My gut instinct is instead of using this mutex we instead create a new EVP_CIPHER_CTX for each operation instead of reusing, via newCipherCtx and don't store it in any of the structs such as evpCipher. This has the drawback that the caller will have to free that context to avoid leaks, but that's ok. We can also wrap the EVP_CIPHER_CTX in a struct with a finalizer to free the memory when that object is GC'd in the case of a programmer error where free is not called and it starts to leak.

cc @dagood @qmuntal @ueno @simo5

@derekparker derekparker requested review from qmuntal, dagood, ueno and simo5 May 10, 2024 17:01
@derekparker
Copy link
Contributor

Closing in favor of #146.

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.

2 participants