From e0a98fda2a4fcce3264eb5d1c0bd9855f7215a41 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 2 Aug 2026 01:29:42 +0000 Subject: [PATCH] fix: avoid use-after-free in rate_limiter periodic cleanup check_limit held a reference into std::flat_map requests[key], then erase_if'd empty entries on every 1000th call. flat_map erase invalidates all references, so the following size()/push_back was UAF whenever cleanup removed anything. Record the allow/deny decision before cleanup; add a regression covering the orphan-key path. Co-authored-by: Kaius Ruokonen --- net/net-http_server_middlewares.c++m | 18 +++++++--------- net/net-http_server_middlewares.test.c++ | 27 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/net/net-http_server_middlewares.c++m b/net/net-http_server_middlewares.c++m index aa1a234..e83a0eb 100644 --- a/net/net-http_server_middlewares.c++m +++ b/net/net-http_server_middlewares.c++m @@ -224,21 +224,19 @@ struct rate_limiter key_requests.end() ); - // Periodic cleanup: remove empty key entries to prevent memory leak + // Check / record before erase_if: std::flat_map erase invalidates all + // references (not just the erased key). Cleaning empty entries first + // left key_requests dangling on every 1000th call that removed anything. + const auto allow = key_requests.size() < max_requests; + if(allow) + key_requests.push_back(now); + if(++request_count % 1000 == 0) { std::erase_if(requests, [](const auto& pair) { return pair.second.empty(); }); } - // Check if limit exceeded - if(key_requests.size() >= max_requests) - { - return false; // Rate limit exceeded - } - - // Add current request - key_requests.push_back(now); - return true; // Within limit + return allow; } }; diff --git a/net/net-http_server_middlewares.test.c++ b/net/net-http_server_middlewares.test.c++ index 3e42fcc..bfaa595 100644 --- a/net/net-http_server_middlewares.test.c++ +++ b/net/net-http_server_middlewares.test.c++ @@ -770,6 +770,33 @@ auto register_middleware_tests() }; }; + // Regression: check_limit used to erase_if empty flat_map entries while still + // holding auto& into requests[key]. flat_map erase invalidates all references, + // so the subsequent size()/push_back was use-after-free on every 1000th call + // that removed at least one empty key. + tester::bdd::scenario("rate_limiter check_limit survives periodic empty-key cleanup, [net]") = [] { + tester::bdd::given("A limiter with one empty orphan key and 998 populated keys") = [] { + auto limiter = ::http::middleware::make_rate_limiter(); + const auto window = std::chrono::seconds{60}; + + // max_requests == 0 denies without push_back → leaves an empty map entry. + check_false(limiter->check_limit("orphan", 0, window)); + + for(int i = 0; i < 998; ++i) + check_true(limiter->check_limit(std::format("k{}", i), 100, window)); + + tester::bdd::when("The 1000th check_limit erases the orphan via flat_map cleanup") = [limiter, window] { + // request_count hits 1000 here; erase_if removes "orphan". + check_true(limiter->check_limit("victim", 100, window)); + + tester::bdd::then("Further checks on the victim key remain well-defined") = [limiter, window] { + check_true(limiter->check_limit("victim", 100, window)); + check_eq(limiter->requests.size(), 999uz); // 998 + victim; orphan gone + }; + }; + }; + }; + tester::bdd::scenario("metrics_middleware - records OK request and scrape body, [net]") = [] { tester::bdd::given("A metrics middleware with a successful handler") = [] { auto registry = ::http::middleware::metrics_registry{};