This is an automated email from the ASF dual-hosted git repository.
liaoxin01 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 2137067cccd [fix](load) Reject malformed numeric HTTP headers instead
of killing BE (#68462)
2137067cccd is described below
commit 2137067cccd3d647ffa36e6363e2365856864838
Author: Xin Liao <[email protected]>
AuthorDate: Wed Oct 14 22:51:57 2026 +0800
[fix](load) Reject malformed numeric HTTP headers instead of killing BE
(#68462)
### What problem does this PR solve?
Problem Summary:
A single raw request to the BE HTTP port takes the whole BE process
down:
PUT /api/_http_stream HTTP/1.1
Host: <be>
group_commit: async_mode
Content-Length: not_a_number
The client gets no response and times out, FE then fails queries with
`No backend available as scan node`, and `SHOW BACKENDS` reports the BE
as
not alive until it is restarted.
Root cause: `HttpStreamAction::_handle_group_commit()` parsed
`Content-Length`
with a bare `std::stoll()`. libevent invokes the header hook from its
own C
frames, so the `std::invalid_argument` thrown there unwinds through C
code and
terminates the process. `StreamLoadAction::_can_group_commit()` already
guards
the same parse with try/catch - the http stream path was simply missed.
Auditing the other numeric parses behind the BE http port, three more
are
reachable from untrusted input and unguarded, all the same one line
defect:
- `wal_id` in `HttpStreamAction::on_chunk_data()`
- the deprecated `auth_code` in `parse_basic_auth()`, which every load
action calls
- `reset_value` in `SetJeHeapProfileResetActions::handle()`, i.e. a
plain
`GET /jeheap/reset/not_a_number`
Everything else already catches, so this closes the class.
The fix adds `safe_stoll()` next to the existing `safe_stoi()` in
`string_util`
and parses all four values with it, so a malformed value fails that one
request
instead of the process:
- `Content-Length` and `wal_id` fail the load with `INVALID_ARGUMENT`
- a malformed `auth_code` is simply an invalid credential, so
authentication
fails. The value itself is kept out of the log, matching the masking
that
`is_sensitive_header()` already applies to that header
- `reset_value` answers `400`
One caller needed a fix to make the `auth_code` rejection effective:
`StreamLoad2PCAction::handle()` built the authentication error and then
overwrote it with the result of `operate_txn_2pc()`, so the request
reached FE
anyway. It now answers with the error and returns, like the other
failure paths
in that same function.
After the fix the request above is answered with an `INVALID_ARGUMENT`
status
and the BE stays alive.
---
be/src/service/http/action/http_stream.cpp | 18 ++++++++----
be/src/service/http/action/jeprofile_actions.cpp | 12 ++++++--
be/src/service/http/action/stream_load_2pc.cpp | 3 ++
be/src/service/http/utils.cpp | 11 +++++++-
be/src/util/string_util.cpp | 12 ++++++++
be/src/util/string_util.h | 2 ++
be/test/service/http/http_utils_test.cpp | 22 +++++++++++++++
be/test/service/http/stream_load_test.cpp | 36 ++++++++++++++++++++++++
be/test/util/string_util_test.cpp | 15 ++++++++++
9 files changed, 123 insertions(+), 8 deletions(-)
diff --git a/be/src/service/http/action/http_stream.cpp
b/be/src/service/http/action/http_stream.cpp
index c374b21de7d..0da3aacca8c 100644
--- a/be/src/service/http/action/http_stream.cpp
+++ b/be/src/service/http/action/http_stream.cpp
@@ -268,8 +268,14 @@ void HttpStreamAction::on_chunk_data(HttpRequest* req) {
if (ctx == nullptr || !ctx->status.ok()) {
return;
}
- if (!req->header(HTTP_WAL_ID_KY).empty()) {
- ctx->wal_id = std::stoll(req->header(HTTP_WAL_ID_KY));
+ const auto& wal_id_str = req->header(HTTP_WAL_ID_KY);
+ if (!wal_id_str.empty()) {
+ auto wal_id = safe_stoll(wal_id_str, HTTP_WAL_ID_KY);
+ if (!wal_id.has_value()) {
+ ctx->status = std::move(wal_id).error();
+ return;
+ }
+ ctx->wal_id = wal_id.value();
}
struct evhttp_request* ev_req = req->get_evhttp_request();
auto evbuf = evhttp_request_get_input_buffer(ev_req);
@@ -447,9 +453,11 @@ Status HttpStreamAction::_handle_group_commit(HttpRequest*
req,
if (config::wait_internal_group_commit_finish) {
group_commit_mode = "sync_mode";
}
- int64_t content_length = req->header(HttpHeaders::CONTENT_LENGTH).empty()
- ? 0
- :
std::stoll(req->header(HttpHeaders::CONTENT_LENGTH));
+ int64_t content_length = 0;
+ const auto& content_length_str = req->header(HttpHeaders::CONTENT_LENGTH);
+ if (!content_length_str.empty()) {
+ content_length = DORIS_TRY(safe_stoll(content_length_str,
HttpHeaders::CONTENT_LENGTH));
+ }
if (content_length < 0) {
std::stringstream ss;
ss << "This http load content length <0 (" << content_length
diff --git a/be/src/service/http/action/jeprofile_actions.cpp
b/be/src/service/http/action/jeprofile_actions.cpp
index d7702c8fb63..3ab041bd21d 100644
--- a/be/src/service/http/action/jeprofile_actions.cpp
+++ b/be/src/service/http/action/jeprofile_actions.cpp
@@ -20,6 +20,7 @@
#include <jemalloc/jemalloc.h>
#include <unistd.h>
+#include <cstdint>
#include <string>
#include "agent/utils.h"
@@ -31,6 +32,7 @@
#include "service/http/http_handler_with_auth.h"
#include "service/http/http_headers.h"
#include "service/http/http_request.h"
+#include "util/string_util.h"
namespace doris {
@@ -92,9 +94,15 @@ void SetJeHeapProfileResetActions::handle(HttpRequest* req) {
req->add_output_header(HttpHeaders::CONTENT_TYPE, HEADER_JSON.c_str());
if (compile_check(req)) {
const auto& lg_sample_str = req->param("reset_value");
- size_t lg_sample = std::stol(lg_sample_str);
+ auto parsed_lg_sample = safe_stoll(lg_sample_str, "reset_value");
+ if (!parsed_lg_sample.has_value()) {
+ HttpChannel::send_reply(req, HttpStatus::BAD_REQUEST,
+
parsed_lg_sample.error().to_string_no_stack() + "\n");
+ return;
+ }
+ int64_t lg_sample = parsed_lg_sample.value();
if (lg_sample > 0 &&
HeapProfiler::instance()->check_enable_heap_profiler() &&
- HeapProfiler::instance()->heap_profiler_reset(lg_sample)) {
+
HeapProfiler::instance()->heap_profiler_reset(static_cast<size_t>(lg_sample))) {
HttpChannel::send_reply(req, HttpStatus::OK,
fmt::format("Jemalloc reset all memory
profile statistics and "
"update the sample rate:{}\n",
diff --git a/be/src/service/http/action/stream_load_2pc.cpp
b/be/src/service/http/action/stream_load_2pc.cpp
index 15ca8066cf9..4a20b0f8b93 100644
--- a/be/src/service/http/action/stream_load_2pc.cpp
+++ b/be/src/service/http/action/stream_load_2pc.cpp
@@ -82,6 +82,9 @@ void StreamLoad2PCAction::handle(HttpRequest* req) {
if (!parse_basic_auth(*req, &ctx->auth)) {
LOG(WARNING) << "parse basic authorization failed.";
status = Status::InternalError("no valid Basic authorization");
+ status_result = status.to_json();
+ HttpChannel::send_reply(req, HttpStatus::OK, status_result);
+ return;
}
status = _exec_env->stream_load_executor()->operate_txn_2pc(ctx.get());
diff --git a/be/src/service/http/utils.cpp b/be/src/service/http/utils.cpp
index a3afe6e8a30..38de663d010 100644
--- a/be/src/service/http/utils.cpp
+++ b/be/src/service/http/utils.cpp
@@ -46,6 +46,7 @@
#include "util/md5.h"
#include "util/path_util.h"
#include "util/security.h"
+#include "util/string_util.h"
#include "util/url_coding.h"
namespace doris {
@@ -111,7 +112,15 @@ bool parse_basic_auth(const HttpRequest& req, AuthInfo*
auth) {
} else if (!auth_token.empty()) {
auth->token = auth_token;
} else if (!auth_code.empty()) {
- auth->auth_code = std::stoll(auth_code); // deprecated
+ // auth_code comes straight from a request header, a malformed one is
just an
+ // invalid credential and must not throw out of the http callback. The
value is a
+ // credential that is_sensitive_header() masks elsewhere, so it is not
logged.
+ auto parsed_auth_code = safe_stoll(auth_code, HTTP_AUTH_CODE);
+ if (!parsed_auth_code.has_value()) {
+ LOG(WARNING) << "parse auth code failed, malformed " <<
HTTP_AUTH_CODE << " header";
+ return false;
+ }
+ auth->auth_code = parsed_auth_code.value(); // deprecated
} else if (!valid_basic_auth) {
return false;
}
diff --git a/be/src/util/string_util.cpp b/be/src/util/string_util.cpp
index 259392b020d..0588a78aaa3 100644
--- a/be/src/util/string_util.cpp
+++ b/be/src/util/string_util.cpp
@@ -43,4 +43,16 @@ Result<int> safe_stoi(const std::string& input, const
std::string& name) {
std::string("'{}' value out of range: '{}', {}"), name, input,
e.what()));
}
}
+
+Result<int64_t> safe_stoll(const std::string& input, const std::string& name) {
+ try {
+ return std::stoll(input);
+ } catch (const std::invalid_argument& e) {
+ return ResultError(Status::Error<ErrorCode::INVALID_ARGUMENT>(
+ std::string("Invalid format of '{}': '{}', {}"), name, input,
e.what()));
+ } catch (const std::out_of_range& e) {
+ return ResultError(Status::Error<ErrorCode::INVALID_ARGUMENT>(
+ std::string("'{}' value out of range: '{}', {}"), name, input,
e.what()));
+ }
+}
} // namespace doris
diff --git a/be/src/util/string_util.h b/be/src/util/string_util.h
index 54a93fff8c6..6cf7f7c2956 100644
--- a/be/src/util/string_util.h
+++ b/be/src/util/string_util.h
@@ -24,6 +24,7 @@
#include <boost/tokenizer.hpp>
#include <cctype>
#include <cstddef>
+#include <cstdint>
#include <map>
#include <set>
#include <sstream>
@@ -143,6 +144,7 @@ public:
size_t hash_of_path(const std::string& identifier, const std::string& path);
Result<int> safe_stoi(const std::string& input, const std::string& name);
+Result<int64_t> safe_stoll(const std::string& input, const std::string& name);
using StringCaseSet = std::set<std::string, StringCaseLess>;
using StringCaseUnorderedSet = std::unordered_set<std::string,
StringCaseHasher, StringCaseEqual>;
template <class T>
diff --git a/be/test/service/http/http_utils_test.cpp
b/be/test/service/http/http_utils_test.cpp
index ba096bc153f..9f7942043a4 100644
--- a/be/test/service/http/http_utils_test.cpp
+++ b/be/test/service/http/http_utils_test.cpp
@@ -21,7 +21,9 @@
#include <string>
+#include "common/utils.h"
#include "gtest/gtest_pred_impl.h"
+#include "service/http/http_common.h"
#include "service/http/http_headers.h"
#include "service/http/http_request.h"
#include "service/http/utils.h"
@@ -96,4 +98,24 @@ TEST_F(HttpUtilsTest, parse_basic_auth) {
}
}
+TEST_F(HttpUtilsTest, parse_auth_code) {
+ {
+ HttpRequest req(_evhttp_req);
+ req._headers.emplace(HttpHeaders::AUTHORIZATION,
encode_basic_auth("doris", "passwd"));
+ req._headers.emplace(HTTP_AUTH_CODE, "1234567890123");
+ AuthInfo auth;
+ EXPECT_TRUE(parse_basic_auth(req, &auth));
+ EXPECT_EQ(1234567890123, auth.auth_code);
+ }
+ {
+ // a malformed auth code is rejected instead of throwing out of the
http callback
+ HttpRequest req(_evhttp_req);
+ req._headers.emplace(HttpHeaders::AUTHORIZATION,
encode_basic_auth("doris", "passwd"));
+ req._headers.emplace(HTTP_AUTH_CODE, "not_a_number");
+ AuthInfo auth;
+ EXPECT_FALSE(parse_basic_auth(req, &auth));
+ EXPECT_EQ(-1, auth.auth_code);
+ }
+}
+
} // namespace doris
diff --git a/be/test/service/http/stream_load_test.cpp
b/be/test/service/http/stream_load_test.cpp
index 93dd9d044c7..1405092f401 100644
--- a/be/test/service/http/stream_load_test.cpp
+++ b/be/test/service/http/stream_load_test.cpp
@@ -195,4 +195,40 @@ TEST_F(StreamLoadTest,
HttpStreamBodySizeLimitPlusOneErrorIncludesExactBytes) {
"this load is reasonable");
evhttp_request_free(evhttp_req);
}
+
+// A malformed Content-Length used to escape as an exception from the libevent
header
+// callback and take the whole BE process down.
+TEST_F(StreamLoadTest, HttpStreamMalformedContentLengthIsRejected) {
+ for (const auto* content_length : {"not_a_number", "9223372036854775808"})
{
+ auto* evhttp_req = evhttp_request_new(nullptr, nullptr);
+ HttpRequest req(evhttp_req);
+ req.set_header(HTTP_GROUP_COMMIT, "async_mode");
+ req.set_header(HttpHeaders::CONTENT_LENGTH, content_length);
+
+ HttpStreamAction action(nullptr);
+ auto ctx = std::make_shared<StreamLoadContext>(nullptr);
+ auto status = action._handle_group_commit(&req, ctx);
+
+ EXPECT_TRUE(status.is<ErrorCode::INVALID_ARGUMENT>()) <<
content_length;
+ evhttp_request_free(evhttp_req);
+ }
+}
+
+TEST_F(StreamLoadTest, HttpStreamMalformedWalIdIsRejected) {
+ auto* evhttp_req = evhttp_request_new(nullptr, nullptr);
+ HttpRequest req(evhttp_req);
+ req.set_header(HTTP_WAL_ID_KY, "not_a_number");
+
+ HttpStreamAction action(nullptr);
+ auto ctx = std::make_shared<StreamLoadContext>(nullptr);
+ req.set_handler(&action);
+ req.set_handler_ctx(ctx);
+
+ action.on_chunk_data(&req);
+
+ EXPECT_TRUE(ctx->status.is<ErrorCode::INVALID_ARGUMENT>());
+ // ctx carries no ExecEnv, so detach it before ~HttpRequest calls
free_handler_ctx()
+ req.set_handler_ctx(nullptr);
+ evhttp_request_free(evhttp_req);
+}
} // namespace doris
diff --git a/be/test/util/string_util_test.cpp
b/be/test/util/string_util_test.cpp
index 77b090be4c2..8a8bb3a92da 100644
--- a/be/test/util/string_util_test.cpp
+++ b/be/test/util/string_util_test.cpp
@@ -200,4 +200,19 @@ TEST_F(StringUtilTest, normal) {
}
}
+TEST_F(StringUtilTest, safe_stoll) {
+ EXPECT_EQ(0, safe_stoll("0", "content-length").value());
+ EXPECT_EQ(-1, safe_stoll("-1", "content-length").value());
+ // beyond the range of safe_stoi
+ EXPECT_EQ(9223372036854775807LL, safe_stoll("9223372036854775807",
"content-length").value());
+
+ auto invalid = safe_stoll("not_a_number", "content-length");
+ EXPECT_FALSE(invalid.has_value());
+ EXPECT_TRUE(invalid.error().is<ErrorCode::INVALID_ARGUMENT>());
+
+ auto out_of_range = safe_stoll("9223372036854775808", "content-length");
+ EXPECT_FALSE(out_of_range.has_value());
+ EXPECT_TRUE(out_of_range.error().is<ErrorCode::INVALID_ARGUMENT>());
+}
+
} // namespace doris
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
