Skip to content

Commit 22d8f07

Browse files
etrclaude
andcommitted
TASK-036 review: fix callback=nullptr and counter isolation
- Add = nullptr default initializer to modded_request::callback field, eliminating UB per the C++ standard (findings 1 & 5: code-quality- reviewer + code-simplifier). The is_allowed guard already prevents the pointer from being invoked for unrecognized methods; this makes the contract explicit at the declaration site. - Update the comment in resolve_method_callback to remove the stale "pre-existing latent bug" note now that callback has a safe default. - Add per-test counter reset at the start of the three deferred tests that share the static counter (finding 12: test-quality-reviewer). tear_down() also resets but cannot guarantee ordering when tests run in non-default order or under skip conditions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 47e926d commit 22d8f07

3 files changed

Lines changed: 11 additions & 4 deletions

File tree

src/detail/webserver_request.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -366,8 +366,9 @@ void webserver_impl::resolve_method_callback(const char* method,
366366
// call hrm->is_allowed without re-scanning the wire string.
367367
// Unrecognised methods leave mr->method_enum at the default
368368
// (count_), so is_allowed(count_) returns false and the request
369-
// takes the 405 path. Pre-existing latent bug: mr->callback may
370-
// also be left un-set here; see TASK-027 for the dispatch redesign.
369+
// takes the 405 path. mr->callback is left at nullptr (its
370+
// default-initializer value) for unrecognised methods; the 405 guard
371+
// in dispatch_resource_handler fires before it is ever invoked.
371372
if (0 == strcmp(method, http_utils::http_method_get)) {
372373
mr->callback = &http_resource::render_get;
373374
mr->method_enum = http_method::get;

src/httpserver/detail/modded_request.hpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,11 @@ struct modded_request {
4848
webserver* ws = nullptr;
4949

5050
// TASK-036: pointer-to-member dispatch slot; render_* now return
51-
// http_response by value (PRD-RSP-REQ-007 / DR-004).
52-
http_response (httpserver::http_resource::*callback)(const httpserver::http_request&);
51+
// http_response by value (PRD-RSP-REQ-007 / DR-004). Initialized to
52+
// nullptr; set by resolve_method_callback for recognized HTTP methods.
53+
// For unrecognized methods mr->method_enum is left at count_ and
54+
// finalize_answer takes the 405 path before invoking this pointer.
55+
http_response (httpserver::http_resource::*callback)(const httpserver::http_request&) = nullptr;
5356

5457
// TASK-021: enum form of the wire method, decoded once at the
5558
// dispatch boundary in webserver_impl::answer_to_connection. Used

test/integ/deferred.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,7 @@ LT_BEGIN_SUITE(deferred_suite)
189189
LT_END_SUITE(deferred_suite)
190190

191191
LT_BEGIN_AUTO_TEST(deferred_suite, deferred_response_suite)
192+
counter = 0; // reset per-test; tear_down also resets but order may vary
192193
deferred_resource resource;
193194
ws->register_path("base", as_shared(resource));
194195
curl_global_init(CURL_GLOBAL_ALL);
@@ -207,6 +208,7 @@ LT_BEGIN_AUTO_TEST(deferred_suite, deferred_response_suite)
207208
LT_END_AUTO_TEST(deferred_response_suite)
208209

209210
LT_BEGIN_AUTO_TEST(deferred_suite, deferred_response_with_data)
211+
counter = 0; // reset per-test; tear_down also resets but order may vary
210212
deferred_resource_with_data resource;
211213
ws->register_path("base", as_shared(resource));
212214
curl_global_init(CURL_GLOBAL_ALL);
@@ -225,6 +227,7 @@ LT_BEGIN_AUTO_TEST(deferred_suite, deferred_response_with_data)
225227
LT_END_AUTO_TEST(deferred_response_with_data)
226228

227229
LT_BEGIN_AUTO_TEST(deferred_suite, deferred_response_empty_content)
230+
counter = 0; // reset per-test; tear_down also resets but order may vary
228231
deferred_resource_empty_content resource;
229232
ws->register_path("base", as_shared(resource));
230233
curl_global_init(CURL_GLOBAL_ALL);

0 commit comments

Comments
 (0)