Merge TASK-027 majors+minors (37/59 resolved, 22 deferred)

Author: etr

specs/unworked_review_issues/2026-05-10_230348_task-027.md

@@ -98,7 +98,7 @@ void webserver_impl::invalidate_route_cache() {
     // standardized_url only; v2's is keyed on (method, path). A
     // registration may invalidate both, so clear both atomically.
     {
-        std::lock_guard<std::mutex> lock(route_cache_mutex);
+        std::lock_guard<std::mutex> lock(route_cache_mutex_);
         route_cache_list.clear();
         route_cache_map.clear();
     }
@@ -202,11 +202,14 @@ webserver_impl::lookup_v2(http_method method, const std::string& path) {
         }
     }  // table_lock released.
 
-    // Step 3: install into cache (cache mutex only).
+    // Step 3: install into cache (cache mutex only). Move result.entry and
+    // result.captured_params into the cache_value to avoid a second copy
+    // of the shared_ptr ref-count bump and captured vector on the miss path.
+    // result is not used after this point.
     if (result.found) {
         cache_value v;
-        v.entry = result.entry;
-        v.captured_params = result.captured_params;
+        v.entry = std::move(result.entry);
+        v.captured_params = std::move(result.captured_params);
         route_cache_v2.insert(key, std::move(v));
     }
 
