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

Feature/test #250

Merged
merged 2 commits into from
Sep 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 39 additions & 37 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,26 +9,26 @@ check_struct_has_member("struct mmsghdr" msg_hdr sys/socket.h HAVE_MMSG_HDR)
check_symbol_exists(sendmmsg sys/socket.h HAVE_SENDMMSG_API)
check_symbol_exists(recvmmsg sys/socket.h HAVE_RECVMMSG_API)

if(HAVE_MMSG_HDR)
if (HAVE_MMSG_HDR)
add_definitions(-DHAVE_MMSG_HDR)
endif()
if(HAVE_SENDMMSG_API)
endif ()
if (HAVE_SENDMMSG_API)
add_definitions(-DHAVE_SENDMMSG_API)
endif()
if(HAVE_RECVMMSG_API)
endif ()
if (HAVE_RECVMMSG_API)
add_definitions(-DHAVE_RECVMMSG_API)
endif()
endif ()

# check the socket buffer size set by the upper cmake project, if it is set, use the setting of the upper cmake project, otherwise set it to 256K
# if the socket buffer size is set to 0, it means that the socket buffer size is not set, and the kernel default value is used(just for linux)
if(DEFINED SOCKET_DEFAULT_BUF_SIZE)
if (DEFINED SOCKET_DEFAULT_BUF_SIZE)
if (SOCKET_DEFAULT_BUF_SIZE EQUAL 0)
message(STATUS "Socket default buffer size is not set, use the kernel default value")
else()
else ()
message(STATUS "Socket default buffer size is set to ${SOCKET_DEFAULT_BUF_SIZE}")
endif ()
add_definitions(-DSOCKET_DEFAULT_BUF_SIZE=${SOCKET_DEFAULT_BUF_SIZE})
endif()
endif ()
#加载自定义模块
#Load custom modules
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
Expand All @@ -42,47 +42,49 @@ set(EXECUTABLE_OUTPUT_PATH ${PROJECT_BINARY_DIR}/bin)
#Set subdirectories
set(SUB_DIR_LIST "Network" "Poller" "Thread" "Util")

if(WIN32)
if (WIN32)
list(APPEND SUB_DIR_LIST "win32")
#防止Windows.h包含Winsock.h
#Prevent Windows.h from including Winsock.h
add_definitions(-DWIN32_LEAN_AND_MEAN)
if (MSVC)
add_compile_options("/utf-8")
endif()
endif ()
set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)
endif()
else ()
add_compile_options("-Wno-comment")
endif ()

#安装目录
#Installation directory
if(WIN32)
if (WIN32)
set(INSTALL_PATH_LIB $ENV{HOME}/${PROJECT_NAME}/lib)
set(INSTALL_PATH_INCLUDE $ENV{HOME}/${PROJECT_NAME}/include)
else()
else ()
set(INSTALL_PATH_LIB lib)
set(INSTALL_PATH_INCLUDE include)
endif()
endif ()

foreach(SUB_DIR ${SUB_DIR_LIST})
foreach (SUB_DIR ${SUB_DIR_LIST})
#遍历源文件
#Traverse source file
aux_source_directory(src/${SUB_DIR} SRC_LIST)
#安装头文件至系统目录
#Install header file to system directory
install(DIRECTORY src/${SUB_DIR} DESTINATION ${INSTALL_PATH_INCLUDE} FILES_MATCHING PATTERN "*.h")
endforeach()
endforeach ()

#非苹果平台移除.mm类型的文件
#Remove .mm type files from non-apple platforms
if(NOT APPLE)
if (NOT APPLE)
list(REMOVE_ITEM SRC_LIST "src/Network/Socket_ios.mm")
endif()
endif ()

if(WIN32)
if (WIN32)
set(LINK_LIB_LIST WS2_32 Iphlpapi shlwapi)
else()
else ()
set(LINK_LIB_LIST)
endif()
endif ()


set(ENABLE_OPENSSL ON CACHE BOOL "enable openssl")
Expand All @@ -92,29 +94,29 @@ set(ASAN_USE_DELETE OFF CACHE BOOL "use delele[] or free when asan enabled")
#查找openssl是否安装
#Find out if openssl is installed
find_package(OpenSSL QUIET)
if(OPENSSL_FOUND AND ENABLE_OPENSSL)
if (OPENSSL_FOUND AND ENABLE_OPENSSL)
message(STATUS "找到openssl库:\"${OPENSSL_INCLUDE_DIR}\",ENABLE_OPENSSL宏已打开")
include_directories(${OPENSSL_INCLUDE_DIR})
add_definitions(-DENABLE_OPENSSL)
list(APPEND LINK_LIB_LIST ${OPENSSL_LIBRARIES})
endif()
list(APPEND LINK_LIB_LIST ${OPENSSL_LIBRARIES})
endif ()

