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

generateKeyPairAsync is not thread-safe #59

Closed
ospfranco opened this issue Jun 24, 2022 · 0 comments · Fixed by #304
Closed

generateKeyPairAsync is not thread-safe #59

ospfranco opened this issue Jun 24, 2022 · 0 comments · Fixed by #304
Labels
enhancement New feature or request

Comments

@ospfranco
Copy link
Collaborator

ospfranco commented Jun 24, 2022

When parsing/creating encryption keys there is a variable amount of parameters needed depending on the selected scheme (rsa, rsapps, etc). Node implements the argument parsing by reading directly from the passed arguments from JS, this is also necessary because some data needs to be read/passed/transformed by OpenSSL (BIOPointers and some EVP functions) before actually generating/reading the key.

This is however problematic when using the JSI, because we need to pass the runtime to the threaded implementation. Accessing the runtime from a std::thread can cause crashes. The solution is clear, we need to read all the arguments before doing threaded work. This however is a big big big change, since the number of arguments and their types will change depending on the scheme, we basically need to duplicate/de-attach a lot of the logic and then re-apply it as pure c++ values when doing the key parsing. Basically a from-the-ground up implementation.

One possible path is reading the data from JS without a thread and only doing the key generation step in a thread, here however not sure how much performance we would lose, but it wouldn't require a major re-write. This should be explored first.

I already faced some trouble with race conditions accessing data, I have changed the data parsed to occur before spawning the thread, this seems to work fine for now. But we still need to keep an eye open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants