Merge TASK-016 review cleanup (24/50 fixed; 26 minors pending)

Author: etr

specs/unworked_review_issues/2026-05-04_233051_task-016.md

@@ -151,11 +151,15 @@ MHD_Result http_request_impl::build_request_args(void* cls, MHD_ValueKind kind,
     std::string_view key_sv(key);
     std::string_view val_sv((arg_value != nullptr) ? arg_value : "");
 
-    // Apply count limit: check how many unique keys exist so far.
+    // Hoist the find so we use the iterator both for the count guard and
+    // for the insert branch below -- eliminates the double lookup.
+    // (code-simplifier-iter1-1)
     auto& args = *aa->arguments;
-    const std::size_t new_unique =
-        (args.find(key_sv) == args.end()) ? 1u : 0u;
-    if (args.size() + new_unique > aa->max_args_count) {
+    auto existing_it = args.find(key_sv);
+
+    // Apply count limit: would this key add a new unique entry?
+    const bool is_new_key = (existing_it == args.end());
+    if (args.size() + (is_new_key ? 1u : 0u) > aa->max_args_count) {
         return MHD_NO;
     }
 
@@ -167,19 +171,29 @@ MHD_Result http_request_impl::build_request_args(void* cls, MHD_ValueKind kind,
     aa->accumulated_bytes += this_pair_bytes;
 
     // Unescape into a temporary std::string (the C-style unescaper is
-    // string-typed). The unescape itself touches the global heap if the
-    // key/value spill out of std::string's small-buffer; tracked by
-    // TASK-018 (move the unescape onto the arena too).
+    // string-typed). This causes one global-heap allocation (the temp) even
+    // on the warm arena path, followed by a second copy from the temp into
+    // the arena-backed pmr::string via emplace_back below. The warm-path
+    // zero-upstream-allocs guarantee therefore does NOT hold for requests
+    // with GET arguments when an unescaper is active.
+    //
+    // Tracked as TASK-018: route the unescape output directly into a
+    // pmr::string using the arena allocator, eliminating both the temp and
+    // the second copy. Until then, the acceptance criterion '0 bytes
+    // global-heap allocation from impl construction on warm connection'
+    // applies to construction and arena-backed containers only -- not to
+    // argument population when an unescaper is registered.
+    // (performance-reviewer-iter1-1: acknowledged deferral)
     std::string value(val_sv);
     http::base_unescaper(&value, aa->unescaper);
 
-    // Look up via heterogeneous string_view (no allocation), insert the
-    // key as pmr::string in the map's allocator domain on miss. The
-    // value vector is allocator-constructed in place via the same
-    // allocator (scoped propagation gives nested pmr::strings the
-    // right allocator too).
+    // Reuse the iterator from the hoisted find above: insert if missing,
+    // then append the (possibly unescaped) value. The key is stored as
+    // pmr::string in the map's allocator domain; the value vector is
+    // allocator-constructed in place via the same allocator (scoped
+    // propagation gives nested pmr::strings the right allocator too).
     auto pmr_alloc = args.get_allocator();
-    auto it = args.find(key_sv);
+    auto it = existing_it;
     if (it == args.end()) {
         std::pmr::vector<std::pmr::string> empty(pmr_alloc);
         auto inserted = args.emplace(

src/detail/http_request_impl.cpp

@@ -111,6 +111,11 @@ void run_post_processor_if_attached(modded_request* mr,
 }  // namespace
 
 MHD_Result webserver_impl::requests_answer_first_step(MHD_Connection* connection, struct detail::modded_request* mr) {
+    // The http_request constructor calls pick_resource(connection) internally
+    // to locate the per-connection arena installed by connection_notify via
+    // MHD_CONNECTION_INFO_SOCKET_CONTEXT, then allocates the http_request_impl
+    // from that arena. The arena plumbing is therefore implicit at this call
+    // site but explicit within the constructor. (spec-alignment-checker-iter1-2/3)
     mr->dhr.reset(new http_request(connection, parent->unescaper));
     mr->dhr->set_file_cleanup_callback(parent->file_cleanup_callback);
 

src/detail/webserver_body_pipeline.cpp

@@ -128,6 +128,14 @@ void webserver_impl::request_completed(void *cls, struct MHD_Connection *connect
     //     both atomically. The next request on this keep-alive connection
     //     reuses the same memory (verified by http_request_arena unit test).
     //
+    // Unconditional release is correct regardless of the `toe`
+    // (MHD_RequestTerminationCode) value: step (1) above always destroys
+    // the modded_request (and thus all arena-backed objects) before this
+    // point, so the arena holds no live objects for any termination code,
+    // including MHD_REQUEST_TERMINATED_WITH_ERROR. Resetting unconditionally
+    // is therefore both safe and necessary to prepare the arena for the next
+    // keep-alive request. (code-quality-reviewer-iter1-4)
+    //
     // MHD ordering guarantee: NOTIFY_COMPLETED always fires before
     // NOTIFY_CLOSED for the same connection (MHD documentation, section
     // "Thread model guarantees"). Therefore the connection_state pointer

src/detail/webserver_callbacks.cpp

@@ -46,11 +46,12 @@ namespace detail {
 // keep-alive connection, request_completed calls arena_.release() to
 // rewind the bump pointer, so a second request reuses the same memory.
 //
-// Lifetime contract for views returned by http_request getters: they
-// remain valid until the request-completion callback fires for the
+// Lifetime contract for views returned by http_request getters
+// (string_view, const& to pmr::string / pmr::vector / pmr::map members):
+// they remain valid until the request-completion callback fires for the
 // request they were derived from. Capturing them past the user
 // handler's return is undefined behavior. (See architecture doc
-// 04-components/http-request.md.)
+// 04-components/http-request.md.) (spec-alignment-checker-iter1-4)
 //
 // Initial-buffer sizing math (8 KiB):
 //   - sizeof(http_request_impl) ~= 600-700 B with libstdc++/libc++
@@ -96,10 +97,23 @@ struct connection_state {
     // Explicit zeroing after release() closes that residual-credential
     // window. (security-reviewer-iter1-3, CWE-226.)
     //
+    // Scope limitation: zeroing covers only initial_buffer_ (ARENA_INITIAL_BYTES).
+    // If a request's arena usage overflows the initial buffer, the monotonic_
+    // buffer_resource silently allocates additional blocks from the upstream
+    // resource (heap). Those overflow blocks are freed by arena_.release()
+    // but NOT zeroed here. Credentials that spill past ARENA_INITIAL_BYTES
+    // are therefore not cleared by this call. In practice the buffer is sized
+    // to hold typical requests without overflow (see sizing comment above);
+    // if sizing assumptions change, this limitation should be revisited.
+    // (code-quality-reviewer-iter1-5)
+    //
     // Using std::memset here (rather than explicit_bzero / SecureZeroMemory)
     // is acceptable because the buffer is accessed again immediately by the
     // next request's arena allocation, preventing the compiler from
-    // optimising the clear away as a dead store.
+    // optimising the clear away as a dead store. (security-reviewer-iter1-4,
+    // CWE-14 risk acknowledged; std::atomic_signal_fence or volatile
+    // initial_buffer_ would provide a language-level guarantee if needed
+    // by future deployments.)
     void reset_arena() noexcept {
         arena_.release();
         std::memset(initial_buffer_.data(), 0, ARENA_INITIAL_BYTES);

src/httpserver/detail/connection_state.hpp

@@ -281,11 +281,15 @@ class http_request_impl {
 //   max_args_bytes: maximum total key+value bytes accumulated before
 //     returning MHD_NO. Applies the same protection on a byte basis.
 //
-// Defaults are deliberately large (64 K / 64 KiB) so existing callers
-// that construct the accumulator without setting these fields remain
-// compatible. The webserver hot path sets these from connection_state
-// or a compile-time constant once the create_webserver API exposes them
-// (TODO(M5)).
+// Defaults are deliberately generous (64 unique keys / 64 KiB total bytes)
+// so existing callers that construct the accumulator without setting these
+// fields remain compatible. The webserver hot path sets these from
+// connection_state or a compile-time constant once the create_webserver
+// API exposes them (TODO(M5)).
+//
+// NOTE: POST argument limits are handled upstream by MHD_OPTION_CONNECTION_
+// MEMORY_LIMIT; the guards here apply only to GET arguments processed via
+// build_request_args / populate_args. (security-reviewer-iter1-5)
 struct arguments_accumulator {
     unescaper_ptr unescaper = nullptr;
     // TASK-016: points at the impl's pmr-backed map.

src/httpserver/detail/http_request_impl.hpp

@@ -21,6 +21,7 @@
 //      on the warm path -- after the first request grew the arena, the
 //      second request's upstream alloc count stays flat.
 
+#include <algorithm>
 #include <cstddef>
 #include <cstdint>
 #include <cstring>
@@ -64,6 +65,10 @@ LT_END_SUITE(http_request_arena_suite)
 // (1) connection_state must own a std::pmr::monotonic_buffer_resource named
 //     arena_, and arena_.release() must rewind the bump pointer so a second
 //     allocation lands at the same address as the first.
+//
+//     Note: impl_address_reuse_after_release (test 3) extends this contract
+//     to http_request_impl construction, so the split between these two
+//     tests is intentional rather than redundant. (test-quality-reviewer-iter1-5)
 LT_BEGIN_AUTO_TEST(http_request_arena_suite, arena_release_resets_bump_pointer)
     httpserver::detail::connection_state cs;
 
@@ -94,7 +99,13 @@ LT_BEGIN_AUTO_TEST(http_request_arena_suite, warm_path_zero_upstream_allocs)
     using httpserver::detail::http_request_impl;
     using impl_alloc_t = std::pmr::polymorphic_allocator<http_request_impl>;
 
-    // First request: grows the arena (consumes some of the 8 KiB).
+    // First request (cold path): grows the arena (consumes some of the 8 KiB).
+    // The baseline is sampled AFTER the cold cycle so the warm-path assertion
+    // below measures only warm-path allocations. If the 8 KiB buffer is too
+    // small for the cold cycle, the cold cycle will spill to upstream but
+    // baseline will be non-zero, and the warm-path assertion would become
+    // vacuous. The warm_path_zero_upstream_allocs_with_containers test (7)
+    // asserts baseline == 0 there to guard against this. (test-quality-iter1-6)
     {
         impl_alloc_t alloc(&arena);
         auto* p = alloc.new_object<http_request_impl>(nullptr, nullptr, alloc);
@@ -244,14 +255,9 @@ LT_BEGIN_AUTO_TEST(http_request_arena_suite, reset_arena_clears_initial_buffer)
     cs.reset_arena();
 
     // After reset, the bytes at that location must be zero.
-    bool all_zero = true;
-    for (std::size_t i = 0; i < sentinel_size; ++i) {
-        if (alloc_ptr[i] != std::byte{0}) {
-            all_zero = false;
-            break;
-        }
-    }
-    LT_CHECK(all_zero);
+    // (test-quality-reviewer-iter1-4: use std::all_of for a cleaner assertion)
+    LT_CHECK(std::all_of(alloc_ptr, alloc_ptr + sentinel_size,
+                         [](std::byte b) { return b == std::byte{0}; }));
 
     // Verify the arena is also released (bump pointer rewound): a new
     // allocation of the same size must land at the same address.
@@ -268,6 +274,13 @@ LT_END_AUTO_TEST(reset_arena_clears_initial_buffer)
 //     test only exercised construction with a null connection (no container
 //     population), so it did not validate the acceptance criterion for a
 //     request that actually populates the arena-backed containers.
+//
+//     code-quality-reviewer-iter1-3 / spec-alignment-checker-iter1-7:
+//     The baseline is asserted to be zero to confirm the cold cycle itself
+//     fit within the 8 KiB initial buffer (no spill to upstream). If the
+//     buffer is ever too small, the cold-cycle spill would be silently
+//     included in the baseline and the warm-path assertion would still
+//     pass -- making the test vacuous.
 LT_BEGIN_AUTO_TEST(http_request_arena_suite, warm_path_zero_upstream_allocs_with_containers)
     assert_no_upstream_resource upstream;
 
@@ -307,13 +320,69 @@ LT_BEGIN_AUTO_TEST(http_request_arena_suite, warm_path_zero_upstream_allocs_with
     arena.release();
     const std::size_t baseline = upstream.upstream_alloc_count();
 
+    // Baseline must be zero: the 8 KiB initial buffer must be sufficient to
+    // hold all cold-cycle allocations. If this fails, the buffer is too small
+    // and the warm-path assertion below would be vacuous (the "baseline" would
+    // absorb cold-cycle spills and the warm path would appear zero-delta even
+    // if it also spills). (code-quality-reviewer-iter1-3)
+    LT_CHECK_EQ(baseline, std::size_t{0});
+
     // Warm cycle: reuses the arena's initial buffer -- upstream must stay flat.
     one_request_cycle();
     arena.release();
 
     LT_CHECK_EQ(upstream.upstream_alloc_count(), baseline);
 LT_END_AUTO_TEST(warm_path_zero_upstream_allocs_with_containers)
 
+// (6) Simulate the request_completed trampoline contract: destroy the
+//     arena-backed impl (running its destructor), then call reset_arena()
+//     on the connection_state, and verify the arena is ready for reuse at
+//     the same address. This tests the two-step sequence in
+//     webserver_impl::request_completed without requiring a live MHD daemon.
+//
+//     Acceptance criterion: 'MHD_RequestTerminationCode callback resets
+//     the arena (test)'. The structural tests (1) and (3) verify the
+//     bump-pointer rewind property; this test verifies the ordering contract
+//     between ~http_request_impl (step 1) and reset_arena() (step 2) by
+//     running them in the same order request_completed does.
+//     (code-quality-reviewer-iter1-2 / test-quality-reviewer-iter1-1)
+LT_BEGIN_AUTO_TEST(http_request_arena_suite, request_completed_trampoline_contract)
+    using httpserver::detail::http_request_impl;
+    using impl_alloc_t = std::pmr::polymorphic_allocator<http_request_impl>;
+
+    httpserver::detail::connection_state cs;
+
+    // Step 1 (mirrors request_completed): allocate and construct an
+    // http_request_impl from the connection_state's arena, populate
+    // a couple of args to give it live PMR state, then call its
+    // destructor via delete_object (mirrors the arena_deleter: destructor
+    // only, no deallocation). This must not touch the arena bump pointer.
+    impl_alloc_t alloc(&cs.arena_);
+    auto* p = alloc.new_object<http_request_impl>(nullptr, nullptr, alloc);
+
+    constexpr std::size_t limit = 1024;
+    p->set_arg("user", "alice", limit);
+    p->querystring = "?user=alice";
+    p->requestor_ip = "10.0.0.1";
+
+    // Capture the address before destruction so we can verify reuse.
+    const void* const first_addr = static_cast<void*>(p);
+
+    // Destroy the impl without deallocating (mirrors destroy_impl_arena).
+    p->~http_request_impl();
+
+    // Step 2 (mirrors request_completed): rewind and zero the arena.
+    cs.reset_arena();
+
+    // After reset, a new allocation of the same type must land at the same
+    // address (bump pointer rewound to the start of the initial buffer).
+    auto* p2 = alloc.new_object<http_request_impl>(nullptr, nullptr, alloc);
+    const void* const second_addr = static_cast<void*>(p2);
+    alloc.delete_object(p2);
+
+    LT_CHECK_EQ(first_addr, second_addr);
+LT_END_AUTO_TEST(request_completed_trampoline_contract)
+
 LT_BEGIN_AUTO_TEST_ENV()
     AUTORUN_TESTS()
 LT_END_AUTO_TEST_ENV()