Skip to content

Commit 0395b50

Browse files
authored
[Fix](Exception) Fix potential use-after-free because Exception::to_string is not thread safe (#59558)
### What problem does this PR solve? Problem Summary: `Exception::to_string()` may by accessed concurrently in the following situation: - thread A and thread B access the same `DorisCallOnce` object - An Exception `e` is throw when thread A is calling `DorisCallOnce::call` and `e` is thrown to thread A - `e` will be also thrown to thread B later by DorisCallOnce - thread A and thread B access `Exception::to_string()` concurrently This may cause use-after-free due to the assignment of `_cache_string` in `Exception::to_string` Considering that `Exception` should not be frequently used, this PR construct `_cache_string` in constructor `Exception::Exception` rather than lazily creating it. This can avoid additional unnessary synchronazation. ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
1 parent a8a92f9 commit 0395b50

File tree

2 files changed

+11
-17
lines changed

2 files changed

+11
-17
lines changed

be/src/common/exception.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,14 @@ Exception::Exception(int code, const std::string_view& msg, bool from_status) {
4848
// std::cout << "Exception: " << code << ", " << msg << ", " << get_stack_trace(0, "DISABLED")
4949
// << std::endl;
5050
#endif
51+
52+
fmt::memory_buffer buf;
53+
fmt::format_to(buf, "[E{}] {}", _code, _err_msg->_msg);
54+
if (!_err_msg->_stack.empty()) {
55+
fmt::format_to(buf, "\n{}", _err_msg->_stack);
56+
}
57+
_cache_string = fmt::to_string(buf);
58+
5159
if (config::exit_on_exception) {
5260
LOG(FATAL) << "[ExitOnException] error code: " << code << ", message: " << msg;
5361
}

be/src/common/exception.h

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,9 @@ class Exception : public std::exception {
4646
int code() const { return _code; }
4747
std::string message() const { return _err_msg ? _err_msg->_msg : ""; }
4848

49-
const std::string& to_string() const;
49+
const std::string& to_string() const { return _cache_string; }
5050

51-
const char* what() const noexcept override { return to_string().c_str(); }
51+
const char* what() const noexcept override { return _cache_string.c_str(); }
5252

5353
Status to_status() const { return {code(), _err_msg->_msg, _err_msg->_stack}; }
5454

@@ -61,22 +61,8 @@ class Exception : public std::exception {
6161
std::string _stack;
6262
};
6363
std::unique_ptr<ErrMsg> _err_msg;
64-
mutable std::string _cache_string;
64+
std::string _cache_string {};
6565
};
66-
67-
inline const std::string& Exception::to_string() const {
68-
if (!_cache_string.empty()) {
69-
return _cache_string;
70-
}
71-
fmt::memory_buffer buf;
72-
fmt::format_to(buf, "[E{}] {}", _code, _err_msg ? _err_msg->_msg : "");
73-
if (_err_msg && !_err_msg->_stack.empty()) {
74-
fmt::format_to(buf, "\n{}", _err_msg->_stack);
75-
}
76-
_cache_string = fmt::to_string(buf);
77-
return _cache_string;
78-
}
79-
8066
} // namespace doris
8167

8268
#define RETURN_IF_CATCH_EXCEPTION(stmt) \

0 commit comments

Comments
 (0)