@@ -219,7 +222,7 @@ namespace detail {
 
 std::optional<webserver_impl::regex_route_lookup>
 webserver_impl::lookup_route_cache(const std::string& key) {
-    std::lock_guard<std::mutex> cache_lock(route_cache_mutex);
+    std::lock_guard<std::mutex> cache_lock(route_cache_mutex_);
     auto cache_it = route_cache_map.find(key);
     if (cache_it == route_cache_map.end()) {
         return std::nullopt;
@@ -265,7 +268,7 @@ webserver_impl::scan_regex_routes(const detail::http_endpoint& target) {
 void webserver_impl::store_route_cache(const std::string& key,
                                        const detail::http_endpoint& matched,
                                        std::shared_ptr<http_resource> hrm) {
-    std::lock_guard<std::mutex> cache_lock(route_cache_mutex);
+    std::lock_guard<std::mutex> cache_lock(route_cache_mutex_);
     route_cache_list.emplace_front(key, route_cache_entry{matched, std::move(hrm)});
     route_cache_map[key] = route_cache_list.begin();
     if (route_cache_map.size() > ROUTE_CACHE_MAX_SIZE) {

src/detail/webserver_dispatch.cpp

@@ -204,28 +204,24 @@ void webserver::register_resource(const std::string& resource,
 //   1. Validates the handler, the method set (non-empty, no count_
 void webserver::unregister_impl_(const string& resource, bool family) {
     detail::http_endpoint he(resource, family, true, regex_checking);
-    std::unique_lock registered_resources_lock(impl_->registered_resources_mutex);
 
     {
-        std::lock_guard<std::mutex> cache_lock(impl_->route_cache_mutex);
-        impl_->route_cache_list.clear();
-        impl_->route_cache_map.clear();
-    }
-
-    impl_->registered_resources.erase(he);
-    impl_->registered_resources_regex.erase(he);
-    // The string-keyed fast-path map only ever holds exact (non-family)
-    // entries (see register_impl_). A prefix unregister has nothing to
-    // do here.
-    if (!family) {
-        impl_->registered_resources_str.erase(he.get_url_complete());
+        std::unique_lock registered_resources_lock(impl_->registered_resources_mutex);
+        impl_->registered_resources.erase(he);
+        impl_->registered_resources_regex.erase(he);
+        // The string-keyed fast-path map only ever holds exact (non-family)
+        // entries (see register_impl_). A prefix unregister has nothing to
+        // do here.
+        if (!family) {
+            impl_->registered_resources_str.erase(he.get_url_complete());
+        }
     }
 
     // TASK-027: mirror the erasure into the v2 3-tier table. Lock order:
-    // we already hold registered_resources_lock (v1 table); take
-    // route_table_mutex_ next, then route_cache_mutex_ via the
-    // route_cache::clear() helper. The discipline (table BEFORE cache)
-    // is consistent with register_impl_ and the documented invariant.
+    // registered_resources_mutex released above; take route_table_mutex_
+    // next, then invalidate_route_cache() takes route_cache_mutex_.
+    // This matches the pattern used in register_impl_ / on_methods_:
+    // table lock released before cache is cleared.
     {
         std::unique_lock table_lock(impl_->route_table_mutex_);
         const std::string& key = he.get_url_complete();
@@ -245,7 +241,7 @@ void webserver::unregister_impl_(const string& resource, bool family) {
                 impl_->regex_routes_.end());
         }
     }
-    impl_->route_cache_v2.clear();
+    impl_->invalidate_route_cache();
 }
 
 void webserver::unregister_path(const string& path) {
@@ -261,24 +257,19 @@ void webserver::unregister_resource(const string& resource) {
     detail::http_endpoint he_exact(resource, /*family=*/false, true, regex_checking);
     detail::http_endpoint he_prefix(resource, /*family=*/true,  true, regex_checking);
 
-    // Hold a single write-lock across both erasures so no request thread can
-    // observe a partially-unregistered state (CWE-367 TOCTOU fix: the exact
-    // entry and the prefix entry are removed atomically).
-    std::unique_lock registered_resources_lock(impl_->registered_resources_mutex);
-
     {
-        std::lock_guard<std::mutex> cache_lock(impl_->route_cache_mutex);
-        impl_->route_cache_list.clear();
-        impl_->route_cache_map.clear();
+        // Hold a single write-lock across both v1 erasures so no request thread
+        // can observe a partially-unregistered state (CWE-367 TOCTOU fix: the
+        // exact entry and the prefix entry are removed atomically).
+        std::unique_lock registered_resources_lock(impl_->registered_resources_mutex);
+        impl_->registered_resources.erase(he_exact);
+        impl_->registered_resources.erase(he_prefix);
+        impl_->registered_resources_regex.erase(he_exact);
+        impl_->registered_resources_regex.erase(he_prefix);
+        // The string-keyed fast-path map only holds exact (non-family) entries.
+        impl_->registered_resources_str.erase(he_exact.get_url_complete());
     }
 
-    impl_->registered_resources.erase(he_exact);
-    impl_->registered_resources.erase(he_prefix);
-    impl_->registered_resources_regex.erase(he_exact);
-    impl_->registered_resources_regex.erase(he_prefix);
-    // The string-keyed fast-path map only holds exact (non-family) entries.
-    impl_->registered_resources_str.erase(he_exact.get_url_complete());
-
     // TASK-027: mirror into the v2 3-tier table. Erase under both
     // classifications so a prior register_path AND register_prefix on
     // the same path are both cleared atomically.
@@ -297,7 +288,10 @@ void webserver::unregister_resource(const string& resource) {
                            }),
             impl_->regex_routes_.end());
     }
-    impl_->route_cache_v2.clear();
+    // Delegate cache clearing to invalidate_route_cache() matching the
+    // pattern used by register_impl_ and on_methods_ (table lock released
+    // before cache is cleared).
+    impl_->invalidate_route_cache();
 }
 
 // TASK-029: The v2.0 public IP-control API is the pair block_ip / unblock_ip.

src/detail/webserver_register.cpp

@@ -93,6 +93,8 @@ using httpserver::http::ip_representation;
 using httpserver::http::base_unescaper;
 
 
+namespace detail {
+
 namespace {
 
 // Apply one path segment to the running stack: "" / "." are skipped,
@@ -108,9 +110,13 @@ void apply_normalized_segment(std::vector<std::string>& segments,
     segments.push_back(seg);
 }
 
-}  // namespace
-
-static std::string normalize_path(const std::string& path) {
+// NOTE: the caller (should_skip_auth) must receive an already-unescaped
+// path (i.e., no %XX sequences remain). MHD's unescaper_func runs before
+// should_skip_auth, so this invariant is satisfied on the dispatch path.
+// Double slashes (//) and trailing slashes are collapsed automatically:
+// apply_normalized_segment skips empty segment strings, so consecutive
+// '/' separators produce zero segments (same as a single '/').
+std::string normalize_path(const std::string& path) {
     std::vector<std::string> segments;
     std::string::size_type start = 0;
     if (!path.empty() && path[0] == '/') start = 1;
@@ -128,7 +134,7 @@ static std::string normalize_path(const std::string& path) {
     return normalized;
 }
 
-namespace detail {
+}  // namespace
 
 bool webserver_impl::should_skip_auth(const std::string& path) const {
     std::string normalized = normalize_path(path);

src/detail/webserver_request.cpp

@@ -209,8 +209,9 @@ void webserver_impl::insert_fresh_v2_entry(const detail::http_endpoint& idx,
     auto tier = classify_route_tier(idx);
     switch (tier.kind) {
     case route_tier_kind::radix:
-        // Unreachable: callers route url_pars-non-empty through
-        // upsert_v2_param_route, never here.
+        // Unreachable: upsert_v2_table_entry routes url_pars-non-empty paths
+        // through upsert_v2_param_route before calling insert_fresh_v2_entry.
+        __builtin_unreachable();
         break;
     case route_tier_kind::exact: {
         detail::route_entry entry;

src/detail/webserver_routes.cpp

@@ -20,12 +20,11 @@
 
 // TASK-027: segment-trie for the parameterized + prefix route tier.
 //
-// The architecture spec (§4.7) commits only to the OUTER shape (three-tier
-// + cache); the radix-tree implementation choice is intentionally left
-// open. A segment trie is sufficient for the 9-method / N-segment
-// registration shape libhttpserver supports and avoids dragging in a
-// vendored library (which would conflict with the project's tightly
-// curated source tree and LGPL-2.1 distribution).
+// A segment trie avoids dragging in a vendored library (which would
+// conflict with the project's tightly curated source tree and
+// LGPL-2.1 distribution). The architecture spec (§4.7) commits only
+// to the outer three-tier + cache shape; the implementation choice
+// is intentionally left open.
 //
 // Internal header — only reachable when compiling libhttpserver.
 #if !defined(HTTPSERVER_COMPILATION)
@@ -49,28 +48,19 @@
 namespace httpserver {
 namespace detail {
 
-// radix_match: result type of radix_tree<T>::find. `entry` is a non-owning
-// pointer into the tree; valid until the next mutation. `captures` lists
-// (parameter-name, captured-value) pairs in the order the wildcards
-// appear along the matched path. `is_prefix_match` is true iff the match
-// came from a `is_prefix=true` registration that did not consume every
-// remaining request segment.
+// radix_match: result type of radix_tree<T>::find.
+// `entry` is a non-owning pointer into the tree; valid until the next mutation.
 template <typename T>
 struct radix_match {
     const T* entry = nullptr;
     std::vector<std::pair<std::string, std::string>> captures;
     bool is_prefix_match = false;
 };
 
-// Single trie node. Children are split into:
-//   - `children_`: keyed by the literal segment string (exact match).
-//   - `wildcard_child_`: optional single child consuming any one segment.
-//
-// Each node may carry an `exact_terminus_` (registration with is_prefix=false
-// that ends here) and/or a `prefix_terminus_` (is_prefix=true). The two
-// are kept separately because a prefix and an exact registration may both
-// terminate at the same node (e.g. /static prefix + /static exact would be
-// a user error caught at registration time, but the storage allows it).
+// Single trie node. `wildcard_child_` is a single optional pointer (not
+// another map entry) because there can be at most one unnamed wildcard per
+// path level by design — two {name} siblings at the same depth are
+// ambiguous and rejected at registration time.
 template <typename T>
 struct radix_node {
     std::unordered_map<std::string, std::unique_ptr<radix_node>> children_;
@@ -221,6 +211,9 @@ class radix_tree {
 
     // Remove the entry at `path`. is_prefix selects which terminus to
     // clear. Returns true iff a terminus was actually cleared.
+    // NOTE: unlike find(), where descent uses the concrete request-path
+    // segment value (e.g. "42"), remove() receives the registered pattern
+    // (e.g. "{id}") and matches wildcard nodes by the placeholder shape.
     bool remove(std::string_view path, bool is_prefix) {
         radix_node<T>* node = root_.get();
         const auto segments = tokenize(path);

src/httpserver/detail/radix_tree.hpp

@@ -45,6 +45,7 @@
 #include <functional>
 #include <list>
 #include <mutex>
+#include <stdexcept>
 #include <string>
 #include <string_view>
 #include <unordered_map>
@@ -69,11 +70,13 @@ struct cache_key {
 };
 
 struct cache_key_hash {
+    // Golden-ratio mix constant: reduces hash clustering vs. plain XOR.
+    static constexpr std::size_t kHashMix = 0x9e3779b97f4a7c15ULL;
+
     std::size_t operator()(const cache_key& k) const noexcept {
         std::size_t h1 = std::hash<std::string>{}(k.path);
         std::size_t h2 = static_cast<std::size_t>(k.method);
-        // boost::hash_combine constant.
-        return h1 ^ (h2 + 0x9e3779b97f4a7c15ULL + (h1 << 6) + (h1 >> 2));
+        return h1 ^ (h2 + kHashMix + (h1 << 6) + (h1 >> 2));
     }
 };
 
@@ -97,7 +100,12 @@ struct cache_value {
 class route_cache {
  public:
     explicit route_cache(std::size_t max_entries = 256)
-        : max_entries_(max_entries) {}
+        : max_entries_(max_entries) {
+        if (max_entries == 0) {
+            throw std::invalid_argument(
+                "route_cache max_entries must be > 0");
+        }
+    }
 
     // Find by key; returns true on hit and copies the value into `out`.
     // Promotes the hit to the front of the LRU list as a side effect.
@@ -124,7 +132,7 @@ class route_cache {
         // Compute the same hash as cache_key_hash without owning `path`.
         std::size_t h1 = std::hash<std::string_view>{}(path);
         std::size_t h2 = static_cast<std::size_t>(method);
-        std::size_t bucket_hash = h1 ^ (h2 + 0x9e3779b97f4a7c15ULL
+        std::size_t bucket_hash = h1 ^ (h2 + cache_key_hash::kHashMix
                                         + (h1 << 6) + (h1 >> 2));
         std::lock_guard<std::mutex> lock(mutex_);
         // Walk the bucket manually via equal_range on the raw bucket index.
@@ -163,13 +171,15 @@ class route_cache {
         }
     }
 
-    void clear() noexcept {
+    // Note: not noexcept — std::lock_guard can throw std::system_error
+    // (though in practice it does not on POSIX under normal conditions).
+    void clear() {
         std::lock_guard<std::mutex> lock(mutex_);
         map_.clear();
         list_.clear();
     }
 
-    std::size_t size() const noexcept {
+    std::size_t size() const {
         std::lock_guard<std::mutex> lock(mutex_);
         return map_.size();
     }

src/httpserver/detail/route_cache.hpp

@@ -30,6 +30,7 @@
 #include <optional>
 // Disabling lint error on regex (the only reason it errors is because the Chromium team prefers google/re2)
 #include <regex>  // NOLINT [build/c++11]
+#include <stdexcept>
 
 #include "httpserver/detail/http_endpoint.hpp"
 
@@ -78,8 +79,18 @@ inline route_tier_result classify_route_tier(const detail::http_endpoint& idx) {
         // pattern is trivially ^/literal$ and the exact hash tier is
         // faster and correct. Otherwise the path has meaningful regex
         // metacharacters and belongs in the regex tier.
-        std::regex re(idx.get_url_normalized(),
-                      std::regex::extended | std::regex::icase);
+        // Guard std::regex construction (CWE-248): a malformed pattern
+        // throws std::regex_error. Convert to std::invalid_argument so
+        // callers get a catchable typed exception at registration time.
+        std::regex re;
+        try {
+            re = std::regex(idx.get_url_normalized(),
+                            std::regex::extended | std::regex::icase);
+        } catch (const std::regex_error& e) {
+            throw std::invalid_argument(
+                std::string("invalid regex route pattern '")
+                + idx.get_url_normalized() + "': " + e.what());
+        }
         if (std::regex_match(idx.get_url_complete(), re)) {
             res.kind = route_tier_kind::exact;
         } else {