|
| 1 | +# Unworked Review Issues |
| 2 | + |
| 3 | +**Run:** 2026-05-04 23:25:02 |
| 4 | +**Task:** TASK-016 |
| 5 | +**Total:** 13 (0 critical fixed-deferred, 2 major deferred, 9 minor) |
| 6 | + |
| 7 | +## Critical / Major Deferred (acknowledged scope deferrals) |
| 8 | + |
| 9 | +1. [ ] **performance-reviewer** | `src/http_request.cpp:217` | memory-allocation |
| 10 | + In build_request_args, the argument value is copied into a temporary std::string on the global heap on every argument key-value pair before unescaping. This is a warm-path allocation that defeats the stated acceptance criterion of 0 bytes global-heap allocation for requests with GET arguments. The comment in the code acknowledges this as tracked by TASK-018. |
| 11 | + *Recommendation:* TASK-018 should inline the unescape into an arena-backed pmr::string directly, eliminating the temporary. This is a deliberate deferral accepted for TASK-016 scope. |
| 12 | + |
| 13 | +2. [ ] **performance-reviewer** | `src/http_request.cpp:622` | memory-allocation |
| 14 | + get_path_pieces() allocates a new default-allocator std::vector<std::string> on every call, copying each pmr::string element from the arena-backed path_pieces cache. This is a measurable regression relative to a const-reference return on a hot path. The code comment acknowledges this as TASK-017 scope. |
| 15 | + *Recommendation:* TASK-017 should change the return type to const& aliasing the impl-side pmr::vector<pmr::string> storage, eliminating the copy. Deliberate deferral for TASK-016 scope. |
| 16 | + |
| 17 | +## Minor |
| 18 | + |
| 19 | +3. [ ] **security-reviewer** | `src/http_request.cpp:560` | insecure-design |
| 20 | + pick_resource() silently falls back to new_delete_resource() if MHD_CONNECTION_INFO_SOCKET_CONTEXT returns null or if socket_context is null. A whole-server misconfiguration causes every request to fall back to heap allocation with no observable error. The debug-mode counter added in TASK-016 makes this observable in development but is silent in production. |
| 21 | + *Recommendation:* Consider a diagnostic API to expose g_arena_fallback_count to integration tests and monitoring. In production the fallback is safe, but the silence makes integration bugs hard to diagnose. |
| 22 | + |
| 23 | +4. [ ] **security-reviewer** | `src/http_request.cpp:538` | insecure-design |
| 24 | + destroy_impl_arena calls p->~http_request_impl() but the ordering in request_completed is load-bearing: (1) delete modded_request then (2) arena_.release() (now reset_arena()). If an exception or early return in the future skips step (2), arena memory is never released. The correctness of the sequence is undocumented in destroy_impl_arena itself. |
| 25 | + *Recommendation:* Add a comment inside request_completed at the reset_arena() call site that explicitly links it to the preceding delete (step 1 must complete before step 2). Consider a scope guard (ScopeExit) pattern around the two-step sequence to guarantee reset_arena() fires even if delete throws. |
| 26 | + |
| 27 | +5. [ ] **performance-reviewer** | `src/http_request.cpp:293` | memory-allocation |
| 28 | + ensure_path_pieces_cached calls http::http_utils::tokenize_url(std::string(path)) which allocates a default-allocator std::vector<std::string> of tokens and then copies them element-wise into the pmr-backed path_pieces. The intermediate std::string(path) and tokenize_url's returned vector are global-heap transient allocations on the first call of get_path_pieces/get_path_piece on a warm request. |
| 29 | + *Recommendation:* Overload tokenize_url to write directly into a pmr::vector<pmr::string>. Acceptable to defer to TASK-017/018. |
| 30 | + |
| 31 | +6. [ ] **performance-reviewer** | `src/http_request.cpp:256` | memory-allocation |
| 32 | + build_request_querystring calls qs->reserve() once per parameter. For many small parameters, a single upfront reserve with the total expected size would be more efficient than repeated reserve calls. |
| 33 | + *Recommendation:* Minor. Current approach is O(n log n) amortized due to doubling. Pre-counting parameters and reserving upfront (or using a larger initial capacity) would help on a warm path with many small parameters. |
| 34 | + |
| 35 | +7. [ ] **performance-reviewer** | `test/unit/http_request_arena_test.cpp:85` | missing-caching |
| 36 | + The warm_path_zero_upstream_allocs test constructs with null connection — the lazy-cache MHD paths are not exercised (populate_args, build_request_querystring, etc. return early). A new test warm_path_zero_upstream_allocs_with_containers was added to cover the direct container-population path (set_arg, querystring =, ensure_path_pieces_cached). The MHD-driven population path (build_request_args via MHD_get_connection_values with a mock connection) is still not covered. |
| 37 | + *Recommendation:* Add a test that mocks MHD_get_connection_values to drive build_request_args through the arena path and verifies the upstream counter stays flat on the warm pass. Requires MHD mock infrastructure not currently present in the test harness. |
| 38 | + |
| 39 | +8. [ ] **performance-reviewer** | `src/http_request.cpp:578` | memory-allocation |
| 40 | + pick_resource() calls MHD_get_connection_info on every http_request construction. In production with a properly wired connection_notify, this is a single pointer lookup but still an MHD API call on every request. The g_arena_fallback_count counter (added in TASK-016) only increments in debug builds; production has no observable signal. |
| 41 | + *Recommendation:* Acceptable for TASK-016 scope. If profiling shows MHD_get_connection_info is a bottleneck, caching the arena pointer directly in the modded_request would eliminate the call. |
| 42 | + |
| 43 | +9. [ ] **security-reviewer** | `src/httpserver/detail/webserver_impl.hpp:107` | memory-disclosure |
| 44 | + reset_arena() uses std::memset to zero the initial_buffer_ after release(). Unlike explicit_bzero / SecureZeroMemory, a sufficiently aggressive compiler may optimise away a memset on a buffer that it can prove is never read afterwards in the same scope. However, since the buffer is a class member reused in subsequent requests, the write is not dead-store-optimisable in practice. The comment in reset_arena() documents this rationale. |
| 45 | + *Recommendation:* If the threat model requires compiler-proof zeroing (e.g., for FIPS compliance), replace std::memset with explicit_bzero (POSIX) or SecureZeroMemory (Windows). The current implementation is adequate for the stated threat model. |
| 46 | + |
| 47 | +10. [ ] **security-reviewer** | `src/httpserver/detail/webserver_impl.hpp:103` | denial-of-service |
| 48 | + The monotonic_buffer_resource overflow path spills to new_delete_resource() with no upper bound. With ARENA_INITIAL_BYTES now 8 KiB (up from 4 KiB), overflow is less likely for typical requests, but a crafted request with many large headers or a large requestor IP can still cause heap overflow blocks. arena_.release() (now reset_arena()) does NOT reclaim heap blocks allocated by the overflow path -- they leak to the upstream new_delete_resource on keep-alive connections. |
| 49 | + *Recommendation:* Document in connection_state comment that arena overflow blocks are not reclaimed by reset_arena() (only the initial 8 KiB is rewound+zeroed). The arg-count/bytes guard added in TASK-016 mitigates GET argument exhaustion; header overflow remains unguarded. Consider TASK-018 or later adding a guard on MHD_HEADER_KIND iteration. |
| 50 | + |
| 51 | +11. [ ] **performance-reviewer** | `test/unit/http_request_arena_test.cpp:85` | missing-caching |
| 52 | + The warm_path_zero_upstream_allocs test baseline is taken after the first cold cycle and arena release. If ARENA_INITIAL_BYTES (now 8 KiB) is exactly right, the warm cycle generates zero upstream allocs. But if any allocation in the cold cycle spilled to heap (overflow block), the baseline already includes those blocks, masking the overflow from the warm-cycle assertion. The test passes but doesn't prove the initial buffer is large enough. |
| 53 | + *Recommendation:* Check that the baseline after the cold cycle is 0 upstream allocations (i.e., the cold cycle itself fit in the initial buffer). Add LT_CHECK_EQ(upstream.upstream_alloc_count(), 0) before the arena.release() and baseline capture. This was not addressed in TASK-016 to avoid over-constraining the test. |
| 54 | + |
| 55 | +12. [ ] **security-reviewer** | `src/httpserver/detail/webserver_impl.hpp:107` | memory-disclosure |
| 56 | + The initial_buffer_ value-initialization ({}) zeroes the buffer at construction (via connection_state constructor), but reset_arena() now explicitly zeroes it again on every request_completed. On a connection's very first request, reset_arena() is not called before the first request starts -- only after it completes. Credentials written by the first request are cleared before the second request starts. The zero-at-construction covers the interval before any request arrives. |
| 57 | + *Recommendation:* No action needed. The value-initialization zero and the reset_arena() clear form a complete credential-protection sequence with no gap. |
| 58 | + |
| 59 | +13. [ ] **performance-reviewer** | `src/httpserver/detail/webserver_impl.hpp:103` | memory-allocation |
| 60 | + ARENA_INITIAL_BYTES was increased from 4 KiB to 8 KiB (performance-reviewer-iter1-1). Per-connection RSS cost is now 8 KiB * N concurrent connections. For a server with 10,000 concurrent keep-alive connections this is 80 MiB reserved for arenas. This is likely acceptable but has not been profiled. |
| 61 | + *Recommendation:* TODO(M5): expose ARENA_INITIAL_BYTES via create_webserver so production deployments can tune it without recompiling. The TODO comment in webserver_impl.hpp tracks this. |
0 commit comments