Fix null key dereference in post_iterator (#375) (#376)

MHD may invoke the post iterator with a null key on a continuation
chunk (off > 0): the field name is supplied only on the first call and
not repeated. handle_post_form_arg passed the raw key pointer straight
into std::string (via set_arg / grow_last_arg), which throws
std::logic_error on null. Because the throw escapes the C post-iterator
callback, it propagates as an uncaught exception and aborts the process
via std::terminate.

Guard key for null before constructing std::string: with no field name
there is nothing to store the value under, so accept and silently skip
the chunk (MHD_YES keeps the request alive; MHD_NO would abort it).
Same class of bug as the null-uri fix in uri_log (#371).

Adds unit/post_iterator_null_key_test.cpp pinning the regression (the
test terminates without the guard) plus the happy-path initial and
continuation-append cases.


Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: etr

src/detail/webserver_callbacks.cpp

@@ -374,6 +374,18 @@ namespace detail {
 
 MHD_Result webserver_impl::handle_post_form_arg(detail::modded_request* mr,
         const char* key, const char* data, size_t size, uint64_t off) {
+    // MHD may invoke the post iterator with a null key on a continuation
+    // chunk (off > 0): the field name was supplied on the first call and
+    // is not repeated. With no field name there is nothing to store the
+    // value under, so silently accept the chunk (MHD_YES tells MHD to
+    // continue; MHD_NO would abort the whole request). Guarding here also
+    // stops the raw pointer from reaching std::string, which throws
+    // std::logic_error on null and aborts the process via std::terminate
+    // because the throw escapes a C callback. See issue #375 (same class
+    // of bug as the null-uri fix in uri_log, issue #371).
+    if (key == nullptr) {
+        return MHD_YES;
+    }
     // No file: set the arg key/value and return. A non-zero @p off
     // means MHD is feeding us a continuation chunk of a previously-
     // started value, so append rather than replace.

test/Makefile.am

@@ -26,7 +26,7 @@ LDADD += -lcurl
 
 AM_CPPFLAGS = -I$(top_srcdir)/src -I$(top_srcdir)/src/httpserver/ -DHTTPSERVER_COMPILATION
 METASOURCES = AUTO
-check_PROGRAMS = basic file_upload http_utils threaded nodelay string_utilities http_endpoint ban_system ws_start_stop authentication digest_challenge_format deferred http_resource http_response create_webserver create_webserver_explicit new_response_types daemon_info uri_log feature_unavailable header_hygiene_iovec header_hygiene iovec_entry http_method constants body http_response_sbo http_response_factories http_response_digest_factory http_response_move_sanitizer webserver_pimpl http_request_pimpl create_test_request http_request_arena http_request_unescape_arena http_request_const_getters http_request_tls_accessors http_request_operator_stream webserver_register_smartptr webserver_register_path_prefix webserver_on_methods webserver_route route_table lookup_pipeline route_table_concurrency routing_regression route_lookup_canonicalize auth_skip_normalize http_resource_allow_cache v2_dispatch_contract threadsafety_stress webserver_features webserver_ws_unavailable webserver_ws_available webserver_register_ws_smartptr webserver_dauth_unavailable webserver_dauth_available consumer_fixture header_hygiene_hooks hook_api_shape hooks_no_firing hooks_accept_ctx_shape hooks_connection_lifecycle hooks_accept_decision_banned hooks_accept_decision_throwing hooks_body_chunk_ctx_shape hooks_request_received_short_circuit hooks_body_chunk_observes_progress hooks_body_chunk_short_circuit_no_leak hooks_before_handler_ctx_shape hooks_route_resolved_miss_and_hit hooks_before_handler_short_circuit hooks_alias_count hooks_alias_functional hooks_handler_exception_chain hooks_handler_exception_user_handler_throws_continues_chain hooks_handler_exception_fallback_to_hardcoded_500 hooks_handler_exception_slot hooks_response_sent_ctx_shape hooks_request_completed_ctx_shape hooks_after_handler_replaces_response hooks_after_handler_mutates_response_in_place hooks_response_sent_carries_status_bytes_timing hooks_request_completed_fires_on_early_failure hooks_log_access_alias_slot hooks_per_route_invalid_phase_throws hooks_per_route_order hooks_per_route_early_413_per_endpoint hooks_per_route_resource_destroyed_first hooks_per_route_concurrent_registration hooks_not_found_alias auth_handler_optional_signature no_v1_compat_shim cookie_header_sentinel cookie_render cookie_deprecation_sentinel http_response_cookie_wire http_request_cookies_parsed peer_address_to_string secure_zero_dce connection_state_sentinel connection_state_body_residue debug_dump_request_body_unset debug_dump_request_body_set littletest_skip_semantics digest_client_self_test
+check_PROGRAMS = basic file_upload http_utils threaded nodelay string_utilities http_endpoint ban_system ws_start_stop authentication digest_challenge_format deferred http_resource http_response create_webserver create_webserver_explicit new_response_types daemon_info uri_log post_iterator_null_key feature_unavailable header_hygiene_iovec header_hygiene iovec_entry http_method constants body http_response_sbo http_response_factories http_response_digest_factory http_response_move_sanitizer webserver_pimpl http_request_pimpl create_test_request http_request_arena http_request_unescape_arena http_request_const_getters http_request_tls_accessors http_request_operator_stream webserver_register_smartptr webserver_register_path_prefix webserver_on_methods webserver_route route_table lookup_pipeline route_table_concurrency routing_regression route_lookup_canonicalize auth_skip_normalize http_resource_allow_cache v2_dispatch_contract threadsafety_stress webserver_features webserver_ws_unavailable webserver_ws_available webserver_register_ws_smartptr webserver_dauth_unavailable webserver_dauth_available consumer_fixture header_hygiene_hooks hook_api_shape hooks_no_firing hooks_accept_ctx_shape hooks_connection_lifecycle hooks_accept_decision_banned hooks_accept_decision_throwing hooks_body_chunk_ctx_shape hooks_request_received_short_circuit hooks_body_chunk_observes_progress hooks_body_chunk_short_circuit_no_leak hooks_before_handler_ctx_shape hooks_route_resolved_miss_and_hit hooks_before_handler_short_circuit hooks_alias_count hooks_alias_functional hooks_handler_exception_chain hooks_handler_exception_user_handler_throws_continues_chain hooks_handler_exception_fallback_to_hardcoded_500 hooks_handler_exception_slot hooks_response_sent_ctx_shape hooks_request_completed_ctx_shape hooks_after_handler_replaces_response hooks_after_handler_mutates_response_in_place hooks_response_sent_carries_status_bytes_timing hooks_request_completed_fires_on_early_failure hooks_log_access_alias_slot hooks_per_route_invalid_phase_throws hooks_per_route_order hooks_per_route_early_413_per_endpoint hooks_per_route_resource_destroyed_first hooks_per_route_concurrent_registration hooks_not_found_alias auth_handler_optional_signature no_v1_compat_shim cookie_header_sentinel cookie_render cookie_deprecation_sentinel http_response_cookie_wire http_request_cookies_parsed peer_address_to_string secure_zero_dce connection_state_sentinel connection_state_body_residue debug_dump_request_body_unset debug_dump_request_body_set littletest_skip_semantics digest_client_self_test
 
 MOSTLYCLEANFILES = *.gcda *.gcno *.gcov
 
@@ -98,6 +98,14 @@ uri_log_SOURCES = unit/uri_log_test.cpp
 # it needs an explicit -lmicrohttpd in its link line on top of the default
 # LDADD (modern ld enforces --no-copy-dt-needed-entries).
 uri_log_LDADD = $(LDADD) -lmicrohttpd
+# post_iterator_null_key: issue #375 regression. Drives the static
+# webserver_impl::post_iterator callback with a null key (the continuation-
+# chunk case MHD hits when a field is split across callbacks) and asserts it
+# no longer throws std::logic_error / terminates. Constructs a
+# modded_request whose ~modded_request() touches libmicrohttpd, so it needs
+# the explicit -lmicrohttpd link the same way uri_log does.
+post_iterator_null_key_SOURCES = unit/post_iterator_null_key_test.cpp
+post_iterator_null_key_LDADD = $(LDADD) -lmicrohttpd
 feature_unavailable_SOURCES = unit/feature_unavailable_test.cpp
 header_hygiene_iovec_SOURCES = unit/header_hygiene_iovec_test.cpp
 # header_hygiene: TASK-007 sentinel TU. Mimics a true consumer:

test/unit/post_iterator_null_key_test.cpp

@@ -0,0 +1,126 @@
+/*
+     This file is part of libhttpserver
+     Copyright (C) 2011-2019 Sebastiano Merlino
+
+     This library is free software; you can redistribute it and/or
+     modify it under the terms of the GNU Lesser General Public
+     License as published by the Free Software Foundation; either
+     version 2.1 of the License, or (at your option) any later version.
+
+     This library is distributed in the hope that it will be useful,
+     but WITHOUT ANY WARRANTY; without even the implied warranty of
+     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+     Lesser General Public License for more details.
+
+     You should have received a copy of the GNU Lesser General Public
+     License along with this library; if not, write to the Free Software
+     Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301
+     USA
+*/
+
+#include <string>
+
+#include "./httpserver.hpp"
+#include "httpserver/create_test_request.hpp"
+#include "httpserver/detail/modded_request.hpp"
+#include "httpserver/detail/webserver_impl.hpp"
+
+#include "./littletest.hpp"
+
+// post_iterator is the MHD post-processor callback defined as a static
+// member of webserver_impl (it lives in the dispatch helper header under
+// the class's public: section). The no-file branch funnels into
+// handle_post_form_arg, which is the function issue #375 hardens. We drive
+// the public callback so the test exercises the same entry point MHD does.
+//
+// modded_request::dhr is a unique_ptr<http_request> with the default
+// deleter, and http_request's move constructor is private (friended only
+// to create_test_request), so the request cannot be make_unique'd. Instead
+// we own an http_request on the stack -- built via the public test builder
+// using guaranteed copy elision -- and point dhr at it, detaching dhr
+// before destruction so the stack object is not double-freed.
+namespace {
+struct request_fixture {
+    httpserver::http_request req =
+        httpserver::create_test_request().build();
+    httpserver::detail::modded_request mr;
+
+    request_fixture() {
+        // ws is never read on the no-file form-arg path, so leave it null.
+        mr.dhr.reset(&req);
+    }
+
+    ~request_fixture() {
+        // Detach before mr (and its unique_ptr) is destroyed: req is
+        // stack-owned and must not be deleted through dhr.
+        mr.dhr.release();
+    }
+
+    MHD_Result feed(const char* key, const char* data, uint64_t off,
+                    size_t size) {
+        return httpserver::detail::webserver_impl::post_iterator(
+            &mr, MHD_POSTDATA_KIND, key, /*filename=*/nullptr,
+            /*content_type=*/nullptr, /*transfer_encoding=*/nullptr,
+            data, off, size);
+    }
+};
+}  // namespace
+
+LT_BEGIN_SUITE(post_iterator_null_key_suite)
+    void set_up() {
+    }
+
+    void tear_down() {
+    }
+LT_END_SUITE(post_iterator_null_key_suite)
+
+// Regression test for issue #375: MHD may invoke the post iterator with a
+// null key on a continuation chunk (off > 0) because the field name was
+// only supplied on the first call. The previous implementation passed the
+// raw key pointer into std::string, which throws std::logic_error on null
+// and aborts the process via std::terminate (the throw escapes a C
+// callback). The guard must instead accept and silently skip the chunk.
+LT_BEGIN_AUTO_TEST(post_iterator_null_key_suite, null_key_continuation_does_not_throw)
+    request_fixture f;
+    MHD_Result r = MHD_NO;
+    LT_CHECK_NOTHROW(r = f.feed(/*key=*/nullptr, "value", /*off=*/5, 5));
+    // MHD_YES keeps the request alive; MHD_NO would abort it.
+    LT_CHECK_EQ(r, MHD_YES);
+    // Nothing was stored: there was no field name to key the value under.
+    LT_CHECK_EQ(f.mr.dhr->get_args().size(), static_cast<size_t>(0));
+LT_END_AUTO_TEST(null_key_continuation_does_not_throw)
+
+// Same guard on the initial-chunk path (off == 0). MHD should not normally
+// hand us a null key here, but the guard is unconditional, so pin it.
+LT_BEGIN_AUTO_TEST(post_iterator_null_key_suite, null_key_initial_does_not_throw)
+    request_fixture f;
+    MHD_Result r = MHD_NO;
+    LT_CHECK_NOTHROW(r = f.feed(/*key=*/nullptr, "value", /*off=*/0, 5));
+    LT_CHECK_EQ(r, MHD_YES);
+    LT_CHECK_EQ(f.mr.dhr->get_args().size(), static_cast<size_t>(0));
+LT_END_AUTO_TEST(null_key_initial_does_not_throw)
+
+// Happy path: a non-null key on the initial chunk stores the value under
+// that field name, proving the guard did not regress normal form handling.
+LT_BEGIN_AUTO_TEST(post_iterator_null_key_suite, valid_key_stores_arg)
+    request_fixture f;
+    MHD_Result r = f.feed("field", "value", /*off=*/0, 5);
+    LT_CHECK_EQ(r, MHD_YES);
+    LT_CHECK_EQ(std::string(f.mr.dhr->get_arg_flat("field")),
+                std::string("value"));
+LT_END_AUTO_TEST(valid_key_stores_arg)
+
+// Continuation chunk with a (repeated) non-null key appends to the value
+// MHD started on the first call - the legitimate large-field split that
+// commit 1b5fe8f (issue #337) introduced grow_last_arg to handle.
+LT_BEGIN_AUTO_TEST(post_iterator_null_key_suite, valid_key_continuation_appends)
+    request_fixture f;
+    LT_CHECK_EQ(f.feed("field", "hel", /*off=*/0, 3), MHD_YES);
+    LT_CHECK_EQ(f.feed("field", "lo", /*off=*/3, 2), MHD_YES);
+    LT_CHECK_EQ(std::string(f.mr.dhr->get_arg_flat("field")),
+                std::string("hello"));
+LT_END_AUTO_TEST(valid_key_continuation_appends)
+
+LT_BEGIN_AUTO_TEST_ENV()
+    AUTORUN_TESTS()
+LT_END_AUTO_TEST_ENV()