TASK-008: review-pass fixes (security + performance iter1)

Applies fixes from the iter1 review pass on the detail::body hierarchy:

file_body (CWE-367 / perf):
- Open + fstat moved to constructor; size() is now accurate immediately.
- Drops lseek(SEEK_END); materialize() uses st_size from fstat.
  Closes the TOCTOU window between size discovery and the fd handed
  to MHD_create_response_from_fd, and removes the side-effect on the
  fd's read position.
- Adds destructor that closes fd_ only when MHD never took ownership
  (materialized_ stays false until from_fd returns non-null).

deferred_body (CWE-476):
- trampoline() guards against null cls and empty producer_ before
  invoking the std::function. MHD's callback path doesn't catch C++
  exceptions, so a bad_function_call would terminate in MHD's IO
  thread; the guard returns MHD_CONTENT_READER_END_WITH_ERROR instead.
- Constructor asserts producer_ is non-empty (debug-only precondition).

Header docs:
- file_body: documents path-canonicalisation contract (O_NOFOLLOW only
  blocks the final component) and fd ownership lifecycle.
- iovec_body: documents the borrowed-pointer lifetime contract
  (iov_base buffers must outlive the MHD_Response*) and the heap
  allocation note from DR-005.
- deferred_body: documents the std::function SBO caveat — capturing
  more than the implementation-defined threshold silently heap-allocates.

Tests:
- file_body_size_known_before_materialize: size() must be correct at
  construction (21 bytes for test_content), not only after materialize.
- deferred_body_trampoline_null_cls_returns_error: trampoline with
  cls==nullptr returns MHD_CONTENT_READER_END_WITH_ERROR rather than
  dereferencing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: etr

src/details/body.cpp

@@ -102,35 +102,54 @@ MHD_Response* string_body::materialize() {
 }
 
 // ---------------------------------------------------------------------------
-// file_body — replicates v1 file_response::get_raw_response exactly.
+// file_body — opens the file and fstat's it at construction so size() is
+// accurate immediately.  materialize() uses fstat's st_size; it never calls
+// lseek(), so the fd's read position remains at 0 when handed to
+// MHD_create_response_from_fd (security-reviewer-iter1-1 / CWE-367).
 // ---------------------------------------------------------------------------
