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 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)); 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..4889519e --- /dev/null +++ b/test/unit/post_iterator_null_key_test.cpp @@ -0,0 +1,108 @@ +/* + 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 + +#include "./httpserver.hpp" +#include "httpserver/details/modded_request.hpp" + +#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 { + +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 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) + 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, +// 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{}; + 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); +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{}; + MHD_Result r = MHD_NO; + LT_CHECK_NOTHROW(r = feed(&mr, /*key=*/nullptr, "value", /*off=*/0, 5)); + LT_CHECK_EQ(r, MHD_YES); +LT_END_AUTO_TEST(null_key_initial_does_not_throw) + +LT_BEGIN_AUTO_TEST_ENV() + AUTORUN_TESTS() +LT_END_AUTO_TEST_ENV()