Skip to content

Commit f4bb3d2

Browse files
etrclaude
andcommitted
TASK-009: http_response value type with SBO buffer + detail/ rename
Convert http_response from a copy-able subclass-rooted type to a move-only value type that carries a 64-byte SBO buffer for the polymorphic body. Also rename src/[httpserver/]details/ to src/[httpserver/]detail/ so the directory name matches the httpserver::detail namespace (resolves the plural/singular mismatch left by TASK-008). Layout (PRD-HDR-REQ-004 exemption, DR-005): - status_code_, headers_, footers_, cookies_, kind_ - alignas(16) std::byte body_storage_[64] - detail::body* body_, bool body_inline_ - public: body_pointer_type = detail::body*, body_buf_size = 64 The body lives in body_storage_ inline (the common case) or on the heap via ::operator new(sizeof(T))+placement-new for outsized bodies. body_inline_ discriminates so the destructor knows whether to invoke ::operator delete. Hand-written noexcept move ctor and move-assign cover all four inline/heap cross-product cases through two single-source helpers (destroy_body / adopt_body_from); copy ops are deleted (DR-005). Self-move-assign is guarded explicitly. Detail layer amendment (TASK-008 follow-up): re-enable noexcept move construction on detail::body subclasses and add a noexcept move_into(void*) virtual so http_response can placement-move an inline body across SBO buffers without copying. file_body and pipe_body get hand-written move ctors that std::exchange fd_ and flip materialized_ to suppress double-close in the moved-from object. Per-subclass static_assert(std::is_nothrow_move_constructible_v<>) locks the contract in. V1 subclass headers (string/file/empty/digest/basic/deferred response) now declare = delete for copy ops to follow the move-only base. The iovec/pipe response headers were already deleted-copy. `final` is deferred to TASK-013 (the v1 subclasses still inherit at this point; TASK-013 removes them and seals the class). The class stays virtual-destructor for the same reason. Test TU exercises the SBO 4-case cross-product through a single friend struct (http_response_sbo_test_access) declared in the header. Test: - 26 testsuite entries (was 25), all green; new http_response_sbo test passes 10 tests / 30 checks - body_test still green (detail/ amendment is contract-compatible) - new SBO test sanitizer-clean under ASan + UBSan - cpplint clean across all changed files Refs: PRD-HDR-REQ-004 (exemption clause), PRD-RSP-REQ-001, PRD-RSP-REQ-007, DR-003a, DR-005 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c953e85 commit f4bb3d2

25 files changed

Lines changed: 718 additions & 83 deletions

Makefile.am

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -126,12 +126,12 @@ check-headers:
126126
@echo " PASS: A.4 umbrella does not leak _HTTPSERVER_HPP_INSIDE_"
127127

128128
# check-install-layout asserts that `make install DESTDIR=$(STAGE)` produces
129-
# a public include tree with NO `details/` directory and NO `*_impl.hpp` files.
129+
# a public include tree with NO `detail/` directory and NO `*_impl.hpp` files.
130130
# This protects the public/private split as described in TASK-002 / DR-002.
131131
CHECK_INSTALL_STAGE = $(abs_top_builddir)/.install-stage
132132

133133
check-install-layout:
134-
@echo "=== check-install-layout: staged install must hide details/ and *_impl.hpp ==="
134+
@echo "=== check-install-layout: staged install must hide detail/ and *_impl.hpp ==="
135135
@if test "$(CHECK_INSTALL_SHARED)" != "yes"; then \
136136
rm -rf $(CHECK_INSTALL_STAGE); \
137137
$(MAKE) $(AM_MAKEFLAGS) install DESTDIR=$(CHECK_INSTALL_STAGE) >check-install.log 2>&1 || { \
@@ -143,10 +143,10 @@ check-install-layout:
143143
}; \
144144
rm -f check-install.log; \
145145
fi
146-
@leaked_details=`find $(CHECK_INSTALL_STAGE) -type d -name details 2>/dev/null`; \
147-
if test -n "$$leaked_details"; then \
148-
echo "FAIL: details/ directory leaked into install:"; \
149-
echo "$$leaked_details"; \
146+
@leaked_detail=`find $(CHECK_INSTALL_STAGE) -type d -name detail 2>/dev/null`; \
147+
if test -n "$$leaked_detail"; then \
148+
echo "FAIL: detail/ directory leaked into install:"; \
149+
echo "$$leaked_detail"; \
150150
if test "$(CHECK_INSTALL_SHARED)" != "yes"; then rm -rf $(CHECK_INSTALL_STAGE); fi; \
151151
exit 1; \
152152
fi

