Skip to content

Commit

Permalink
Ensure thread safety of ssl initor
Browse files Browse the repository at this point in the history
  • Loading branch information
@xia-chu
xia-chu committed Sep 21, 2024
1 parent 6a392de commit ba5deb1
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 3 deletions.
10 changes: 8 additions & 2 deletions src/Util/SSLBox.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,7 @@ SSL_Initor::~SSL_Initor() {
#endif //defined(ENABLE_OPENSSL)
}

bool SSL_Initor::loadCertificate(const string &pem_or_p12, bool server_mode, const string &password, bool is_file,
bool is_default) {
bool SSL_Initor::loadCertificate(const string &pem_or_p12, bool server_mode, const string &password, bool is_file, bool is_default) {
auto cers = SSLUtil::loadPublicKey(pem_or_p12, password, is_file);
auto key = SSLUtil::loadPrivateKey(pem_or_p12, password, is_file);
auto ssl_ctx = SSLUtil::makeSSLContext(cers, key, server_mode, true);
Expand Down Expand Up @@ -128,6 +127,7 @@ int SSL_Initor::findCertificate(SSL *ssl, int *, void *arg) {
if (!ctx) {
//未找到对应的证书 [AUTO-TRANSLATED:d4550e6f]
//No corresponding certificate found
std::lock_guard<std::recursive_mutex> lck(ref._mtx);
WarnL << "Can not find any certificate of host: " << vhost
<< ", select default certificate of: " << ref._default_vhost[(bool) (arg)];
}
Expand All @@ -153,6 +153,7 @@ int SSL_Initor::findCertificate(SSL *ssl, int *, void *arg) {
}

bool SSL_Initor::setContext(const string &vhost, const shared_ptr<SSL_CTX> &ctx, bool server_mode, bool is_default) {
std::lock_guard<std::recursive_mutex> lck(_mtx);
if (!ctx) {
return false;
}
Expand Down Expand Up @@ -240,6 +241,7 @@ void SSL_Initor::setupCtx(SSL_CTX *ctx) {
}

shared_ptr<SSL> SSL_Initor::makeSSL(bool server_mode) {
std::lock_guard<std::recursive_mutex> lck(_mtx);
#if defined(ENABLE_OPENSSL)
#ifdef SSL_ENABLE_SNI
//openssl 版本支持SNI [AUTO-TRANSLATED:b8029f6c]
Expand All @@ -256,6 +258,7 @@ shared_ptr<SSL> SSL_Initor::makeSSL(bool server_mode) {
}

bool SSL_Initor::trustCertificate(X509 *cer, bool server_mode) {
std::lock_guard<std::recursive_mutex> lck(_mtx);
return SSLUtil::trustCertificate(_ctx_empty[server_mode].get(), cer);
}

Expand All @@ -276,6 +279,7 @@ std::shared_ptr<SSL_CTX> SSL_Initor::getSSLCtx(const string &vhost, bool server_
}

std::shared_ptr<SSL_CTX> SSL_Initor::getSSLCtxWildcards(const string &vhost, bool server_mode) {
std::lock_guard<std::recursive_mutex> lck(_mtx);
for (auto &pr : _ctxs_wildcards[server_mode]) {
auto pos = strcasestr(vhost.data(), pr.first.data());
if (pos && pos + pr.first.size() == &vhost.back() + 1) {
Expand All @@ -286,6 +290,7 @@ std::shared_ptr<SSL_CTX> SSL_Initor::getSSLCtxWildcards(const string &vhost, boo
}

std::shared_ptr<SSL_CTX> SSL_Initor::getSSLCtx_l(const string &vhost_in, bool server_mode) {
std::lock_guard<std::recursive_mutex> lck(_mtx);
auto vhost = vhost_in;
if (vhost.empty()) {
if (!_default_vhost[server_mode].empty()) {
Expand All @@ -309,6 +314,7 @@ std::shared_ptr<SSL_CTX> SSL_Initor::getSSLCtx_l(const string &vhost_in, bool se
}

string SSL_Initor::defaultVhost(bool server_mode) {
std::lock_guard<std::recursive_mutex> lck(_mtx);
return _default_vhost[server_mode];
}

Expand Down
3 changes: 2 additions & 1 deletion src/Util/SSLBox.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ class SSL_Initor {
* [AUTO-TRANSLATED:1b3438d0]
*/
void setupCtx(SSL_CTX *ctx);
static void setupCtx(SSL_CTX *ctx);

std::shared_ptr<SSL_CTX> getSSLCtx_l(const std::string &vhost, bool server_mode);

Expand Down Expand Up @@ -184,6 +184,7 @@ class SSL_Initor {
};

private:
std::recursive_mutex _mtx;
std::string _default_vhost[2];
std::shared_ptr<SSL_CTX> _ctx_empty[2];
std::map<std::string, std::shared_ptr<SSL_CTX>, less_nocase> _ctxs[2];
Expand Down

2 comments on commit ba5deb1

@alexliyu7352
Copy link
Member

Choose a reason for hiding this comment

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

AI Review for src/Util/SSLBox.cpp:

Code Review: Patch to src/Util/SSLBox.cpp

Summary

This patch introduces thread safety to the SSL_Initor class by adding mutex locks to various methods. This is a positive change that improves the robustness of the code.

Detailed Feedback

Code Overview

The patch adds std::lock_guard<std::recursive_mutex> locks to the following methods in the SSL_Initor class:

  • loadCertificate
  • findCertificate
  • setContext
  • makeSSL
  • trustCertificate
  • getSSLCtxWildcards
  • getSSLCtx_l
  • defaultVhost

This ensures that these methods are thread-safe and prevent potential race conditions.

Strengths

  • Improved Thread Safety: The addition of mutex locks significantly enhances the thread safety of the SSL_Initor class, making it more robust in multi-threaded environments.
  • Clear Locking: The use of std::lock_guard makes the locking mechanism explicit and easy to understand.

Areas for Improvement

1. Recursive Mutex Usage

  • Issue: The patch uses std::recursive_mutex for locking. While this allows for recursive locking, it might be overkill in this case. If the code doesn't require recursive locking, using a standard std::mutex would be more efficient.
  • Suggestion: Consider using std::mutex instead of std::recursive_mutex unless recursive locking is truly necessary.
  • Example: 
    // Replace:
std::lock_guard<std::recursive_mutex> lck(_mtx);
// With:
std::lock_guard<std::mutex> lck(_mtx);

2. Locking Granularity

  • Issue: The patch locks the entire SSL_Initor object for most methods. This might be too coarse-grained, potentially leading to unnecessary blocking.
  • Suggestion: Analyze the code to identify critical sections that require locking and lock only those sections. This can improve concurrency and performance.
  • Example: 
    // Instead of locking the entire _ctxs map:
std::lock_guard<std::mutex> lck(_mtx);
// Consider locking only the specific entry being accessed:
std::lock_guard<std::mutex> lck(_ctxs[server_mode][vhost]._mtx);

3. Documentation

  • Issue: The patch doesn't include any documentation about the changes made.
  • Suggestion: Add a comment explaining the purpose of the changes and the rationale for using mutex locks. This will help future developers understand the code better.
  • Example: 
    // Add a comment like this:
// This patch introduces thread safety to the SSL_Initor class by adding mutex locks to various methods.
// This is necessary to prevent race conditions in multi-threaded environments.

Conclusion

This patch is a valuable improvement that enhances the thread safety of the SSL_Initor class. However, the use of std::recursive_mutex and the coarse-grained locking might be unnecessary. Consider using std::mutex and refining the locking granularity for better performance. Additionally, adding documentation about the changes would be beneficial.

TRANS_BY_GITHUB_AI_ASSISTANT

@alexliyu7352
Copy link
Member

Choose a reason for hiding this comment

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

AI Review for src/Util/SSLBox.h:

Code Review: Patch to src/Util/SSLBox.h

Summary

This patch introduces a mutex to the SSL_Initor class to ensure thread safety. This is a positive change that addresses a potential race condition in the original code.

Detailed Feedback

Code Overview

The patch adds a std::recursive_mutex named _mtx to the SSL_Initor class. This mutex is used to protect the access to the _ctxs, _ctxs_wildcards, _ctx_empty, and _default_vhost members, which are shared between threads.

Strengths

  • Thread Safety: The addition of the mutex effectively addresses the potential race condition in the original code. This is a significant improvement in terms of code correctness and reliability.
  • Minimal Impact: The patch is concise and only modifies the SSL_Initor class, minimizing the impact on other parts of the codebase.

Areas for Improvement

1. Mutex Usage

  • Issue: The mutex is declared as a member variable of the SSL_Initor class. This means that the mutex is created and destroyed along with the SSL_Initor object. However, the SSL_Initor is a singleton, meaning that it is only created once. This could lead to unnecessary overhead if the mutex is not actually needed for the entire lifetime of the application.
  • Suggestion: Consider using a static mutex instead of a member variable. This would ensure that the mutex is only created once and destroyed when the application exits.
  • Example: 
    static std::recursive_mutex _mtx;

2. Mutex Locking

  • Issue: The patch does not explicitly show how the mutex is being used to protect the shared members. It is assumed that the mutex is being locked and unlocked appropriately in the relevant methods.
  • Suggestion: Add comments or code to clearly indicate where the mutex is being locked and unlocked. This would make the code more readable and easier to understand.
  • Example: 
    std::shared_ptr<SSL_CTX> getSSLCtx_l(const std::string &vhost, bool server_mode) {
    std::lock_guard<std::recursive_mutex> lock(_mtx); // Lock the mutex
    // ... access shared members ...
}

Conclusion

This patch is a positive step towards improving the thread safety of the SSL_Initor class. However, there are some areas for improvement related to mutex usage and locking. By addressing these points, the code can be made more efficient and easier to maintain.

TRANS_BY_GITHUB_AI_ASSISTANT

Please sign in to comment.