#查找mysql是否安装
#Find out if mysql is installed
find_package(MYSQL QUIET)
if(MYSQL_FOUND AND ENABLE_MYSQL)
if (MYSQL_FOUND AND ENABLE_MYSQL)
message(STATUS "找到mysqlclient库:\"${MYSQL_INCLUDE_DIR}\",ENABLE_MYSQL宏已打开")
include_directories(${MYSQL_INCLUDE_DIR})
include_directories(${MYSQL_INCLUDE_DIR}/mysql)
add_definitions(-DENABLE_MYSQL)
list(APPEND LINK_LIB_LIST ${MYSQL_LIBRARIES})
endif()
list(APPEND LINK_LIB_LIST ${MYSQL_LIBRARIES})
endif ()

#是否使用delete[]替代free,用于解决开启asan后在MacOS上的卡死问题
#use delele[] or free when asan enabled
if(ASAN_USE_DELETE)
if (ASAN_USE_DELETE)
add_definitions(-DASAN_USE_DELETE)
endif()
endif ()

#打印库文件
#Print library files
Expand All @@ -123,20 +125,20 @@ message(STATUS "将链接依赖库:${LINK_LIB_LIST}")
#Enable c++11
set(CMAKE_CXX_STANDARD 11)

if(NOT WIN32)
if (NOT WIN32)
add_compile_options(-Wno-deprecated-declarations)
add_compile_options(-Wno-predefined-identifier-outside-function)
endif()
endif ()

#编译动态库
#Compile dynamic library
if(NOT IOS AND NOT ANDROID AND NOT WIN32)
if (NOT IOS AND NOT ANDROID AND NOT WIN32)
add_library(${PROJECT_NAME}_shared SHARED ${SRC_LIST})
target_include_directories(${PROJECT_NAME}_shared PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/src)
target_link_libraries(${PROJECT_NAME}_shared ${LINK_LIB_LIST})
set_target_properties(${PROJECT_NAME}_shared PROPERTIES OUTPUT_NAME "${PROJECT_NAME}")
install(TARGETS ${PROJECT_NAME}_shared ARCHIVE DESTINATION ${INSTALL_PATH_LIB} LIBRARY DESTINATION ${INSTALL_PATH_LIB})
endif()
install(TARGETS ${PROJECT_NAME}_shared ARCHIVE DESTINATION ${INSTALL_PATH_LIB} LIBRARY DESTINATION ${INSTALL_PATH_LIB})
endif ()

#编译静态库
#Compile static library
Expand All @@ -152,6 +154,6 @@ install(TARGETS ${PROJECT_NAME}_static ARCHIVE DESTINATION ${INSTALL_PATH_LIB})

#测试程序
#Test program
if(NOT IOS)
if (NOT IOS)
add_subdirectory(tests)
endif()
endif ()
10 changes: 8 additions & 2 deletions src/Util/SSLBox.cpp
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 adds mutex locks to various methods in the SSL_Initor class to ensure thread safety. This is a positive change that improves the code's robustness and prevents potential race conditions.

Detailed Feedback

Code Overview

The patch introduces std::lock_guard to protect shared data structures within the SSL_Initor class. This is a necessary step to prevent data corruption and ensure thread safety.

Strengths

  • Thread Safety: The addition of mutex locks significantly improves the thread safety of the SSL_Initor class.
  • Clear Locking: The use of std::lock_guard makes the locking mechanism explicit and easy to understand.

Areas for Improvement

1. Locking Granularity

  • Issue: Some methods, like loadCertificate, acquire a lock for the entire method duration. This could potentially lead to unnecessary blocking if other threads need to access the same data structures.
  • Suggestion: Consider using finer-grained locking by acquiring locks only for the specific data structures being accessed within each method. This can improve concurrency and performance.
  • Example:
    bool SSL_Initor::loadCertificate(const string &pem_or_p12, bool server_mode, const string &password, bool is_file, bool is_default) {
        std::lock_guard<std::recursive_mutex> lck(_mtx); // Lock for _mtx
        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);
        if (!ssl_ctx) {
            return false;
        }
        for (auto &cer : cers) {
            auto server_name = SSLUtil::getServerName(cer.get());
            {
                std::lock_guard<std::recursive_mutex> lck2(_ctxs[server_mode]); // Lock for _ctxs[server_mode]
                _ctxs[server_mode][server_name] = ssl_ctx;
            }
            if (is_default) {
                std::lock_guard<std::recursive_mutex> lck3(_default_vhost); // Lock for _default_vhost
                _default_vhost[server_mode] = server_name;
            }
            if (server_name.find("*.") == 0) {
                std::lock_guard<std::recursive_mutex> lck4(_ctxs_wildcards[server_mode]); // Lock for _ctxs_wildcards[server_mode]
                _ctxs_wildcards[server_mode][server_name.substr(1)] = ssl_ctx;
            }
            break;
        }
        return true;
    }

