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 0ae34be commit f8add83
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 f8add83

@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 and reliability of the code.

Detailed Feedback

Code Overview

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

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

This ensures that access to shared data structures within the SSL_Initor class is synchronized, preventing race conditions and potential data corruption.

Strengths

  • Improved Thread Safety: The addition of mutex locks significantly enhances the thread safety of the SSL_Initor class, making it more robust and reliable in multi-threaded environments.
  • Clear and Concise Changes: The patch is well-written and easy to understand. The use of std::lock_guard makes the locking mechanism clear and concise.

Areas for Improvement

1. Potential Deadlock

  • Issue: The use of a std::recursive_mutex in SSL_Initor might lead to potential deadlocks if the same thread attempts to acquire the mutex multiple times within a nested call.
  • Suggestion: Consider using a std::mutex instead of a std::recursive_mutex if the code does not require recursive locking. This would simplify the locking mechanism and reduce the risk of deadlocks.
  • Example: 
    // Replace std::recursive_mutex with std::mutex
std::mutex _mtx;

2. Locking Granularity

  • Issue: The patch locks the entire SSL_Initor object for every method call. This might be overly restrictive and could lead to performance bottlenecks if multiple threads need to access different parts of the SSL_Initor object concurrently.
  • Suggestion: Consider using finer-grained locking by locking only the specific data structures that are being accessed within each method. This would improve concurrency and performance.
  • Example: 
    // Lock only the specific data structure being accessed
std::lock_guard<std::mutex> lck(_ctxs_wildcards[server_mode]);

3. Documentation

  • Issue: The patch does not include any documentation explaining the reason for the changes or the potential impact on the code.
  • Suggestion: Add a comment at the beginning of the patch file explaining the purpose of the changes and the rationale behind using mutex locks. This would make the patch more understandable and maintainable.

Conclusion

This patch is a valuable improvement that addresses a critical issue of thread safety in the SSL_Initor class. However, the use of a std::recursive_mutex and the coarse-grained locking might introduce potential issues. It is recommended to review the locking strategy and consider using a std::mutex and finer-grained locking for better performance and reduced risk of deadlocks. Additionally, adding documentation to the patch would enhance its clarity and maintainability.

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 std::recursive_mutex to the SSL_Initor class, aiming to ensure thread safety.

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. This is a good step towards making the SSL_Initor thread-safe.

Strengths

  • Thread Safety: The addition of the std::recursive_mutex is a positive step towards making the SSL_Initor thread-safe. This is important for applications that might access the SSL context from multiple threads.

Areas for Improvement

1. Mutex Usage and Scope

  • Issue: The mutex is declared as a member of the SSL_Initor class. This means that the mutex is created and destroyed along with the SSL_Initor object. However, the mutex is only used to protect the access to the _ctxs, _ctxs_wildcards, _ctx_empty, and _default_vhost members. These members are likely to be accessed from multiple threads, even after the SSL_Initor object has been destroyed.
  • Suggestion: Consider using a static mutex instead of a member mutex. This will ensure that the mutex is available even after the SSL_Initor object has been destroyed.
  • Example: 
    static std::recursive_mutex _mtx;

2. Mutex Locking and Unlocking

  • Issue: The patch only adds the mutex declaration but doesn't actually lock and unlock the mutex in the relevant methods.
  • Suggestion: Lock the mutex before accessing the protected members and unlock it after the access is complete.
  • Example: 
    bool setContext(const std::string &vhost, const std::shared_ptr<SSL_CTX> &ctx, bool server_mode, bool is_default = true) {
    std::lock_guard<std::recursive_mutex> lck(_mtx); // Lock the mutex
    if (!ctx) {
        return false;
    }
    setupCtx(ctx.get());
    // ... rest of the method ...
}

3. Documentation

  • Issue: The patch doesn't include any documentation about the changes.
  • Suggestion: Add a comment explaining the purpose of the mutex and how it ensures thread safety.

Conclusion

The patch is a good start towards making the SSL_Initor thread-safe. However, it needs to be completed by adding mutex locking and unlocking in the relevant methods. Additionally, the documentation should be updated to reflect the changes.

TRANS_BY_GITHUB_AI_ASSISTANT

Please sign in to comment.