Skip to content

Commit 305979e

Browse files
committed
Fix race condition in unregister_resource cache invalidation
Move route cache invalidation inside registered_resources_mutex lock in unregister_resource() to prevent use-after-free when threads retrieve cached resource pointers during unregistration. Also invalidate route cache on register_resource() for correctness when resources are dynamically registered at runtime. Add threading model documentation to http_request lazy caching.
1 parent 7e683da commit 305979e

File tree

2 files changed

+19
-4
lines changed

2 files changed

+19
-4
lines changed

src/httpserver/http_request.hpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -427,7 +427,11 @@ class http_request {
427427
std::string_view get_connection_value(std::string_view key, enum MHD_ValueKind kind) const;
428428
const http::header_view_map get_headerlike_values(enum MHD_ValueKind kind) const;
429429

430-
// Cache certain data items on demand so we can consistently return views
430+
// http_request objects are owned by a single connection and are not
431+
// shared across threads. Lazy caching (path_pieces, args, etc.) is
432+
// safe without synchronization under this invariant.
433+
434+
// Cache certain data items on demand so we can consistently return views
431435
// over the data. Some things we transform before returning to the user for
432436
// simplicity (e.g. query_str, requestor), others out of necessity (arg unescaping).
433437
// Others (username, password, digested_user) MHD returns as char* that we need

src/webserver.cpp

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -221,9 +221,12 @@ bool webserver::register_resource(const std::string& resource, http_resource* hr
221221
if (idx.is_regex_compiled()) {
222222
registered_resources_regex.insert(map<details::http_endpoint, http_resource*>::value_type(idx, hrm));
223223
}
224+
registered_resources_lock.unlock();
225+
invalidate_route_cache();
226+
return true;
224227
}
225228

226-
return result.second;
229+
return false;
227230
}
228231

229232
bool webserver::start(bool blocking) {
@@ -402,12 +405,20 @@ void webserver::unregister_resource(const string& resource) {
402405
// family does not matter - it just checks the url_normalized anyhow
403406
details::http_endpoint he(resource, false, true, regex_checking);
404407
std::unique_lock registered_resources_lock(registered_resources_mutex);
408+
409+
// Invalidate cache while holding registered_resources_mutex to prevent
410+
// any thread from retrieving dangling resource pointers from the cache
411+
// after we erase from the resource maps.
412+
{
413+
std::lock_guard<std::mutex> cache_lock(route_cache_mutex);
414+
route_cache_list.clear();
415+
route_cache_map.clear();
416+
}
417+
405418
registered_resources.erase(he);
406419
registered_resources.erase(he.get_url_complete());
407420
registered_resources_str.erase(he.get_url_complete());
408421
registered_resources_regex.erase(he);
409-
registered_resources_lock.unlock();
410-
invalidate_route_cache();
411422
}
412423

413424
void webserver::ban_ip(const string& ip) {

0 commit comments

Comments
 (0)