liaoxin01 commented on code in PR #68462:
URL: https://github.com/apache/doris/pull/68462#discussion_r4260776870
##########
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:
Fixed. The guard no longer just swallows the exception. `HttpRequest` now
tracks whether a reply was already handed to libevent (`mark_reply_sent()` is
set at every `evhttp_send_reply`/`evhttp_send_error` site in `HttpChannel`,
plus the two direct sends in `StreamLoadForwardHandler`), and `on_request`
answers with `500` via `send_handler_exception_reply()` when `handle()` throws
and nothing was sent. `EvHttpServer::on_header` does the same instead of
returning -1. The reply-state check is what keeps a handler that replied and
then threw from double replying, and `forward_request_done()` now bails out
when the original request was already answered.
##########
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:
Fixed. An exception from `on_chunk_data` now calls
`request->mark_handler_failed()`. `on_request` checks that flag first and
answers with `500` without running the terminal handler, so neither a commit
nor a forward can proceed on partial input. Subsequent chunk callbacks on a
failed request drain the input buffer instead of accumulating the rest of the
body.
##########
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:
Fixed, and you are right that the caller is the real problem.
`StreamLoad2PCAction::handle()` built the authentication error and then
unconditionally overwrote it with `operate_txn_2pc()`. It now serialises the
error and returns, matching the other failure paths in that same function.
##########
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:
Fixed. The warning no longer goes through the `safe_stoll()` status; it logs
only the header name, so the malformed credential never reaches the log.
Comment added pointing at `is_sensitive_header()` as the reason.
--
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]
