Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/detail/webserver_callbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
10 changes: 9 additions & 1 deletion test/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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:
Expand Down
126 changes: 126 additions & 0 deletions test/unit/post_iterator_null_key_test.cpp
Original file line number Diff line number Diff line change
@@ -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()