github-actions[bot] commented on code in PR #68462:
URL: https://github.com/apache/doris/pull/68462#discussion_r4089051248
##########
be/src/service/http/ev_http_server.cpp:
##########
@@ -49,9 +50,31 @@ struct evhttp;
namespace doris {
+// libevent runs the hooks below from its own C frames, so an exception
escaping a
+// handler would unwind through them and terminate the whole BE process.
Handlers are
+// expected to report failures through Status, and this is the last resort
that keeps a
+// missed one from taking the process down: the exception is logged and the
request is
+// failed, never the BE.
+// raw_path() is logged instead of uri() to keep query string credentials out
of the log.
+template <typename Fn>
+static bool catch_handler_exception(const char* hook, const HttpRequest*
request, Fn&& fn) {
+ try {
+ fn();
+ return true;
+ } catch (const std::exception& e) {
+ LOG(WARNING) << "http handler throws exception in " << hook
+ << ", path=" << request->raw_path() << ", error=" <<
e.what();
+ } catch (...) {
+ LOG(WARNING) << "http handler throws unknown exception in " << hook
+ << ", path=" << request->raw_path();
+ }
+ return false;
+}
+
static void on_chunked(struct evhttp_request* ev_req, void* param) {
HttpRequest* request = (HttpRequest*)ev_req->on_free_cb_arg;
- request->handler()->on_chunk_data(request);
+ catch_handler_exception("on_chunk_data", request,
Review Comment:
[P1] Abort progressive requests when a chunk handler throws
This also ignores the failed catch result. The pinned libevent drains the
callback buffer and continues reading before eventually invoking `handle()`, so
the logged exception does not actually fail the request. For example,
`StreamLoadForwardHandler` removes bytes before
`request_data_chunks.emplace_back`; if that allocation throws, this boundary
catches it and the terminal handler can forward only the previously queued
chunks. Please terminate or poison the request here so terminal commit or
forwarding cannot proceed on partial input.
##########
be/src/service/http/ev_http_server.cpp:
##########
@@ -66,7 +89,7 @@ static void on_request(struct evhttp_request* ev_req, void*
arg) {
// In this case, request's on_header return -1
return;
}
- request->handler()->handle(request);
+ catch_handler_exception("handle", request, [request] {
request->handler()->handle(request); });
Review Comment:
[P1] Complete the request after a caught terminal exception
When `handle()` throws, this return value is ignored, so the callback
returns without sending a reply or closing/canceling the libevent request. This
is reachable with `/jeheap/reset/not_a_number`, whose handler calls
`std::stol`; libevent then leaves the request in its writing state, and Doris
configures no server timeout. A client disconnect can also detach the
unfinished request without running `on_free`, retaining its `HttpRequest` and
handler context. Please make the failed catch path deterministically send an
error or terminate the request, with reply-state handling to avoid a double
reply.
##########
be/src/service/http/utils.cpp:
##########
@@ -111,7 +112,14 @@ 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
+ auto parsed_auth_code = safe_stoll(auth_code, HTTP_AUTH_CODE);
+ if (!parsed_auth_code.has_value()) {
+ LOG(WARNING) << "parse auth code failed: " <<
parsed_auth_code.error();
Review Comment:
[P2] Keep the malformed auth code out of logs
`safe_stoll()` includes the original input in its error `Status`, so this
warning writes the full malformed `auth_code` even though
`HttpRequest::is_sensitive_header()` explicitly masks that header elsewhere.
For example, an out-of-range credential is reproduced verbatim in the BE
warning log. Please log only the field name/error category, or otherwise redact
the input.
##########
be/src/service/http/utils.cpp:
##########
@@ -111,7 +112,14 @@ 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
+ auto parsed_auth_code = safe_stoll(auth_code, HTTP_AUTH_CODE);
+ if (!parsed_auth_code.has_value()) {
+ LOG(WARNING) << "parse auth code failed: " <<
parsed_auth_code.error();
+ return false;
Review Comment:
[P1] Stop stream-load 2PC after this auth failure
`StreamLoad2PCAction::handle()` does not return when this branch reports
failure: it stores an error and immediately overwrites it with
`operate_txn_2pc(ctx.get())`. Because valid Basic credentials were already
copied into `ctx->auth` before `auth_code` was parsed, a request with valid
Basic auth plus malformed `auth_code` can still reach FE and commit or abort
the transaction. Please make that caller send the authentication error and
return before invoking the executor, and cover the caller rather than only this
helper.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