2. Locking Consistency

  • Issue: The findCertificate method acquires a lock on ref._mtx within the if (vhost && vhost[0] != '\0') block, but not in the subsequent if (!ctx) blocks. This could lead to a race condition if another thread modifies _default_vhost while the lock is not held.
  • Suggestion: Ensure that all accesses to shared data structures within the findCertificate method are protected by the same lock.
  • Example:
    int SSL_Initor::findCertificate(SSL *ssl, int *, void *arg) {
    #if !defined(ENABLE_OPENSSL) || !defined(SSL_ENABLE_SNI)
        return 0;
    #else
        if (!ssl) {
            return SSL_TLSEXT_ERR_ALERT_FATAL;
        }
    
        SSL_CTX *ctx = nullptr;
        static auto &ref = SSL_Initor::Instance();
        const char *vhost = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
    
        std::lock_guard<std::recursive_mutex> lck(ref._mtx); // Acquire lock for ref._mtx
        if (vhost && vhost[0] != '\0') {
            //根据域名找到证书  [AUTO-TRANSLATED:783a55d8]
            //Find the certificate based on the domain name
            ctx = ref.getSSLCtx(vhost, (bool) (arg)).get();
            if (!ctx) {
                //未找到对应的证书  [AUTO-TRANSLATED:d4550e6f]
                //No corresponding certificate found
                WarnL << "Can not find any certificate of host: " << vhost
                      << ", select default certificate of: " << ref._default_vhost[(bool) (arg)];
            }
        }
    
        if (!ctx) {
            //客户端未指定域名或者指定的证书不存在,那么选择一个默认的证书  [AUTO-TRANSLATED:35115b5c]
            //The client did not specify a domain name or the specified certificate does not exist, so a default certificate is selected
            ctx = ref.getSSLCtx("", (bool) (arg)).get();
        }
    
        if (!ctx) {
            //未有任何有效的证书  [AUTO-TRANSLATED:e1d7f5b7]
            //No valid certificate available
            WarnL << "Can not find any available certificate of host: " << (vhost ? vhost : "default host")
                  << ", tls handshake failed";
            return SSL_TLSEXT_ERR_ALERT_FATAL;
        }
    
        SSL_set_SSL_CTX(ssl, ctx);
        return SSL_TLSEXT_ERR_OK;
    #endif
    }

3. Deadlock Potential

  • Issue: The getSSLCtx_l method acquires a lock on _mtx before accessing _ctxs and _default_vhost. If another thread is holding a lock on _ctxs or _default_vhost and tries to acquire a lock on _mtx, a deadlock could occur.
  • Suggestion: Consider using a single mutex for all shared data structures within SSL_Initor to avoid potential deadlocks. Alternatively, if multiple mutexes are necessary, ensure that they are acquired in a consistent order to prevent circular dependencies.

Conclusion

This patch is a positive step towards improving the thread safety of the SSL_Initor class. However, the locking granularity and consistency should be reviewed to ensure optimal performance and prevent potential deadlocks.

TRANS_BY_GITHUB_AI_ASSISTANT

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 several changes to the SSL_Initor class, primarily focused on adding mutex locks to protect shared data structures. This is a positive step towards thread safety, but there are some areas for improvement in terms of consistency and clarity.

Detailed Feedback

Code Overview

The patch adds mutex locks to various methods within the SSL_Initor class, aiming to prevent race conditions when multiple threads access shared data structures like _ctxs, _ctxs_wildcards, and _default_vhost.

Strengths

  • Thread Safety: The addition of mutex locks is a significant improvement for thread safety, especially in a network programming framework where concurrent access is common.
  • Clearer Intent: The use of std::lock_guard makes the locking mechanism more explicit and easier to understand.

Areas for Improvement

1. Consistency in Locking

  • Missing Locks: The loadCertificate method lacks a mutex lock, potentially leading to race conditions when multiple threads attempt to load certificates concurrently.
  • Unnecessary Locks: The getSSLCtxWildcards method acquires a lock even though it only reads from the _ctxs_wildcards map. This lock is unnecessary and could introduce performance overhead.

