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

Fix webserver thread safety #330

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix webserver thread safety #330

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2ff6e31
add test
fchevassu-antidot Jul 24, 2023
ba2234d
Use const when possible
fchevassu-antidot Jul 24, 2023
d4a89d8
Protect webserver::registered_resources_*
fchevassu-antidot Jul 24, 2023
2f7b4ff
Protect webserver::bans and webserver::allowances
fchevassu-antidot Jul 24, 2023
145e9aa
Split bans_and_allowances_mutex in two distinct mutexes
fchevassu-antidot Aug 11, 2023
589ff8f
Simplify `policy_callback` condition
fchevassu-antidot Aug 11, 2023
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
13 changes: 8 additions & 5 deletions src/httpserver/webserver.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
#include <map>
#include <memory>
#include <set>
#include <shared_mutex>
#include <string>

#include "httpserver/http_utils.hpp"
Expand Down Expand Up @@ -167,16 +168,18 @@ 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::shared_mutex registered_resources_mutex;
std::map<details::http_endpoint, http_resource*> registered_resources;
std::map<std::string, http_resource*> registered_resources_str;

std::shared_mutex bans_and_allowances_mutex;
Copy link
Owner

Choose a reason for hiding this comment

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

It might be worth splitting this into two, given the writing use-cases are isolated

Copy link
Author

Choose a reason for hiding this comment

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

done

std::set<http::ip_representation> bans;
std::set<http::ip_representation> allowances;

Expand Down
81 changes: 46 additions & 35 deletions src/webserver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
#include <iosfwd>
#include <cstring>
#include <memory>
#include <mutex>
#include <stdexcept>
#include <utility>
#include <vector>
Expand Down Expand Up @@ -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<map<details::http_endpoint, http_resource*>::iterator, bool> result = registered_resources.insert(map<details::http_endpoint, http_resource*>::value_type(idx, hrm));

if (!family && result.second) {
Expand Down Expand Up @@ -361,12 +363,14 @@ 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());
}

void webserver::ban_ip(const string& ip) {
std::unique_lock bans_and_allowances_lock(bans_and_allowances_mutex);
ip_representation t_ip(ip);
set<ip_representation>::iterator it = bans.find(t_ip);
if (it != bans.end() && (t_ip.weight() < (*it).weight())) {
Expand All @@ -378,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<ip_representation>::iterator it = allowances.find(t_ip);
if (it != allowances.end() && (t_ip.weight() < (*it).weight())) {
Expand All @@ -389,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));
}

Expand All @@ -402,6 +409,7 @@ MHD_Result policy_callback(void *cls, const struct sockaddr* addr, socklen_t add

if (!(static_cast<webserver*>(cls))->ban_system_enabled) return MHD_YES;

std::shared_lock bans_and_allowances_lock((static_cast<webserver*>(cls))->bans_and_allowances_mutex);
Copy link
Owner

Choose a reason for hiding this comment

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

We can split the if statement here so that we always check only one of the two based on the chosen policy

Copy link
Author

Choose a reason for hiding this comment

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

In both ACCEPT and REJECT cases, we access bans and allowances, so I don't see how we could only lock one of the mutexes.

I added a commit simplifying the code, with no intended functional change.

Copy link
Owner

Choose a reason for hiding this comment

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

[MINOR] I think the cleanest way would be to have two private methods (is_allowed and is_banned) that use the locks internally and just call into those methods from within here. That should localize the locks to the maximum extent possible.

if ((((static_cast<webserver*>(cls))->default_policy == http_utils::ACCEPT) &&
((static_cast<webserver*>(cls))->bans.count(ip_representation(addr))) &&
(!(static_cast<webserver*>(cls))->allowances.count(ip_representation(addr)))) ||
Expand Down Expand Up @@ -626,51 +634,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<details::http_endpoint, http_resource*>::iterator found_endpoint;

details::http_endpoint endpoint(st_url, false, false, false);

map<details::http_endpoint, http_resource*>::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<details::http_endpoint, http_resource*>::iterator found_endpoint;

details::http_endpoint endpoint(st_url, false, false, false);

map<details::http_endpoint, http_resource*>::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<string> url_pars = found_endpoint->first.get_url_pars();
if (found) {
vector<string> url_pars = found_endpoint->first.get_url_pars();

vector<string> url_pieces = endpoint.get_url_pieces();
vector<int> 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<string> url_pieces = endpoint.get_url_pieces();
vector<int> 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) {
Expand Down
42 changes: 42 additions & 0 deletions test/integ/basic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@
*/

#include <curl/curl.h>
#include <atomic>
#include <map>
#include <memory>
#include <numeric>
#include <sstream>
#include <string>
#include <thread>

#include "./httpserver.hpp"
#include "httpserver/string_utilities.hpp"
Expand Down Expand Up @@ -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()