Fix null key dereference in post_iterator (#375)#377
Merged
Conversation
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #377 +/- ##
==========================================
+ Coverage 68.03% 68.07% +0.03%
==========================================
Files 34 34
Lines 1730 1732 +2
Branches 697 698 +1
==========================================
+ Hits 1177 1179 +2
Misses 80 80
Partials 473 473
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6
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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #375.
Problem
MHD may invoke
webserver::post_iteratorwith a nullkeyon a continuation chunk (off > 0) — the field name is supplied only on the first callback and not repeated when a field is split across callbacks (the large-field path from #337 / commit1b5fe8f).The no-file branch passed the raw
keypointer straight intostd::string(viagrow_last_arg/set_arg), which throwsstd::logic_erroron null. Because the throw escapes the C post-iterator callback, it propagates as an uncaught exception and aborts the process viastd::terminate.Fix
Guard
keyfor null before constructing astd::string. With no field name there is nothing to store the value under, so accept and silently skip the chunk:MHD_YEStells MHD to continue (MHD_NOwould abort the whole request). This is the same class of bug — and the same fix shape — as the null-uriguard inuri_log(#371).Testing
webserver::post_iteratoris a private static member with no test seam (unlikeuri_log, which is a free function the unit test re-declares), so this change is covered by the existingfile_uploadintegration test — its 192 checks exercise the normal multipart/form POST path and stay green, confirming no regression to legitimate form/file handling. A dedicated null-key regression unit test ships on thefeature/v2.0branch (#376), wherepost_iteratorwas refactored into a publicly testable static onwebserver_impl.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6