Fix std::terminate when uri_log receives null uri (#371)#372
Merged
Conversation
libmicrohttpd may invoke MHD_OPTION_URI_LOG_CALLBACK with a null uri
pointer before the request line is parsed - for example on port scans,
TLS clients hitting a plain HTTP port, or half-open connections. The
previous code assigned the raw pointer directly into a std::string,
which throws std::logic_error("basic_string::_M_construct null not
valid"). Because the throw originates inside an MHD C callback with
no enclosing handler, std::terminate() was called and the process
aborted under load.
Treat a null uri as an empty string so the assignment is well-defined.
An empty URI fails to match any registered resource and surfaces as a
404, which is the correct graceful behaviour.
Resolves #371.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #372 +/- ##
=======================================
Coverage 67.97% 67.97%
=======================================
Files 28 28
Lines 1636 1636
Branches 672 672
=======================================
Hits 1112 1112
Misses 64 64
Partials 460 460
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
The CodeQL workflow was still pulling libmicrohttpd-0.9.64 from S3, which is below the project's stated minimum of 1.0.0 and is no longer served by the bucket - the install step was failing with "gzip: stdin: not in gzip format" because curl received a 243-byte error response instead of the tarball. Bump to 1.0.3 from the same S3 location so CodeQL can build the project again.
Aligns release.yml and verify-build.yml with codeql-analysis.yml so all workflows pull the same libmicrohttpd-1.0.3.tar.gz from S3. This also brings CI in line with the project's documented minimum of >= 1.0.0 (0.9.77 was below that threshold). Cache keys include the new version so existing 0.9.77 entries are not reused.
Adds test/unit/uri_log_test.cpp to lock in the fix for issue #371. The test calls uri_log() directly (re-declaring the symbol since it has no public header) and verifies three cases: - null uri does not throw and yields an empty complete_uri - valid uri is stored verbatim - empty uri is stored verbatim The first case is the regression check: against the unfixed code, running the test crashes the process (SIGSEGV from dereferencing the null pointer inside std::string's assignment operator on libstdc++ 13; on the older libstdc++ 10 from the bug report it threw std::logic_error and aborted via std::terminate). With the fix in place, all three sub- tests pass cleanly. The new test target needs an explicit -lmicrohttpd in its link line because it instantiates ~modded_request() directly, which references MHD_destroy_post_processor; the default LDADD only pulls libmicrohttpd in transitively via libhttpserver.la, and modern ld enforces --no-copy-dt-needed-entries.
cpplint flags bare "httpserver.hpp" with build/include_subdir [4]. Match the convention used by every other test file in the repo and prefix the include with "./" so cpplint considers the directory explicit.
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
1 participant
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.
Summary
uri_log()insrc/webserver.cppagainst a nulluripointer from libmicrohttpd, treating it as an empty string instead of lettingstd::string's assignment operator throwstd::logic_error.Background
As reported in #371, libmicrohttpd can invoke
MHD_OPTION_URI_LOG_CALLBACKwith a nulluripointer before the request line has been parsed — e.g. when a TCP connection is accepted but the client disconnects, sends non-HTTP data, or a TLS client hits a plain HTTP port. The previous code assigned the raw pointer directly intomr->complete_uri(astd::string), which throwsstd::logic_error(\"basic_string::_M_construct null not valid\"). Because the throw originates inside an MHD C callback with no enclosing handler,std::terminate()was called and the process aborted under load.The reporter's stack trace pointed at
src/webserver.cppline 423 in 0.19.0; the equivalent line on master is nowmr->complete_uri = uri;(line 629), but the bug is identical —std::string::operator=(const char*)is undefined for a null pointer and libstdc++ throws.Fix
An empty URI simply fails to match any registered resource and surfaces as a 404, which is the correct graceful behaviour and matches the suggestion in the issue.
Test plan
makesucceeds with-Werror -Wall -Wextra -pedanticmake check— all 14 tests pass (basic, file_upload, http_utils, threaded, nodelay, string_utilities, http_endpoint, ban_system, ws_start_stop, authentication, deferred, http_resource, http_response, create_webserver)Resolves #371.