From 2ff6e31bfa90a345e82a5dfaab3610979006e496 Mon Sep 17 00:00:00 2001 From: Florian CHEVASSU Date: Mon, 24 Jul 2023 16:40:57 +0200 Subject: [PATCH 1/6] add test --- test/integ/basic.cpp | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/integ/basic.cpp b/test/integ/basic.cpp index 29c9dac8..6f9f2514 100644 --- a/test/integ/basic.cpp +++ b/test/integ/basic.cpp @@ -19,11 +19,13 @@ */ #include +#include #include #include #include #include #include +#include #include "./httpserver.hpp" #include "httpserver/string_utilities.hpp" @@ -1462,6 +1464,46 @@ LT_BEGIN_AUTO_TEST(basic_suite, method_not_allowed_header) curl_easy_cleanup(curl); LT_END_AUTO_TEST(method_not_allowed_header) +LT_BEGIN_AUTO_TEST(basic_suite, thread_safety) + simple_resource resource; + + std::atomic_bool done = false; + auto register_thread = std::thread([&]() { + int i = 0; + using namespace std::chrono; + while (!done) { + ws->register_resource( + std::string("/route") + std::to_string(++i), &resource); + } + }); + + auto get_thread = std::thread([&](){ + while (!done) { + CURL *curl = curl_easy_init(); + std::string s; + std::string url = "localhost:" PORT_STRING "/route" + std::to_string( + (int)((rand() * 10000000.0) / RAND_MAX)); + curl_easy_setopt(curl, CURLOPT_URL, url.c_str()); + curl_easy_setopt(curl, CURLOPT_HTTPGET, 1L); + curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writefunc); + curl_easy_setopt(curl, CURLOPT_WRITEDATA, &s); + curl_easy_perform(curl); + curl_easy_cleanup(curl); + } + }); + + using namespace std::chrono_literals; + std::this_thread::sleep_for(10s); + done = true; + if (register_thread.joinable()) { + register_thread.join(); + } + if (get_thread.joinable()) { + get_thread.join(); + } + LT_CHECK_EQ(1, 1); +LT_END_AUTO_TEST(thread_safety) + LT_BEGIN_AUTO_TEST_ENV() AUTORUN_TESTS() LT_END_AUTO_TEST_ENV() From ba2234d0556bde0139311ff91def7c1a2b5c815e Mon Sep 17 00:00:00 2001 From: Florian CHEVASSU Date: Mon, 24 Jul 2023 16:12:16 +0200 Subject: [PATCH 2/6] Use const when possible --- src/httpserver/webserver.hpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/httpserver/webserver.hpp b/src/httpserver/webserver.hpp index b2cbb1a6..b39e2352 100644 --- a/src/httpserver/webserver.hpp +++ b/src/httpserver/webserver.hpp @@ -167,13 +167,13 @@ class webserver { const std::string file_upload_dir; const bool generate_random_filename_on_upload; const bool deferred_enabled; - bool single_resource; - bool tcp_nodelay; + const bool single_resource; + const bool tcp_nodelay; pthread_mutex_t mutexwait; pthread_cond_t mutexcond; - render_ptr not_found_resource; - render_ptr method_not_allowed_resource; - render_ptr internal_error_resource; + const render_ptr not_found_resource; + const render_ptr method_not_allowed_resource; + const render_ptr internal_error_resource; std::map registered_resources; std::map registered_resources_str; From d4a89d8d61a0c7520e5b6800ca57280b63e4c976 Mon Sep 17 00:00:00 2001 From: Florian CHEVASSU Date: Mon, 24 Jul 2023 16:12:37 +0200 Subject: [PATCH 3/6] Protect webserver::registered_resources_* --- src/httpserver/webserver.hpp | 2 + src/webserver.cpp | 76 +++++++++++++++++++----------------- 2 files changed, 43 insertions(+), 35 deletions(-) diff --git a/src/httpserver/webserver.hpp b/src/httpserver/webserver.hpp index b39e2352..3d7d8e26 100644 --- a/src/httpserver/webserver.hpp +++ b/src/httpserver/webserver.hpp @@ -43,6 +43,7 @@ #include #include #include +#include #include #include "httpserver/http_utils.hpp" @@ -174,6 +175,7 @@ class webserver { const render_ptr not_found_resource; const render_ptr method_not_allowed_resource; const render_ptr internal_error_resource; + std::shared_mutex registered_resources_mutex; std::map registered_resources; std::map registered_resources_str; diff --git a/src/webserver.cpp b/src/webserver.cpp index 86a34c75..0ca405f8 100644 --- a/src/webserver.cpp +++ b/src/webserver.cpp @@ -43,6 +43,7 @@ #include #include #include +#include #include #include #include @@ -192,6 +193,7 @@ bool webserver::register_resource(const std::string& resource, http_resource* hr details::http_endpoint idx(resource, family, true, regex_checking); + std::unique_lock registered_resources_lock(registered_resources_mutex); pair::iterator, bool> result = registered_resources.insert(map::value_type(idx, hrm)); if (!family && result.second) { @@ -361,6 +363,7 @@ bool webserver::stop() { void webserver::unregister_resource(const string& resource) { // family does not matter - it just checks the url_normalized anyhow details::http_endpoint he(resource, false, true, regex_checking); + std::unique_lock registered_resources_lock(registered_resources_mutex); registered_resources.erase(he); registered_resources.erase(he.get_url_complete()); registered_resources_str.erase(he.get_url_complete()); @@ -626,51 +629,54 @@ MHD_Result webserver::finalize_answer(MHD_Connection* connection, struct details bool found = false; struct MHD_Response* raw_response; - if (!single_resource) { - const char* st_url = mr->standardized_url->c_str(); - fe = registered_resources_str.find(st_url); - if (fe == registered_resources_str.end()) { - if (regex_checking) { - map::iterator found_endpoint; - - details::http_endpoint endpoint(st_url, false, false, false); - - map::iterator it; - - size_t len = 0; - size_t tot_len = 0; - for (it = registered_resources.begin(); it != registered_resources.end(); ++it) { - size_t endpoint_pieces_len = (*it).first.get_url_pieces().size(); - size_t endpoint_tot_len = (*it).first.get_url_complete().size(); - if (!found || endpoint_pieces_len > len || (endpoint_pieces_len == len && endpoint_tot_len > tot_len)) { - if ((*it).first.match(endpoint)) { - found = true; - len = endpoint_pieces_len; - tot_len = endpoint_tot_len; - found_endpoint = it; + { + std::shared_lock registered_resources_lock(registered_resources_mutex); + if (!single_resource) { + const char* st_url = mr->standardized_url->c_str(); + fe = registered_resources_str.find(st_url); + if (fe == registered_resources_str.end()) { + if (regex_checking) { + map::iterator found_endpoint; + + details::http_endpoint endpoint(st_url, false, false, false); + + map::iterator it; + + size_t len = 0; + size_t tot_len = 0; + for (it = registered_resources.begin(); it != registered_resources.end(); ++it) { + size_t endpoint_pieces_len = (*it).first.get_url_pieces().size(); + size_t endpoint_tot_len = (*it).first.get_url_complete().size(); + if (!found || endpoint_pieces_len > len || (endpoint_pieces_len == len && endpoint_tot_len > tot_len)) { + if ((*it).first.match(endpoint)) { + found = true; + len = endpoint_pieces_len; + tot_len = endpoint_tot_len; + found_endpoint = it; + } } } - } - if (found) { - vector url_pars = found_endpoint->first.get_url_pars(); + if (found) { + vector url_pars = found_endpoint->first.get_url_pars(); - vector url_pieces = endpoint.get_url_pieces(); - vector chunks = found_endpoint->first.get_chunk_positions(); - for (unsigned int i = 0; i < url_pars.size(); i++) { - mr->dhr->set_arg(url_pars[i], url_pieces[chunks[i]]); - } + vector url_pieces = endpoint.get_url_pieces(); + vector chunks = found_endpoint->first.get_chunk_positions(); + for (unsigned int i = 0; i < url_pars.size(); i++) { + mr->dhr->set_arg(url_pars[i], url_pieces[chunks[i]]); + } - hrm = found_endpoint->second; + hrm = found_endpoint->second; + } } + } else { + hrm = fe->second; + found = true; } } else { - hrm = fe->second; + hrm = registered_resources.begin()->second; found = true; } - } else { - hrm = registered_resources.begin()->second; - found = true; } if (found) { From 2f7b4fffdebf809b434e0f8f1d3d66caebfde6d5 Mon Sep 17 00:00:00 2001 From: Florian CHEVASSU Date: Mon, 24 Jul 2023 16:15:41 +0200 Subject: [PATCH 4/6] Protect webserver::bans and webserver::allowances --- src/httpserver/webserver.hpp | 1 + src/webserver.cpp | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/httpserver/webserver.hpp b/src/httpserver/webserver.hpp index 3d7d8e26..a9dbdea4 100644 --- a/src/httpserver/webserver.hpp +++ b/src/httpserver/webserver.hpp @@ -179,6 +179,7 @@ class webserver { std::map registered_resources; std::map registered_resources_str; + std::shared_mutex bans_and_allowances_mutex; std::set bans; std::set allowances; diff --git a/src/webserver.cpp b/src/webserver.cpp index 0ca405f8..64806c50 100644 --- a/src/webserver.cpp +++ b/src/webserver.cpp @@ -370,6 +370,7 @@ void webserver::unregister_resource(const string& resource) { } void webserver::ban_ip(const string& ip) { + std::unique_lock bans_and_allowances_lock(bans_and_allowances_mutex); ip_representation t_ip(ip); set::iterator it = bans.find(t_ip); if (it != bans.end() && (t_ip.weight() < (*it).weight())) { @@ -381,6 +382,7 @@ void webserver::ban_ip(const string& ip) { } void webserver::allow_ip(const string& ip) { + std::unique_lock bans_and_allowances_lock(bans_and_allowances_mutex); ip_representation t_ip(ip); set::iterator it = allowances.find(t_ip); if (it != allowances.end() && (t_ip.weight() < (*it).weight())) { @@ -392,10 +394,12 @@ void webserver::allow_ip(const string& ip) { } void webserver::unban_ip(const string& ip) { + std::unique_lock bans_and_allowances_lock(bans_and_allowances_mutex); bans.erase(ip_representation(ip)); } void webserver::disallow_ip(const string& ip) { + std::unique_lock bans_and_allowances_lock(bans_and_allowances_mutex); allowances.erase(ip_representation(ip)); } @@ -405,6 +409,7 @@ MHD_Result policy_callback(void *cls, const struct sockaddr* addr, socklen_t add if (!(static_cast(cls))->ban_system_enabled) return MHD_YES; + std::shared_lock bans_and_allowances_lock((static_cast(cls))->bans_and_allowances_mutex); if ((((static_cast(cls))->default_policy == http_utils::ACCEPT) && ((static_cast(cls))->bans.count(ip_representation(addr))) && (!(static_cast(cls))->allowances.count(ip_representation(addr)))) || From 145e9aaa372c948221d4cda6ac19b0dba8149255 Mon Sep 17 00:00:00 2001 From: Florian CHEVASSU Date: Fri, 11 Aug 2023 10:07:12 +0200 Subject: [PATCH 5/6] Split bans_and_allowances_mutex in two distinct mutexes --- src/httpserver/webserver.hpp | 4 +++- src/webserver.cpp | 11 ++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/httpserver/webserver.hpp b/src/httpserver/webserver.hpp index a9dbdea4..26ebb7c7 100644 --- a/src/httpserver/webserver.hpp +++ b/src/httpserver/webserver.hpp @@ -179,8 +179,10 @@ class webserver { std::map registered_resources; std::map registered_resources_str; - std::shared_mutex bans_and_allowances_mutex; + std::shared_mutex bans_mutex; std::set bans; + + std::shared_mutex allowances_mutex; std::set allowances; struct MHD_Daemon* daemon; diff --git a/src/webserver.cpp b/src/webserver.cpp index 64806c50..ddac4b65 100644 --- a/src/webserver.cpp +++ b/src/webserver.cpp @@ -370,7 +370,7 @@ void webserver::unregister_resource(const string& resource) { } void webserver::ban_ip(const string& ip) { - std::unique_lock bans_and_allowances_lock(bans_and_allowances_mutex); + std::unique_lock bans_lock(bans_mutex); ip_representation t_ip(ip); set::iterator it = bans.find(t_ip); if (it != bans.end() && (t_ip.weight() < (*it).weight())) { @@ -382,7 +382,7 @@ void webserver::ban_ip(const string& ip) { } void webserver::allow_ip(const string& ip) { - std::unique_lock bans_and_allowances_lock(bans_and_allowances_mutex); + std::unique_lock allowances_lock(allowances_mutex); ip_representation t_ip(ip); set::iterator it = allowances.find(t_ip); if (it != allowances.end() && (t_ip.weight() < (*it).weight())) { @@ -394,12 +394,12 @@ void webserver::allow_ip(const string& ip) { } void webserver::unban_ip(const string& ip) { - std::unique_lock bans_and_allowances_lock(bans_and_allowances_mutex); + std::unique_lock bans_lock(bans_mutex); bans.erase(ip_representation(ip)); } void webserver::disallow_ip(const string& ip) { - std::unique_lock bans_and_allowances_lock(bans_and_allowances_mutex); + std::unique_lock allowances_lock(allowances_mutex); allowances.erase(ip_representation(ip)); } @@ -409,7 +409,8 @@ MHD_Result policy_callback(void *cls, const struct sockaddr* addr, socklen_t add if (!(static_cast(cls))->ban_system_enabled) return MHD_YES; - std::shared_lock bans_and_allowances_lock((static_cast(cls))->bans_and_allowances_mutex); + std::shared_lock bans_lock(bans_mutex); + std::shared_lock allowances_lock(allowances_mutex); if ((((static_cast(cls))->default_policy == http_utils::ACCEPT) && ((static_cast(cls))->bans.count(ip_representation(addr))) && (!(static_cast(cls))->allowances.count(ip_representation(addr)))) || From 589ff8f9a7f23b3f7ced19573e95386563e746dd Mon Sep 17 00:00:00 2001 From: Florian CHEVASSU Date: Fri, 11 Aug 2023 10:18:11 +0200 Subject: [PATCH 6/6] Simplify `policy_callback` condition --- src/webserver.cpp | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/webserver.cpp b/src/webserver.cpp index ddac4b65..9a15f418 100644 --- a/src/webserver.cpp +++ b/src/webserver.cpp @@ -407,16 +407,18 @@ MHD_Result policy_callback(void *cls, const struct sockaddr* addr, socklen_t add // Parameter needed to respect MHD interface, but not needed here. std::ignore = addrlen; - if (!(static_cast(cls))->ban_system_enabled) return MHD_YES; - - std::shared_lock bans_lock(bans_mutex); - std::shared_lock allowances_lock(allowances_mutex); - if ((((static_cast(cls))->default_policy == http_utils::ACCEPT) && - ((static_cast(cls))->bans.count(ip_representation(addr))) && - (!(static_cast(cls))->allowances.count(ip_representation(addr)))) || - (((static_cast(cls))->default_policy == http_utils::REJECT) && - ((!(static_cast(cls))->allowances.count(ip_representation(addr))) || - ((static_cast(cls))->bans.count(ip_representation(addr)))))) { + const auto ws = static_cast(cls); + + if (!ws->ban_system_enabled) return MHD_YES; + + std::shared_lock bans_lock(ws->bans_mutex); + std::shared_lock allowances_lock(ws->allowances_mutex); + const bool is_banned = ws->bans.count(ip_representation(addr)); + const bool is_allowed = ws->allowances.count(ip_representation(addr)); + + if ((ws->default_policy == http_utils::ACCEPT && is_banned && !is_allowed) || + (ws->default_policy == http_utils::REJECT && (!is_allowed || is_banned))) + { return MHD_NO; }