-MHD_Response* file_body::materialize() {
+file_body::file_body(std::string path) noexcept
+    : path_(std::move(path)) {
 #ifndef _WIN32
-    int fd = ::open(path_.c_str(), O_RDONLY | O_NOFOLLOW);
+    fd_ = ::open(path_.c_str(), O_RDONLY | O_NOFOLLOW);
 #else
-    int fd = ::open(path_.c_str(), O_RDONLY);
+    fd_ = ::open(path_.c_str(), O_RDONLY);
 #endif
-    if (fd == -1) return nullptr;
+    if (fd_ == -1) return;
 
     struct stat sb;
-    if (::fstat(fd, &sb) != 0 || !S_ISREG(sb.st_mode)) {
-        ::close(fd);
-        return nullptr;
+    if (::fstat(fd_, &sb) != 0 || !S_ISREG(sb.st_mode)) {
+        ::close(fd_);
+        fd_ = -1;
+        return;
     }
 
-    off_t size = ::lseek(fd, 0, SEEK_END);
-    if (size == static_cast<off_t>(-1)) {
-        ::close(fd);
-        return nullptr;
+    // Use fstat's st_size directly — no lseek, no TOCTOU, no fd-position
+    // side-effect (security-reviewer-iter1-1 / performance-reviewer-iter1-4).
+    size_ = static_cast<std::size_t>(sb.st_size);
+}
+
+file_body::~file_body() {
+    // Close only if MHD never took ownership (materialized_ stays false until
+    // MHD_create_response_from_fd returns non-null).
+    if (!materialized_ && fd_ != -1) {
+        ::close(fd_);
     }
+}
 
-    if (size) {
-        size_cached_ = static_cast<std::size_t>(size);
-        return MHD_create_response_from_fd(
-            static_cast<std::size_t>(size), fd);
+MHD_Response* file_body::materialize() {
+    if (fd_ == -1) return nullptr;
+
+    if (size_) {
+        MHD_Response* r = MHD_create_response_from_fd(size_, fd_);
+        if (r != nullptr) {
+            materialized_ = true;  // MHD now owns fd_
+        }
+        return r;
     }
-    ::close(fd);
-    size_cached_ = 0;
+    // Zero-byte file: serve empty response without giving the fd to MHD.
+    ::close(fd_);
+    fd_ = -1;
+    materialized_ = true;  // suppress ~file_body's close (already closed)
     return MHD_create_response_from_buffer(
         0, nullptr, MHD_RESPMEM_PERSISTENT);
 }
@@ -177,7 +196,14 @@ MHD_Response* pipe_body::materialize() {
 // ---------------------------------------------------------------------------
 ssize_t deferred_body::trampoline(void* cls, std::uint64_t pos,
                                   char* buf, std::size_t max) {
+    // Guard against null cls or empty producer_ (security-reviewer-iter1-3 /
+    // CWE-476). MHD's callback mechanism does not catch C++ exceptions, so
+    // throwing std::bad_function_call here would call std::terminate().
+    // Return MHD_CONTENT_READER_END_WITH_ERROR instead.
     auto* self = static_cast<deferred_body*>(cls);
+    if (!self || !self->producer_) {
+        return MHD_CONTENT_READER_END_WITH_ERROR;
+    }
     return self->producer_(pos, buf, max);
 }
 

src/httpserver/details/body.hpp

@@ -36,6 +36,7 @@
 #define SRC_HTTPSERVER_DETAILS_BODY_HPP_
 
 #include <sys/types.h>      // ssize_t
+#include <cassert>
 #include <cstddef>
 #include <cstdint>
 #include <functional>
@@ -116,24 +117,41 @@ class string_body final : public body {
 };
 
 // ---------------------------------------------------------------------------
-// file_body — opens path on materialize(); returns nullptr if open or
-// fstat fails (matches v1 file_response::get_raw_response exactly).
-// size_cached_ is reserved for future use; size() currently returns it
-// untouched (set on materialize) so the value reflects the on-disk size
-// only after a successful materialise. This matches v1, which never
-// exposed a size accessor at all.
+// file_body — opens the file and runs fstat at construction so that:
+//   * size() is accurate immediately (no need to call materialize() first)
+//   * materialize() avoids the lseek TOCTOU race (security-reviewer-iter1-1):
+//     st_size from fstat is used directly, the fd position is never changed
+//     before being handed to MHD_create_response_from_fd.
+//   * repeated open/fstat syscalls on re-materialize are eliminated
+//     (performance-reviewer-iter1-2).
+//
+// Caller path contract (security-reviewer-iter1-2 / CWE-23):
+//   path_ is assumed to be a validated, canonicalized path. O_NOFOLLOW
+//   blocks the final component being a symlink, but intermediate components
+//   are still followed. Callers supplying user-derived paths MUST canonicalize
+//   them (e.g. realpath()) before constructing file_body.
+//
+// Ownership / lifecycle:
+//   * If open or fstat fails at construction, fd_ == -1 and size_ == 0;
+//     materialize() will return nullptr.
+//   * If materialize() succeeds, MHD owns the fd (MHD_destroy_response closes
+//     it). materialized_ is set to suppress ~file_body's close.
+//   * If materialize() is never called, ~file_body closes fd_.
 // ---------------------------------------------------------------------------
 class file_body final : public body {
  public:
-    explicit file_body(std::string path) noexcept : path_(std::move(path)) {}
+    explicit file_body(std::string path) noexcept;
+    ~file_body() override;
 
     body_kind kind() const noexcept override { return body_kind::file; }
-    std::size_t size() const noexcept override { return size_cached_; }
+    std::size_t size() const noexcept override { return size_; }
     MHD_Response* materialize() override;
 
  private:
     std::string path_;
-    std::size_t size_cached_ = 0;
+    std::size_t size_ = 0;
+    int fd_ = -1;
+    bool materialized_ = false;
 };
 
 // ---------------------------------------------------------------------------
@@ -145,6 +163,29 @@ class file_body final : public body {
 // total_size_ is computed once at construction so size() is O(1); MHD's
 // MHD_IoVec doesn't expose total length and the architecture-spec
 // size() contract is "logical body size in bytes".
+//
+// LIFETIME CONTRACT (security-reviewer-iter1-2 / CWE-416):
+//   iovec_body owns the entries_ vector (the container), but the iov_base
+//   pointers inside each iovec_entry are BORROWED — they point into
+//   caller-owned buffers. After materialize() returns, libmicrohttpd holds
+//   those borrowed pointers until MHD_destroy_response() is called.
+//
+//   CALLERS MUST guarantee that all iov_base buffers (and the iovec_body
+//   itself) outlive the MHD_Response* returned by materialize(). The
+//   TASK-009/010 factory path enforces this by tying the iovec_body's
+//   lifetime to http_response, and http_response must outlive the MHD
+//   connection. Do NOT free the underlying buffer data before the
+//   MHD_Response is destroyed.
+//
+// ALLOCATION NOTE (performance-reviewer-iter1-1):
+//   std::vector unconditionally heap-allocates its backing store, so every
+//   iovec_body construction performs one heap allocation. The SBO
+//   static_assert only verifies that the vector control block (3 words)
+//   fits in the 64-byte inline slot; the iovec_entry array itself lives on
+//   the heap. This is intentional: iovec payloads are by definition
+//   scatter lists of borrowed pointers, so a further small-vector
+//   optimisation would only save one allocation while adding complexity.
+//   Per DR-005 the heap fallback is accepted for iovec_body.
 // ---------------------------------------------------------------------------
 class iovec_body final : public body {
  public:
@@ -200,14 +241,39 @@ class pipe_body final : public body {
 // The trampoline is the C-callable wrapper MHD invokes; it dispatches
 // to producer_. Exposed publicly (static method) so unit tests can
 // drive it without a daemon.
+//
+// NULL GUARD (security-reviewer-iter1-3 / CWE-476):
+//   trampoline() checks for null cls and empty producer_ before invoking
+//   the callable. MHD's callback mechanism does not catch C++ exceptions;
+//   a null-invoke would call std::terminate() in MHD's IO thread.
+//   If cls is null or producer_ is empty, trampoline returns
+//   MHD_CONTENT_READER_END_WITH_ERROR to signal an error to MHD.
+//
+// ALLOCATION NOTE (performance-reviewer-iter1-3):
+//   std::function internally uses small-buffer optimisation (SBO), but
+//   the SBO threshold is implementation-defined (typically 16-32 bytes on
+//   libstdc++ / libc++). Lambdas that capture more than one pointer (e.g.
+//   a user object reference plus a shared_ptr sentinel) will silently
+//   heap-allocate inside std::function. The static_assert on
+//   sizeof(deferred_body) only verifies that std::function's control
+//   block fits in the 64-byte SBO buffer, NOT that the callable itself
+//   is stored inline. Callers should minimise captures to stay within
+//   std::function's internal SSO threshold if zero-allocation is required.
 // ---------------------------------------------------------------------------
 class deferred_body final : public body {
  public:
     using producer_type =
         std::function<ssize_t(std::uint64_t, char*, std::size_t)>;
 
     explicit deferred_body(producer_type producer) noexcept
-        : producer_(std::move(producer)) {}
+        : producer_(std::move(producer)) {
+        // Precondition: caller must not pass a null/empty callable.
+        // An empty producer_ would cause trampoline() to return
+        // MHD_CONTENT_READER_END_WITH_ERROR on every MHD read callback,
+        // which is unlikely to be the caller's intent.
+        assert(producer_ != nullptr &&
+               "deferred_body: producer must not be empty");
+    }
 
     body_kind kind() const noexcept override { return body_kind::deferred; }
     std::size_t size() const noexcept override { return 0; }  // size unknown

test/unit/body_test.cpp

@@ -152,6 +152,18 @@ LT_BEGIN_AUTO_TEST(body_suite, file_body_kind_and_materialize_existing_file)
     MHD_destroy_response(r);
 LT_END_AUTO_TEST(file_body_kind_and_materialize_existing_file)
 
+// security-reviewer-iter1-1 + performance-reviewer-iter1-2: file is opened and
+// stat'd at construction so size() is accurate before materialize() is called,
+// and materialize() uses fstat's st_size rather than lseek (no fd-position
+// side-effect, no TOCTOU window on the size).
+LT_BEGIN_AUTO_TEST(body_suite, file_body_size_known_before_materialize)
+    // test_content is 21 bytes ("test content of file\n").
+    httpserver::detail::file_body b("test_content");
+    // size() must be non-zero and correct at construction time — the file is
+    // opened and fstat'd in the constructor, not in materialize().
+    LT_CHECK_EQ(b.size(), static_cast<std::size_t>(21));
+LT_END_AUTO_TEST(file_body_size_known_before_materialize)
+
 LT_BEGIN_AUTO_TEST(body_suite, file_body_returns_null_on_missing_file)
     httpserver::detail::file_body b("/no/such/path/should/exist");
     // Mirrors v1 file_response::get_raw_response semantics.
@@ -248,6 +260,17 @@ LT_BEGIN_AUTO_TEST(body_suite, deferred_body_trampoline_invokes_stored_callable)
     LT_CHECK_EQ(out[1], 'i');
 LT_END_AUTO_TEST(deferred_body_trampoline_invokes_stored_callable)
 
+// security-reviewer-iter1-3: trampoline must not invoke an empty/null
+// std::function — it should return MHD_CONTENT_READER_END_WITH_ERROR instead
+// of throwing std::bad_function_call (which would terminate in MHD's IO thread).
+LT_BEGIN_AUTO_TEST(body_suite, deferred_body_trampoline_null_cls_returns_error)
+    // cls == nullptr: trampoline must guard against null self pointer.
+    char out[16] = {};
+    ssize_t n = httpserver::detail::deferred_body::trampoline(
+        nullptr, 0, out, sizeof(out));
+    LT_CHECK_EQ(n, static_cast<ssize_t>(MHD_CONTENT_READER_END_WITH_ERROR));
+LT_END_AUTO_TEST(deferred_body_trampoline_null_cls_returns_error)
+
 LT_BEGIN_AUTO_TEST(body_suite, deferred_body_destructor_releases_callable)
     auto sentinel = std::make_shared<int>(42);
     std::weak_ptr<int> w = sentinel;