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

crypto: data race in SecureContext::AddRootCerts? #45743

Closed
bnoordhuis opened this issue Dec 5, 2022 · 0 comments · Fixed by #45767
Closed

crypto: data race in SecureContext::AddRootCerts? #45743

bnoordhuis opened this issue Dec 5, 2022 · 0 comments · Fixed by #45767
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.

Comments

@bnoordhuis
Copy link
Member

Possible data race when worker_threads are active?

node/src/crypto/crypto_context.cc

Lines 749 to 751 in 1287530

if (root_cert_store == nullptr) {
root_cert_store = NewRootCertStore();
}

root_cert_store is a static X509_STORE* that is assigned to (and read from) without any kind of synchronization, AFAICT.
@bnoordhuis bnoordhuis added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Dec 5, 2022
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Dec 7, 2022
@bnoordhuis
src: fix tls certificate root store data race
ca3e55f 
OpenSSL internally synchronizes access to the X509_STORE. Creation of
the global root store in Node was not properly synchronized, however,
introducing the possibility of data races when multiple threads try to
create it concurrently.

This commit coincidentally removes the last call to the thread-unsafe
ERR_error_string() function.

Fixes: nodejs#45743
@bnoordhuis bnoordhuis mentioned this issue Dec 7, 2022
Merged
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Dec 7, 2022
@bnoordhuis
src: fix tls certificate root store data race
1eb35b3 
OpenSSL internally synchronizes access to the X509_STORE. Creation of
the global root store in Node was not properly synchronized, however,
introducing the possibility of data races when multiple threads try to
create it concurrently.

This commit coincidentally removes the last call to the thread-unsafe
ERR_error_string() function.

Fixes: nodejs#45743
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Dec 8, 2022
@bnoordhuis
src: fix tls certificate root store data race
99e4a76 
OpenSSL internally synchronizes access to the X509_STORE. Creation of
the global root store in Node was not properly synchronized, however,
introducing the possibility of data races when multiple threads try to
create it concurrently.

This commit coincidentally removes the last call to the thread-unsafe
ERR_error_string() function.

Fixes: nodejs#45743
nodejs-github-bot pushed a commit that referenced this issue Dec 19, 2022
@bnoordhuis
src: fix tls certificate root store data race
273d869 
OpenSSL internally synchronizes access to the X509_STORE. Creation of
the global root store in Node was not properly synchronized, however,
introducing the possibility of data races when multiple threads try to
create it concurrently.

This commit coincidentally removes the last call to the thread-unsafe
ERR_error_string() function.

Fixes: #45743
PR-URL: #45767
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this issue Jan 1, 2023
@bnoordhuis @targos
src: fix tls certificate root store data race
d40419e 
OpenSSL internally synchronizes access to the X509_STORE. Creation of
the global root store in Node was not properly synchronized, however,
introducing the possibility of data races when multiple threads try to
create it concurrently.

This commit coincidentally removes the last call to the thread-unsafe
ERR_error_string() function.

Fixes: #45743
PR-URL: #45767
Reviewed-By: Anna Henningsen <anna@addaleax.net>
RafaelGSS pushed a commit that referenced this issue Jan 5, 2023
@bnoordhuis @RafaelGSS
src: fix tls certificate root store data race
2f898f2 
OpenSSL internally synchronizes access to the X509_STORE. Creation of
the global root store in Node was not properly synchronized, however,
introducing the possibility of data races when multiple threads try to
create it concurrently.

This commit coincidentally removes the last call to the thread-unsafe
ERR_error_string() function.

Fixes: #45743
PR-URL: #45767
Reviewed-By: Anna Henningsen <anna@addaleax.net>
juanarbol pushed a commit that referenced this issue Jan 26, 2023
@bnoordhuis @juanarbol
src: fix tls certificate root store data race
f069246 
OpenSSL internally synchronizes access to the X509_STORE. Creation of
the global root store in Node was not properly synchronized, however,
introducing the possibility of data races when multiple threads try to
create it concurrently.

This commit coincidentally removes the last call to the thread-unsafe
ERR_error_string() function.

Fixes: #45743
PR-URL: #45767
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

src: fix tls certificate root store data race bnoordhuis/io.js
1 participant
@bnoordhuis