2. Clarity and Readability

  • Redundant Locking: The findCertificate method acquires a lock within the if (vhost && vhost[0] != '\0') block, but then acquires another lock outside of it. This redundant locking can be simplified.
  • Inconsistent Naming: The getSSLCtx_l method uses a lock named lck, while other methods use _mtx. This inconsistency can be confusing.

3. Potential Performance Impact

  • Lock Granularity: The use of a single mutex for the entire SSL_Initor class might be too coarse-grained. Consider using separate mutexes for different data structures to reduce contention and improve performance.

Conclusion

The patch is a step in the right direction towards thread safety, but it needs further refinement to ensure consistency and clarity. Addressing the missing locks, redundant locking, and inconsistent naming will improve the code's quality and maintainability. Additionally, consider using finer-grained locking to optimize performance.

TRANS_BY_GITHUB_AI_ASSISTANT

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
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 protect the SSL context map in the SSL_Initor class. This is a good step towards making the class thread-safe.

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 _ctxs and _ctxs_wildcards maps, which store SSL contexts for different virtual hosts.

Strengths

  • Thread Safety: The addition of the mutex improves the thread safety of the SSL_Initor class. This is important because the class is likely to be accessed from multiple threads in a multi-threaded application.
  • Recursive Mutex: The use of a std::recursive_mutex is appropriate because the SSL_Initor class may need to acquire the mutex recursively, for example, when loading certificates.

Areas for Improvement

1. Mutex Usage

  • Mutex Scope: The mutex should be acquired and released around all access to the _ctxs and _ctxs_wildcards maps. Currently, the mutex is only declared, but not used in the code.
  • Mutex Locking: The mutex should be locked before accessing the maps and unlocked after the access is complete. This ensures that the maps are protected from concurrent access.

Example:

std::shared_ptr<SSL_CTX> SSL_Initor::getSSLCtx_l(const std::string &vhost, bool server_mode) {
    std::lock_guard<std::recursive_mutex> lock(_mtx); // Acquire mutex
    // ... access _ctxs or _ctxs_wildcards maps ...
    return ...;
}

2. Documentation

  • Mutex Explanation: The patch should include a comment explaining why the mutex is being added and how it is used. This will help other developers understand the changes and maintain the code.

Example:

// _mtx: A recursive mutex to protect the _ctxs and _ctxs_wildcards maps from concurrent access.
std::recursive_mutex _mtx;

Conclusion

This patch is a good step towards making the SSL_Initor class thread-safe. However, the mutex needs to be properly used to ensure that the maps are protected from concurrent access. The patch should also include documentation explaining the changes.

TRANS_BY_GITHUB_AI_ASSISTANT

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 protect the SSL context map in the SSL_Initor class. This is a good step towards making the class thread-safe.

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 _ctxs and _ctxs_wildcards maps, which store SSL contexts for different virtual hosts.

Strengths

  • Thread Safety: The addition of the mutex improves the thread safety of the SSL_Initor class. This is important because the class is likely to be accessed from multiple threads in a multi-threaded application.
  • Recursive Mutex: The use of a std::recursive_mutex is appropriate because the SSL_Initor class may need to acquire the mutex recursively, for example, when loading certificates.

Areas for Improvement

1. Mutex Usage

  • Mutex Scope: The mutex should be acquired and released around all access to the _ctxs and _ctxs_wildcards maps. Currently, the mutex is only declared, but not used in the code.
  • Mutex Locking: The mutex should be locked before accessing the maps and unlocked after the access is complete. This ensures that the maps are protected from concurrent access.

Example:

std::shared_ptr<SSL_CTX> SSL_Initor::getSSLCtx_l(const std::string &vhost, bool server_mode) {
    std::lock_guard<std::recursive_mutex> lock(_mtx); // Acquire mutex
    // ... access _ctxs or _ctxs_wildcards maps ...
    return ...;
}

2. Documentation

  • Mutex Explanation: The patch should include a comment explaining why the mutex is being added and how it is used. This will help other developers understand the changes and maintain the code.

Example:

// _mtx: A recursive mutex to protect the _ctxs and _ctxs_wildcards maps from concurrent access.
std::recursive_mutex _mtx;

Conclusion

This patch is a good step towards making the SSL_Initor class thread-safe. However, the mutex needs to be properly used to ensure that the maps are protected from concurrent access. The patch should also include documentation explaining the changes.

TRANS_BY_GITHUB_AI_ASSISTANT

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
Loading