Skip to content

Commit ef668a4

Browse files
etrclaude
andauthored
Fix null key dereference in post_iterator (#375) (#377)
* 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6 * Add ChangeLog entry for #375 fix Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6 * 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6 * 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 <sstream> (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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 8b6aeb0 commit ef668a4

4 files changed

Lines changed: 134 additions & 1 deletion

File tree

ChangeLog

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,9 @@ Version 0.20.0
4242
Fixed std::terminate when MHD invokes the URI log callback with a
4343
null uri pointer (e.g. port scans, half-open connections, or
4444
non-HTTP traffic). Resolves issue #371.
45+
Fixed std::terminate when MHD invokes the post-processor iterator with
46+
a null key on a continuation chunk (a field split across multiple
47+
callbacks). Resolves issue #375.
4548

4649
Version 0.19.0 - 2023-06-15
4750

src/webserver.cpp

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -832,6 +832,20 @@ MHD_Result webserver::post_iterator(void *cls, enum MHD_ValueKind kind,
832832
struct details::modded_request* mr = (struct details::modded_request*) cls;
833833

834834
if (!filename) {
835+
// MHD may invoke the post iterator with a null key on a
836+
// continuation chunk (off > 0): the field name was supplied on the
837+
// first call and is not repeated. With no field name there is
838+
// nothing to store the value under, so silently accept the chunk
839+
// (MHD_YES tells MHD to continue; MHD_NO would abort the whole
840+
// request). Guarding here also stops the raw pointer from reaching
841+
// std::string, which throws std::logic_error on null and aborts the
842+
// process via std::terminate because the throw escapes a C
843+
// callback. See issue #375 (same class of bug as the null-uri fix
844+
// in uri_log, issue #371).
845+
if (!key) {
846+
return MHD_YES;
847+
}
848+
835849
// There is no actual file, just set the arg key/value and return.
836850
if (off > 0) {
837851
mr->dhr->grow_last_arg(key, std::string(data, size));

test/Makefile.am

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ LDADD += -lcurl
2626

2727
AM_CPPFLAGS = -I$(top_srcdir)/src -I$(top_srcdir)/src/httpserver/
2828
METASOURCES = AUTO
29-
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
29+
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
3030

3131
MOSTLYCLEANFILES = *.gcda *.gcno *.gcov
3232

@@ -51,6 +51,14 @@ uri_log_SOURCES = unit/uri_log_test.cpp
5151
# it needs an explicit -lmicrohttpd in its link line on top of the default
5252
# LDADD (modern ld enforces --no-copy-dt-needed-entries).
5353
uri_log_LDADD = $(LDADD) -lmicrohttpd
54+
# post_iterator_null_key: issue #375 regression. Drives the static
55+
# webserver::post_iterator callback with a null key (the continuation-chunk
56+
# case MHD hits when a field is split across callbacks) and asserts it no
57+
# longer throws std::logic_error / terminates. Constructs an http_request
58+
# and modded_request directly, so it needs -lmicrohttpd the same way
59+
# uri_log does.
60+
post_iterator_null_key_SOURCES = unit/post_iterator_null_key_test.cpp
61+
post_iterator_null_key_LDADD = $(LDADD) -lmicrohttpd
5462

5563
noinst_HEADERS = littletest.hpp
5664
AM_CXXFLAGS += -Wall -fPIC -Wno-overloaded-virtual
Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
/*
2+
This file is part of libhttpserver
3+
Copyright (C) 2011-2019 Sebastiano Merlino
4+
5+
This library is free software; you can redistribute it and/or
6+
modify it under the terms of the GNU Lesser General Public
7+
License as published by the Free Software Foundation; either
8+
version 2.1 of the License, or (at your option) any later version.
9+
10+
This library is distributed in the hope that it will be useful,
11+
but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
13+
Lesser General Public License for more details.
14+
15+
You should have received a copy of the GNU Lesser General Public
16+
License along with this library; if not, write to the Free Software
17+
Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
18+
USA
19+
*/
20+
21+
#include <microhttpd.h>
22+
23+
#include "./httpserver.hpp"
24+
#include "httpserver/details/modded_request.hpp"
25+
26+
#include "./littletest.hpp"
27+
28+
// webserver::post_iterator (the MHD post-processor callback) is a private
29+
// static member, so this test cannot name it directly. Rather than the
30+
// `#define private public` hack -- which breaks libstdc++'s <sstream> when
31+
// it is pulled in transitively under the redefined keyword -- we use the
32+
// explicit-instantiation access loophole: the usual access checks are not
33+
// applied to the names that appear in an explicit template instantiation
34+
// ([temp.spec]/6). The filler<>::instance object below runs its constructor
35+
// at dynamic-initialisation time (before main) and stashes the captured
36+
// pointer into post_iterator_access::ptr. No shipped header is modified and
37+
// no language keyword is redefined, so the technique is portable across
38+
// libstdc++ and libc++.
39+
namespace {
40+
41+
struct post_iterator_access {
42+
using fn_t = MHD_Result (*)(void*, enum MHD_ValueKind, const char*,
43+
const char*, const char*, const char*,
44+
const char*, uint64_t, size_t);
45+
static fn_t ptr;
46+
};
47+
post_iterator_access::fn_t post_iterator_access::ptr = nullptr;
48+
49+
template <post_iterator_access::fn_t Value>
50+
struct filler {
51+
filler() { post_iterator_access::ptr = Value; }
52+
static filler instance;
53+
};
54+
template <post_iterator_access::fn_t Value>
55+
filler<Value> filler<Value>::instance;
56+
57+
// The template argument names the private member; access checking does not
58+
// apply here, so this is well-formed.
59+
template struct filler<&httpserver::webserver::post_iterator>;
60+
61+
// Invoke the captured callback for the no-file form-arg path.
62+
MHD_Result feed(httpserver::details::modded_request* mr, const char* key,
63+
const char* data, uint64_t off, size_t size) {
64+
return post_iterator_access::ptr(
65+
mr, MHD_POSTDATA_KIND, key, /*filename=*/nullptr,
66+
/*content_type=*/nullptr, /*transfer_encoding=*/nullptr,
67+
data, off, size);
68+
}
69+
70+
} // namespace
71+
72+
LT_BEGIN_SUITE(post_iterator_null_key_suite)
73+
void set_up() {
74+
}
75+
76+
void tear_down() {
77+
}
78+
LT_END_SUITE(post_iterator_null_key_suite)
79+
80+
// Regression test for issue #375: MHD may invoke the post iterator with a
81+
// null key on a continuation chunk (off > 0) because the field name was
82+
// only supplied on the first call. The previous implementation passed the
83+
// raw key pointer into std::string, which throws std::logic_error on null
84+
// and aborts the process via std::terminate (the throw escapes a C
85+
// callback). The guard must instead accept and silently skip the chunk,
86+
// returning before any field name is needed -- so dhr is deliberately left
87+
// null here: a correct guard never reaches the std::string construction nor
88+
// the dhr dereference. Without the guard, std::string(nullptr) throws.
89+
LT_BEGIN_AUTO_TEST(post_iterator_null_key_suite, null_key_continuation_does_not_throw)
90+
httpserver::details::modded_request mr{};
91+
MHD_Result r = MHD_NO;
92+
LT_CHECK_NOTHROW(r = feed(&mr, /*key=*/nullptr, "value", /*off=*/5, 5));
93+
// MHD_YES keeps the request alive; MHD_NO would abort it.
94+
LT_CHECK_EQ(r, MHD_YES);
95+
LT_END_AUTO_TEST(null_key_continuation_does_not_throw)
96+
97+
// Same guard on the initial-chunk path (off == 0). MHD should not normally
98+
// hand us a null key here, but the guard is unconditional, so pin it.
99+
LT_BEGIN_AUTO_TEST(post_iterator_null_key_suite, null_key_initial_does_not_throw)
100+
httpserver::details::modded_request mr{};
101+
MHD_Result r = MHD_NO;
102+
LT_CHECK_NOTHROW(r = feed(&mr, /*key=*/nullptr, "value", /*off=*/0, 5));
103+
LT_CHECK_EQ(r, MHD_YES);
104+
LT_END_AUTO_TEST(null_key_initial_does_not_throw)
105+
106+
LT_BEGIN_AUTO_TEST_ENV()
107+
AUTORUN_TESTS()
108+
LT_END_AUTO_TEST_ENV()

0 commit comments

Comments
 (0)