Helps Clang detect certain dangling reference issues, in a similar vein as #36164.
Known limitations
It doesn't catch invalidation nor brace-initialization.
<details><summary>Diff illustrating limitations</summary>
--- a/src/test/httpserver_tests.cpp
+++ b/src/test/httpserver_tests.cpp
@@ -80,6 +80,15 @@ BOOST_AUTO_TEST_CASE(test_query_parameters)
BOOST_AUTO_TEST_CASE(http_headers_tests)
{
+ auto foo = HTTPHeaders{}.FindAll("needle"); // Emits warning
+ (void)foo;
+ auto bar{HTTPHeaders{}.FindAll("needle")}; // No warning with Clang 22.1.8 :/
+ (void)bar;
+
+ HTTPHeaders test;
+ auto baz = test.FindAll("needle");
+ test.Write("needle", "mutation"); // No warning with Clang 22.1.8 :/
+
{
// Writing response headers
HTTPHeaders headers{};
</details>
Clang 24 has experimental invalidation detection so maybe that could be used in the far future: https://clang.llvm.org/docs/LifetimeSafety.html#use-after-invalidation-experimental
Alternative solution A)
Return by copy everywhere. Might introduce more heap activity, especially in the case of HTTPRemoteClient::GetRequest().
Alternative solution B)
Refactor the methods to minimize copying while still making things more memory-safe. Replacing HTTPHeaders::FindAll() with an Iterate()-function taking a lambda which gets to process each header. Gets rid of the heap activity of building a vector but introduces copying of first.
<details><summary>Diff of httpserver.cpp/h</summary>
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -272,15 +272,11 @@ std::optional<std::string> HTTPHeaders::FindFirst(const std::string_view key) co
return std::nullopt;
}
-std::vector<std::string_view> HTTPHeaders::FindAll(const std::string_view key) const
+void HTTPHeaders::Iterate(std::function<void(const std::string& key, const std::string& value)> fn) const
{
- std::vector<std::string_view> ret;
for (const auto& item : m_headers) {
- if (CaseInsensitiveEqual(key, item.first)) {
- ret.push_back(item.second);
- }
+ fn(item.first, item.second);
}
- return ret;
}
void HTTPHeaders::Write(std::string&& key, std::string&& value)
@@ -504,18 +500,21 @@ bool HTTPRequest::LoadBody(LineReader& reader)
// We read all the chunks but never got the last chunk, wait for client to send more
return false;
} else {
+ std::optional<std::string> first;
+ m_headers.Iterate([&first] (const std::string& key, const std::string& value) {
+ if (!CaseInsensitiveEqual(key, "Content-Length")) return;
+ if (!first.has_value()) {
+ first = value;
+ } else if (first != value) {
+ // Duplicate Content-Length headers are allowed only if they all have the same value
+ // https://www.rfc-editor.org/rfc/rfc7230#section-3.3.3
+ throw std::runtime_error("Differing Content-Length values");
+ }
+ });
// No Content-length or Transfer-Encoding header means no body, see libevent evhttp_get_body()
- auto content_length_values{m_headers.FindAll("Content-Length")};
- if (content_length_values.empty()) return true;
-
- // Duplicate Content-Length headers are allowed only if they all have the same value
- // https://www.rfc-editor.org/rfc/rfc7230#section-3.3.3
- const auto& first_content_length_value{content_length_values[0]};
- for (size_t i = 1; i < content_length_values.size(); ++i) {
- if (content_length_values[i] != first_content_length_value) throw std::runtime_error("Differing Content-Length values");
- }
+ if (!first.has_value()) return true;
- const auto content_length{ToIntegral<uint64_t>(first_content_length_value)};
+ const auto content_length{ToIntegral<uint64_t>(first.value())};
if (!content_length) throw std::runtime_error("Cannot parse Content-Length value");
if (*content_length > MAX_BODY_SIZE) throw ContentTooLargeError("Max body size exceeded");
--- a/src/httpserver.h
+++ b/src/httpserver.h
@@ -97,10 +97,9 @@ public:
*/
std::optional<std::string> FindFirst(std::string_view key) const;
/**
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] key The field-name of the header to search for
- * [@returns](/bitcoin-bitcoin/contributor/returns/) Views into all values matching the provided key (valid while this object is alive)
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] fn Receives each header as they are iterated through.
*/
- std::vector<std::string_view> FindAll(std::string_view key) const LIFETIMEBOUND;
+ void Iterate(std::function<void(const std::string& key, const std::string& value)> fn) const;
void Write(std::string&& key, std::string&& value);
/**
* [@param](/bitcoin-bitcoin/contributor/param/)[in] key The field-name of the header to search for and delete
</details>
Rationale
The methods are not called in many places so risk of misuse is low, and we avoid any risk of performance degradation (such as the one found in #35182 (review)). Returning copies without adding mutexes or other thread safety measures does not considerably increase thread-safety.
