diff --git a/src/detail/webserver_callbacks.cpp b/src/detail/webserver_callbacks.cpp index 11a164d5..938326f2 100644 --- a/src/detail/webserver_callbacks.cpp +++ b/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. diff --git a/test/Makefile.am b/test/Makefile.am index 8571301f..eed18df7 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/ -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: 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..5dc9aaa9 --- /dev/null +++ b/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 + +#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 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(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(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()