From bcc1eadcb3347b2393a7fc34929dbe678f41b224 Mon Sep 17 00:00:00 2001 From: Sebastiano Merlino Date: Mon, 29 Jun 2026 11:45:30 -0700 Subject: [PATCH 1/4] Fix null key dereference in post_iterator (#375) 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. The no-file branch passed the raw key pointer into std::string (via grow_last_arg / set_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). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6 --- src/webserver.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/webserver.cpp b/src/webserver.cpp index 647719a7..7d8cd070 100644 --- a/src/webserver.cpp +++ b/src/webserver.cpp @@ -832,6 +832,20 @@ MHD_Result webserver::post_iterator(void *cls, enum MHD_ValueKind kind, struct details::modded_request* mr = (struct details::modded_request*) cls; if (!filename) { + // 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) { + return MHD_YES; + } + // There is no actual file, just set the arg key/value and return. if (off > 0) { mr->dhr->grow_last_arg(key, std::string(data, size)); From 4d48e0d719373861029dc1a283c808af0fa523da Mon Sep 17 00:00:00 2001 From: Sebastiano Merlino Date: Mon, 29 Jun 2026 12:36:33 -0700 Subject: [PATCH 2/4] Add ChangeLog entry for #375 fix Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6 --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index ea6c2045..c81beaab 100644 --- a/ChangeLog +++ b/ChangeLog @@ -42,6 +42,9 @@ Version 0.20.0 Fixed std::terminate when MHD invokes the URI log callback with a null uri pointer (e.g. port scans, half-open connections, or non-HTTP traffic). Resolves issue #371. + Fixed std::terminate when MHD invokes the post-processor iterator with + a null key on a continuation chunk (a field split across multiple + callbacks). Resolves issue #375. Version 0.19.0 - 2023-06-15 From 37d2f9350c8975ee85db0928b129839dd7744e28 Mon Sep 17 00:00:00 2001 From: Sebastiano Merlino Date: Mon, 29 Jun 2026 12:59:06 -0700 Subject: [PATCH 3/4] Add regression unit test for null key in post_iterator (#375) White-box unit test that drives webserver::post_iterator directly with a null key on both the continuation (off > 0) and initial chunks. Without the guard the test process terminates (std::logic_error from std::string(nullptr) escaping the callback); with it the chunk is accepted and skipped. Adds happy-path coverage (initial store and large-field continuation append) to pin that the guard does not regress normal form handling. post_iterator and http_request's constructor / setters are private, so the test uses the standard private->public white-box include technique (standard + libmicrohttpd headers are pulled in first so only libhttpserver's own declarations are affected; member order is unchanged, so object layout matches the normally-compiled library). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6 --- test/Makefile.am | 10 +- test/unit/post_iterator_null_key_test.cpp | 120 ++++++++++++++++++++++ 2 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 test/unit/post_iterator_null_key_test.cpp diff --git a/test/Makefile.am b/test/Makefile.am index 4468ca39..9a0aec7b 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -26,7 +26,7 @@ LDADD += -lcurl AM_CPPFLAGS = -I$(top_srcdir)/src -I$(top_srcdir)/src/httpserver/ METASOURCES = AUTO -check_PROGRAMS = basic file_upload http_utils threaded nodelay string_utilities http_endpoint ban_system ws_start_stop authentication deferred http_resource http_response create_webserver new_response_types daemon_info uri_log +check_PROGRAMS = basic file_upload http_utils threaded nodelay string_utilities http_endpoint ban_system ws_start_stop authentication deferred http_resource http_response create_webserver new_response_types daemon_info uri_log post_iterator_null_key MOSTLYCLEANFILES = *.gcda *.gcno *.gcov @@ -51,6 +51,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::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 an http_request +# and modded_request directly, so it needs -lmicrohttpd 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 noinst_HEADERS = littletest.hpp AM_CXXFLAGS += -Wall -fPIC -Wno-overloaded-virtual diff --git a/test/unit/post_iterator_null_key_test.cpp b/test/unit/post_iterator_null_key_test.cpp new file mode 100644 index 00000000..282007df --- /dev/null +++ b/test/unit/post_iterator_null_key_test.cpp @@ -0,0 +1,120 @@ +/* + 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 +*/ + +// White-box unit test for issue #375. webserver::post_iterator (the MHD +// post-processor callback) and http_request's constructor / set_arg are +// private, so this TU drives them directly to exercise the exact code path +// MHD hits. The standard-library and libmicrohttpd headers are pulled in +// first, normally; the `private -> public` redefinition that follows then +// only affects libhttpserver's own class declarations. Member order is +// unchanged, so object layout matches the normally-compiled library +// (only access labels differ). +#include +#include +#include +#include +#include + +#define private public +#include "./httpserver.hpp" +#include "httpserver/details/modded_request.hpp" +#undef private + +#include "./littletest.hpp" + +namespace { +// Build a modded_request owning a fresh http_request (its arg cache is +// default-initialised, so set_arg / grow_last_arg / get_arg work without a +// live MHD connection). The no-file form-arg branch never reads mr.ws, so +// it is left null. +httpserver::details::modded_request make_request() { + httpserver::details::modded_request mr; + mr.dhr = std::unique_ptr( + new httpserver::http_request()); + return mr; +} + +MHD_Result feed(httpserver::details::modded_request* mr, const char* key, + const char* data, uint64_t off, size_t size) { + return httpserver::webserver::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) + httpserver::details::modded_request mr = make_request(); + MHD_Result r = MHD_NO; + LT_CHECK_NOTHROW(r = feed(&mr, /*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(mr.dhr->get_args().size(), static_cast(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) + httpserver::details::modded_request mr = make_request(); + MHD_Result r = MHD_NO; + LT_CHECK_NOTHROW(r = feed(&mr, /*key=*/nullptr, "value", /*off=*/0, 5)); + LT_CHECK_EQ(r, MHD_YES); + LT_CHECK_EQ(mr.dhr->get_args().size(), static_cast(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) + httpserver::details::modded_request mr = make_request(); + MHD_Result r = feed(&mr, "field", "value", /*off=*/0, 5); + LT_CHECK_EQ(r, MHD_YES); + LT_CHECK_EQ(std::string(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) + httpserver::details::modded_request mr = make_request(); + LT_CHECK_EQ(feed(&mr, "field", "hel", /*off=*/0, 3), MHD_YES); + LT_CHECK_EQ(feed(&mr, "field", "lo", /*off=*/3, 2), MHD_YES); + LT_CHECK_EQ(std::string(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() From 1f591433a14e70a874ea6a3de28832067f2ea48a Mon Sep 17 00:00:00 2001 From: Sebastiano Merlino Date: Mon, 29 Jun 2026 13:16:01 -0700 Subject: [PATCH 4/4] Make post_iterator null-key test portable across stdlibs (#375) The first version of the test used `#define private public` to reach the private static webserver::post_iterator. That breaks libstdc++'s (struct __xfer_bufptrs redeclared with different access) when it is pulled in transitively under the redefined keyword, failing the Linux CI build while passing on macOS/libc++. Replace it with the explicit-instantiation access loophole: access checks do not apply to names used in an explicit template instantiation, so a filler<> whose static member runs at dynamic-init time captures a pointer to post_iterator without redefining any keyword or touching a shipped header. Portable across libstdc++ and libc++. Drop the http_request-backed happy-path cases (still covered by the file_upload integration test's 192 checks) and assert the two null-key paths return MHD_YES without throwing; dhr is left null since a correct guard returns before any dhr access. Verified: clean under -Wall -Wextra -Werror -pedantic and cpplint; the test still terminates without the guard and passes with it. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6 --- test/unit/post_iterator_null_key_test.cpp | 92 ++++++++++------------- 1 file changed, 40 insertions(+), 52 deletions(-) diff --git a/test/unit/post_iterator_null_key_test.cpp b/test/unit/post_iterator_null_key_test.cpp index 282007df..4889519e 100644 --- a/test/unit/post_iterator_null_key_test.cpp +++ b/test/unit/post_iterator_null_key_test.cpp @@ -18,46 +18,55 @@ USA */ -// White-box unit test for issue #375. webserver::post_iterator (the MHD -// post-processor callback) and http_request's constructor / set_arg are -// private, so this TU drives them directly to exercise the exact code path -// MHD hits. The standard-library and libmicrohttpd headers are pulled in -// first, normally; the `private -> public` redefinition that follows then -// only affects libhttpserver's own class declarations. Member order is -// unchanged, so object layout matches the normally-compiled library -// (only access labels differ). -#include -#include -#include -#include #include -#define private public #include "./httpserver.hpp" #include "httpserver/details/modded_request.hpp" -#undef private #include "./littletest.hpp" +// webserver::post_iterator (the MHD post-processor callback) is a private +// static member, so this test cannot name it directly. Rather than the +// `#define private public` hack -- which breaks libstdc++'s when +// it is pulled in transitively under the redefined keyword -- we use the +// explicit-instantiation access loophole: the usual access checks are not +// applied to the names that appear in an explicit template instantiation +// ([temp.spec]/6). The filler<>::instance object below runs its constructor +// at dynamic-initialisation time (before main) and stashes the captured +// pointer into post_iterator_access::ptr. No shipped header is modified and +// no language keyword is redefined, so the technique is portable across +// libstdc++ and libc++. namespace { -// Build a modded_request owning a fresh http_request (its arg cache is -// default-initialised, so set_arg / grow_last_arg / get_arg work without a -// live MHD connection). The no-file form-arg branch never reads mr.ws, so -// it is left null. -httpserver::details::modded_request make_request() { - httpserver::details::modded_request mr; - mr.dhr = std::unique_ptr( - new httpserver::http_request()); - return mr; -} +struct post_iterator_access { + using fn_t = MHD_Result (*)(void*, enum MHD_ValueKind, const char*, + const char*, const char*, const char*, + const char*, uint64_t, size_t); + static fn_t ptr; +}; +post_iterator_access::fn_t post_iterator_access::ptr = nullptr; + +template +struct filler { + filler() { post_iterator_access::ptr = Value; } + static filler instance; +}; +template +filler filler::instance; + +// The template argument names the private member; access checking does not +// apply here, so this is well-formed. +template struct filler<&httpserver::webserver::post_iterator>; + +// Invoke the captured callback for the no-file form-arg path. MHD_Result feed(httpserver::details::modded_request* mr, const char* key, const char* data, uint64_t off, size_t size) { - return httpserver::webserver::post_iterator( + return post_iterator_access::ptr( 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) @@ -73,48 +82,27 @@ LT_END_SUITE(post_iterator_null_key_suite) // 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. +// callback). The guard must instead accept and silently skip the chunk, +// returning before any field name is needed -- so dhr is deliberately left +// null here: a correct guard never reaches the std::string construction nor +// the dhr dereference. Without the guard, std::string(nullptr) throws. LT_BEGIN_AUTO_TEST(post_iterator_null_key_suite, null_key_continuation_does_not_throw) - httpserver::details::modded_request mr = make_request(); + httpserver::details::modded_request mr{}; MHD_Result r = MHD_NO; LT_CHECK_NOTHROW(r = feed(&mr, /*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(mr.dhr->get_args().size(), static_cast(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) - httpserver::details::modded_request mr = make_request(); + httpserver::details::modded_request mr{}; MHD_Result r = MHD_NO; LT_CHECK_NOTHROW(r = feed(&mr, /*key=*/nullptr, "value", /*off=*/0, 5)); LT_CHECK_EQ(r, MHD_YES); - LT_CHECK_EQ(mr.dhr->get_args().size(), static_cast(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) - httpserver::details::modded_request mr = make_request(); - MHD_Result r = feed(&mr, "field", "value", /*off=*/0, 5); - LT_CHECK_EQ(r, MHD_YES); - LT_CHECK_EQ(std::string(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) - httpserver::details::modded_request mr = make_request(); - LT_CHECK_EQ(feed(&mr, "field", "hel", /*off=*/0, 3), MHD_YES); - LT_CHECK_EQ(feed(&mr, "field", "lo", /*off=*/3, 2), MHD_YES); - LT_CHECK_EQ(std::string(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()