src/Makefile.am

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,11 @@
1919
AM_CPPFLAGS = -I../ -I$(srcdir)/httpserver/ -DHTTPSERVER_COMPILATION
2020
METASOURCES = AUTO
2121
lib_LTLIBRARIES = libhttpserver.la
22-
libhttpserver_la_SOURCES = string_utilities.cpp webserver.cpp http_utils.cpp file_info.cpp http_request.cpp http_response.cpp string_response.cpp digest_auth_fail_response.cpp deferred_response.cpp file_response.cpp pipe_response.cpp empty_response.cpp iovec_response.cpp http_resource.cpp create_webserver.cpp details/http_endpoint.cpp details/body.cpp
22+
libhttpserver_la_SOURCES = string_utilities.cpp webserver.cpp http_utils.cpp file_info.cpp http_request.cpp http_response.cpp string_response.cpp digest_auth_fail_response.cpp deferred_response.cpp file_response.cpp pipe_response.cpp empty_response.cpp iovec_response.cpp http_resource.cpp create_webserver.cpp detail/http_endpoint.cpp detail/body.cpp
2323
# noinst_HEADERS: shipped in the tarball but NEVER installed under $prefix/include.
24-
# Detail headers (httpserver/details/*.hpp) live here so they cannot leak to
24+
# Detail headers (httpserver/detail/*.hpp) live here so they cannot leak to
2525
# downstream consumers — the public surface comes in through <httpserver.hpp>.
26-
noinst_HEADERS = httpserver/string_utilities.hpp httpserver/details/modded_request.hpp httpserver/details/http_endpoint.hpp httpserver/details/body.hpp gettext.h
26+
noinst_HEADERS = httpserver/string_utilities.hpp httpserver/detail/modded_request.hpp httpserver/detail/http_endpoint.hpp httpserver/detail/body.hpp gettext.h
2727
nobase_include_HEADERS = httpserver.hpp httpserver/body_kind.hpp httpserver/constants.hpp httpserver/create_webserver.hpp httpserver/webserver.hpp httpserver/http_utils.hpp httpserver/file_info.hpp httpserver/http_request.hpp httpserver/http_response.hpp httpserver/http_resource.hpp httpserver/string_response.hpp httpserver/digest_auth_fail_response.hpp httpserver/deferred_response.hpp httpserver/file_response.hpp httpserver/pipe_response.hpp httpserver/empty_response.hpp httpserver/feature_unavailable.hpp httpserver/iovec_entry.hpp httpserver/iovec_response.hpp httpserver/http_arg_value.hpp httpserver/http_method.hpp
2828

2929
if HAVE_BAUTH
Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
USA
1919
*/
2020

21-
#include "httpserver/details/body.hpp"
21+
#include "httpserver/detail/body.hpp"
2222

2323
#include <fcntl.h>
2424
#include <microhttpd.h>
@@ -143,6 +143,18 @@ file_body::~file_body() {
143143
}
144144
}
145145

146+
// Hand-written move ctor: transfers fd_ ownership to the destination and
147+
// flips the source's materialized_ to true so the source's destructor
148+
// skips the close path. Without this, the moved-from file_body would
149+
// close the fd we just handed off — a classic double-close bug
150+
// (CWE-415). std::exchange keeps the move noexcept.
151+
file_body::file_body(file_body&& o) noexcept
152+
: path_(std::move(o.path_)),
153+
size_(o.size_),
154+
fd_(std::exchange(o.fd_, -1)),
155+
materialized_(std::exchange(o.materialized_, true)) {
156+
}
157+
146158
MHD_Response* file_body::materialize() {
147159
if (fd_ == -1) return nullptr;
148160

@@ -190,6 +202,13 @@ pipe_body::~pipe_body() {
190202
}
191203
}
192204

