From 9e25fce5b989c2001753197568babe9281c36e11 Mon Sep 17 00:00:00 2001 From: tqchen Date: Sun, 6 Sep 2026 14:10:57 +0000 Subject: [PATCH] [WIN] Enhance threadsafety of DbgHelp DbgHelp is single-threaded and the caller must serialize it, but TVMFFIBacktrace called it unguarded. An exception raised while another thread was formatting a backtrace crashed the process inside the reporter, so the original error was never printed. Guard every DbgHelp call with one mutex. Key the session on a duplicated process handle rather than GetCurrentProcess(), whose value is shared by every component, so another DbgHelp user cannot close our symbols. Initialize once and keep the session instead of doing it per call, which also removes module enumeration from the locked region. A failed initialization now yields a backtrace carrying only the caller's own frame instead of walking against a session that was never established. --- src/ffi/backtrace_win.cc | 56 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/src/ffi/backtrace_win.cc b/src/ffi/backtrace_win.cc index ca5cf06d6..6f782b90a 100644 --- a/src/ffi/backtrace_win.cc +++ b/src/ffi/backtrace_win.cc @@ -32,10 +32,51 @@ #include #include +#include #include #include "./backtrace_utils.h" +namespace { + +/*! \brief Global singleton holding tvm_ffi's DbgHelp symbol session. */ +struct DbgHelpSession { + /*! \brief Serializes every DbgHelp call made by tvm_ffi. */ + std::mutex mutex; + /*! \brief Session handle, or nullptr when no session could be established. */ + HANDLE handle = nullptr; + + static DbgHelpSession* Global() { + static DbgHelpSession inst; + return &inst; + } + + DbgHelpSession() { + HANDLE current_process_handle = GetCurrentProcess(); + // Duplicate rather than key the session on GetCurrentProcess() directly: that + // value is the same in every component, so another DbgHelp user in this process + // could close our symbols. A duplicated handle is a key only we hold. + if (!DuplicateHandle(current_process_handle, current_process_handle, current_process_handle, + &handle, 0, FALSE, DUPLICATE_SAME_ACCESS)) { + handle = nullptr; + return; + } + // LOAD_LINES keeps file and line information; UNDNAME demangles C++ names. + SymSetOptions(SYMOPT_LOAD_LINES | SYMOPT_UNDNAME); + // TRUE enumerates loaded modules now, so no module is discovered later while + // the mutex is held. Unlike the previous code the result is checked: a session + // that failed to initialize must not be cached as usable. + if (!SymInitialize(handle, NULL, TRUE)) { + // Nothing was filed under this key, so there is no session to SymCleanup; + // release the handle and leave it null to report the failure. + CloseHandle(handle); + handle = nullptr; + } + } +}; + +} // namespace + const TVMFFIByteArray* TVMFFIBacktrace(const char* filename, int lineno, const char* func, int cross_ffi_boundary) { static thread_local std::string backtrace_str; @@ -51,11 +92,19 @@ const TVMFFIByteArray* TVMFFIBacktrace(const char* filename, int lineno, const c backtrace.Append(filename, func, lineno); } - HANDLE process = GetCurrentProcess(); HANDLE thread = GetCurrentThread(); - SymSetOptions(SYMOPT_LOAD_LINES | SYMOPT_UNDNAME); - SymInitialize(process, NULL, TRUE); + DbgHelpSession* session = DbgHelpSession::Global(); + std::lock_guard lock(session->mutex); + HANDLE process = session->handle; + if (process == nullptr) { + // StackWalk64 resolves through the session, so without one there is nothing + // to walk. Report the caller's own frame instead of using a dead session. + backtrace_str = backtrace.GetBacktrace(); + backtrace_array.data = backtrace_str.data(); + backtrace_array.size = backtrace_str.size(); + return &backtrace_array; + } CONTEXT context = {}; RtlCaptureContext(&context); @@ -138,7 +187,6 @@ const TVMFFIByteArray* TVMFFIBacktrace(const char* filename, int lineno, const c } backtrace.Append(filename, symbol, lineno); } - SymCleanup(process); backtrace_str = backtrace.GetBacktrace(); backtrace_array.data = backtrace_str.data(); backtrace_array.size = backtrace_str.size();