Skip to content

Fix webserver thread safety #330

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

Open
FlorianChevassu 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 1 commit
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
Prev Previous commit
Next Next commit
Protect webserver::bans and webserver::allowances
  • Loading branch information
@fchevassu-antidot
fchevassu-antidot committed Jul 25, 2023
commit 2f7b4fffdebf809b434e0f8f1d3d66caebfde6d5
1 change: 1 addition & 0 deletions src/httpserver/webserver.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ class webserver {
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

@etr etr Aug 1, 2023

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

@FlorianChevassu FlorianChevassu Aug 11, 2023

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
5 changes: 5 additions & 0 deletions src/webserver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ip_representation>::iterator it = bans.find(t_ip);
if (it != bans.end() && (t_ip.weight() < (*it).weight())) {
Expand All @@ -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<ip_representation>::iterator it = allowances.find(t_ip);
if (it != allowances.end() && (t_ip.weight() < (*it).weight())) {
Expand All @@ -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));
}

Expand All @@ -405,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

@etr etr Aug 1, 2023

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

@FlorianChevassu FlorianChevassu Aug 11, 2023

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

@etr etr Aug 17, 2023

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