205+
// Same shape as file_body's move ctor: transfer fd_, mark source as
206+
// already-materialized so its destructor skips close.
207+
pipe_body::pipe_body(pipe_body&& o) noexcept
208+
: fd_(std::exchange(o.fd_, -1)),
209+
materialized_(std::exchange(o.materialized_, true)) {
210+
}
211+
193212
MHD_Response* pipe_body::materialize() {
194213
MHD_Response* r = MHD_create_response_from_pipe(fd_);
195214
if (r != nullptr) {
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
#include <string>
3030
#include <vector>
3131

32-
#include "httpserver/details/http_endpoint.hpp"
32+
#include "httpserver/detail/http_endpoint.hpp"
3333
#include "httpserver/http_utils.hpp"
3434

3535
using std::string;

src/http_response.cpp

Lines changed: 133 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,41 +19,165 @@
1919
*/
2020

2121
#include "httpserver/http_response.hpp"
22+
2223
#include <microhttpd.h>
24+
25+
#include <cstddef>
2326
#include <iostream>
2427
#include <map>
28+
#include <new>
2529
#include <string>
30+
#include <type_traits>
2631
#include <utility>
32+
33+
#include "httpserver/detail/body.hpp" // complete type for body_->~body()
2734
#include "httpserver/http_utils.hpp"
2835

2936
namespace httpserver {
3037

38+
// -----------------------------------------------------------------------
39+
// Layout / trait acceptance asserts (TASK-009 AC). Duplicated in
40+
// test/unit/http_response_sbo_test.cpp; placing them in the .cpp catches
41+
// drift on every library build, even before tests are linked.
42+
// -----------------------------------------------------------------------
43+
static_assert(std::is_nothrow_move_constructible_v<http_response>,
44+
"TASK-009 AC: move ctor must be noexcept");
45+
static_assert(std::is_nothrow_move_assignable_v<http_response>,
46+
"TASK-009 AC: move assign must be noexcept");
47+
static_assert(!std::is_copy_constructible_v<http_response>,
48+
"TASK-009 AC: move-only");
49+
static_assert(!std::is_copy_assignable_v<http_response>,
50+
"TASK-009 AC: move-only");
51+
static_assert(http_response::body_buf_size == 64,
52+
"DR-005: SBO buffer is 64 bytes");
53+
static_assert(alignof(http_response) >= 16,
54+
"alignas(16) std::byte body_storage_[64] requires class "
55+
"alignment >= 16");
56+
57+
// -----------------------------------------------------------------------
58+
// Body lifecycle helpers.
59+
//
60+
// destroy_body() and adopt_body_from() factor out the SBO destruct /
61+
// adopt logic that the destructor, move ctor, and move-assign all need.
62+
// Keeping each branch in exactly one place makes the inline-vs-heap
63+
// discriminator impossible to get out of sync. Both helpers are
64+
// noexcept (DR-005): destroy_body relies on body subclass dtors being
65+
// noexcept, adopt_body_from relies on the noexcept move_into() virtual
66+
// (statically asserted per-subclass in detail/body.hpp).
67+
//
68+
// Members are private; they live as out-of-line member functions so
69+
// they have access without an extra friend declaration.
70+
// -----------------------------------------------------------------------
71+
void http_response::destroy_body() noexcept {
72+
if (body_ == nullptr) return;
73+
body_->~body();
74+
if (!body_inline_) {
75+
// Heap path: ::operator delete pairs with the
76+
// ::operator new(sizeof(T)) the factory uses (TASK-010).
77+
::operator delete(body_);
78+
}
79+
body_ = nullptr;
80+
body_inline_ = false;
81+
}
82+
83+
void http_response::adopt_body_from(http_response& o) noexcept {
84+
if (o.body_ == nullptr) {
85+
return; // destination's body_/body_inline_ already cleared
86+
}
87+
if (o.body_inline_) {
88+
// Placement-move into our buffer, then destroy the source's
89+
// inline body so the source's destructor is a no-op.
90+
o.body_->move_into(body_storage_);
91+
body_ = reinterpret_cast<detail::body*>(body_storage_);
92+
body_inline_ = true;
93+
o.body_->~body();
94+
} else {
95+
// Heap path: pointer transfer — no allocation, no copy.
96+
body_ = o.body_;
97+
body_inline_ = false;
98+
}
99+
o.body_ = nullptr;
100+
o.body_inline_ = false;
101+
}
102+
103+
// -----------------------------------------------------------------------
104+
// Destructor.
105+
//
106+
// Subclass-virtual destructor: required as long as the v1 subclass
107+
// hierarchy still inherits from http_response. TASK-013 marks the class
108+
// `final` once those subclasses are removed.
109+
// -----------------------------------------------------------------------
110+
http_response::~http_response() {
111+
destroy_body();
112+
}
113+
114+
// -----------------------------------------------------------------------
115+
// Move constructor.
116+
//
117+
// noexcept because every member's move is noexcept (header_map is a
118+
// std::map, std::map move is noexcept; std::byte[64] is trivially
119+
// movable; per-subclass body move ctors are noexcept by static_assert in
120+
// detail/body.hpp).
121+
// -----------------------------------------------------------------------
122+
http_response::http_response(http_response&& o) noexcept
123+
: status_code_(o.status_code_),
124+
headers_(std::move(o.headers_)),
125+
footers_(std::move(o.footers_)),
126+
cookies_(std::move(o.cookies_)),
127+
kind_(o.kind_) {
128+
adopt_body_from(o);
129+
}
130+
131+
// -----------------------------------------------------------------------
132+
// Move-assignment.
133+
//
134+
// Linearises the inline×heap inline×heap "four cases" into:
135+
// step 1 — destroy our existing body
136+
// step 2 — adopt source's body
137+
//
138+
// Self-assignment is guarded explicitly because step 1 would otherwise
139+
// destroy the body we are about to read from.
140+
// -----------------------------------------------------------------------
141+
http_response& http_response::operator=(http_response&& o) noexcept {
142+
if (this == &o) {
143+
return *this;
144+
}
145+
destroy_body();
146+
status_code_ = o.status_code_;
147+
headers_ = std::move(o.headers_);
148+
footers_ = std::move(o.footers_);
149+
cookies_ = std::move(o.cookies_);
150+
kind_ = o.kind_;
151+
adopt_body_from(o);
152+
return *this;
153+
}
154+
31155
MHD_Response* http_response::get_raw_response() {
32156
return MHD_create_response_from_buffer(0, nullptr, MHD_RESPMEM_PERSISTENT);
33157
}
34158

35159
void http_response::decorate_response(MHD_Response* response) {
36160
std::map<std::string, std::string, http::header_comparator>::iterator it;
37161

38-
for (it=headers.begin() ; it != headers.end(); ++it) {
162+
for (it=headers_.begin() ; it != headers_.end(); ++it) {
39163
MHD_add_response_header(response, (*it).first.c_str(), (*it).second.c_str());
40164
}
41165

42-
for (it=footers.begin() ; it != footers.end(); ++it) {
166+
for (it=footers_.begin() ; it != footers_.end(); ++it) {
43167
MHD_add_response_footer(response, (*it).first.c_str(), (*it).second.c_str());
44168
}
45169

46-
for (it=cookies.begin(); it != cookies.end(); ++it) {
170+
for (it=cookies_.begin(); it != cookies_.end(); ++it) {
47171
MHD_add_response_header(response, "Set-Cookie", ((*it).first + "=" + (*it).second).c_str());
48172
}
49173
}
50174

51175
int http_response::enqueue_response(MHD_Connection* connection, MHD_Response* response) {
52-
return MHD_queue_response(connection, response_code, response);
176+
return MHD_queue_response(connection, status_code_, response);
53177
}
54178

55179
void http_response::shoutCAST() {
56-
response_code |= http::http_utils::shoutcast_response;
180+
status_code_ |= http::http_utils::shoutcast_response;
57181
}
58182

59183
namespace {
@@ -67,11 +191,11 @@ static inline http::header_view_map to_view_map(const http::header_map& hdr_map)
67191
}
68192

69193
std::ostream &operator<< (std::ostream& os, const http_response& r) {
70-
os << "Response [response_code:" << r.response_code << "]" << std::endl;
194+
os << "Response [response_code:" << r.status_code_ << "]" << std::endl;
71195

72-
http::dump_header_map(os, "Headers", to_view_map(r.headers));
73-
http::dump_header_map(os, "Footers", to_view_map(r.footers));
74-
http::dump_header_map(os, "Cookies", to_view_map(r.cookies));
196+
http::dump_header_map(os, "Headers", to_view_map(r.headers_));
197+
http::dump_header_map(os, "Footers", to_view_map(r.footers_));
198+
http::dump_header_map(os, "Cookies", to_view_map(r.cookies_));
75199

76200
return os;
77201
}

src/httpserver/basic_auth_fail_response.hpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,10 @@ class basic_auth_fail_response : public string_response {
5050
realm(realm),
5151
prefer_utf8(prefer_utf8) { }
5252

53-
basic_auth_fail_response(const basic_auth_fail_response& other) = default;
53+
// Move-only: base http_response is now move-only (TASK-009 / DR-005).
54+
basic_auth_fail_response(const basic_auth_fail_response&) = delete;
5455
basic_auth_fail_response(basic_auth_fail_response&& other) noexcept = default;
55-
basic_auth_fail_response& operator=(const basic_auth_fail_response& b) = default;
56+
basic_auth_fail_response& operator=(const basic_auth_fail_response&) = delete;
5657
basic_auth_fail_response& operator=(basic_auth_fail_response&& b) = default;
5758

5859
~basic_auth_fail_response() = default;

src/httpserver/deferred_response.hpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,9 +58,10 @@ class deferred_response : public string_response {
5858
initial_content(content),
5959
content_offset(0) { }
6060

61-
deferred_response(const deferred_response& other) = default;
61+
// Move-only: base http_response is now move-only (TASK-009 / DR-005).
62+
deferred_response(const deferred_response&) = delete;
6263
deferred_response(deferred_response&& other) noexcept = default;
63-
deferred_response& operator=(const deferred_response& b) = default;
64+
deferred_response& operator=(const deferred_response&) = delete;
6465
deferred_response& operator=(deferred_response&& b) = default;
6566

6667
~deferred_response() = default;

0 commit comments

Comments
 (0)