Skip to content

Conversation

@un33k
Copy link

@un33k un33k commented Aug 17, 2025

Description

This PR fixes crashes that occur during program termination due to static mutex destruction order issues. When static
mutexes are destroyed while other static objects still reference them, it causes undefined behavior and crashes.

Motivation and Context

Static destruction order in C++ is undefined across translation units. This causes crashes when:

  • Logging infrastructure attempts to use mutexes during cleanup
  • Multiple static objects have interdependencies
  • Cleanup order varies between platforms/configurations

Changes

The fix uses heap-allocated mutexes (intentionally leaked) for critical static mutexes to ensure they remain valid
throughout program lifetime:

  1. DefaultLoggerMutex() in logging.cc - Used by logging infrastructure
  2. GetOrtEnvMutex() in ort_env.cc - Used by ORT environment management
  3. GetActiveSessionsMutex() in inference_session.cc - Windows-specific session tracking

Technical Details

  • Uses new std::mutex() without corresponding delete (intentional leak)
  • The leaked memory is negligible (24-40 bytes per mutex)
  • This is a common C++ pattern for avoiding static destruction issues
  • Similar approach used in many production systems (LLVM, Chrome, etc.)

Testing

  • Tested on macOS (ARM64)
  • Existing test suite passes
  • Additional testing on Windows and Linux would be appreciated

Note

This issue was discovered while working on the Rust-based TTS library which uses ONNX Runtime. The crashes were occurring during cleanup when the library was unloaded.

During program termination, static mutexes can be destroyed while other
static objects still reference them, causing crashes. This commonly occurs
when logging during cleanup or when cleanup happens in an unexpected order.

This fix uses heap-allocated mutexes (intentionally leaked) for critical
static mutexes to ensure they remain valid throughout program lifetime:

1. DefaultLoggerMutex() in logging.cc - Used by logging infrastructure
2. GetOrtEnvMutex() in ort_env.cc - Used by ORT environment management
3. GetActiveSessionsMutex() in inference_session.cc - Windows-specific session tracking

The intentional memory leak is negligible (a few bytes per mutex) and only
occurs at program termination. This is a common pattern for avoiding static
destruction order issues in C++.

Fixes crashes during program termination when:
- Logging occurs during static destruction
- Multiple static objects have interdependencies
- Cleanup order varies between platforms/configurations
@un33k un33k force-pushed the fix/static-mutex-destruction-order branch from c4d4c8e to 25a2444 Compare August 17, 2025 21:43
@un33k
Copy link
Author

un33k commented Aug 17, 2025

@microsoft-github-policy-service agree [company="{your company}"]

@un33k please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree [company="{Neekware Inc.}"]

@un33k
Copy link
Author

un33k commented Aug 17, 2025

@un33k the command you issued was incorrect. Please try again.

Examples are:

@microsoft-github-policy-service agree

and

@microsoft-github-policy-service agree company="your company"

@microsoft-github-policy-service agree company="Neekware Inc."

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses crashes during program termination caused by static mutex destruction order issues in C++. The fix replaces static mutexes with heap-allocated mutexes that are intentionally leaked to ensure they remain valid throughout the program's lifetime.

  • Converts static mutexes to function-local heap-allocated mutexes using new std::mutex()
  • Updates all mutex references to use getter functions instead of direct static member access
  • Removes static mutex member declarations from header files

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
onnxruntime/core/session/ort_env.h Removes static mutex member declaration and adds explanatory comment
onnxruntime/core/session/ort_env.cc Implements GetOrtEnvMutex() function and updates all mutex usage
onnxruntime/core/session/inference_session.h Removes Windows-specific static mutex declaration and adds explanatory comment
onnxruntime/core/session/inference_session.cc Implements GetActiveSessionsMutex() function and updates all mutex usage
onnxruntime/core/common/logging/logging.cc Updates DefaultLoggerMutex() to use heap-allocated mutex

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

// (and not shutting down).
static OrtEnv* p_instance_;
static std::mutex m_;
// Mutex is now accessed through GetOrtEnvMutex() function
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should explain why the mutex was moved to a function (to fix static destruction order issues) rather than just stating where it's accessed from.

Suggested change
// Mutex is now accessed through GetOrtEnvMutex() function
// The mutex is accessed through GetOrtEnvMutex() function to avoid static destruction order issues.
// In C++, static objects (like a global mutex) may be destroyed before other static objects that still need them,
// leading to undefined behavior. By moving the mutex into a function, it is constructed on first use and destroyed
// after all other static objects, ensuring safe access during shutdown.

Copilot uses AI. Check for mistakes.
static std::map<uint32_t, InferenceSession*> active_sessions_;
#ifdef _WIN32
static std::mutex active_sessions_mutex_; // Protects access to active_sessions_
// Mutex is now accessed through GetActiveSessionsMutex() function to avoid static destruction issues
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This comment is more informative than the one in ort_env.h but could be more consistent. Consider using similar phrasing across both files for consistency.

Suggested change
// Mutex is now accessed through GetActiveSessionsMutex() function to avoid static destruction issues
// Mutex is accessed via GetActiveSessionsMutex()

Copilot uses AI. Check for mistakes.
@jywu-msft
Copy link
Member

/azp run Linux QNN CI Pipeline, Win_TRT_Minimal_CUDA_Test_CI, Windows ARM64 QNN CI Pipeline, Windows GPU Doc Gen CI Pipeline, Windows x64 QNN CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@a-akoval
Copy link

There's no need to leak as we can use the placement new:

static std::mutex& DefaultLoggerMutex() noexcept {
    alignas(std::mutex) static std::byte storage[sizeof(std::mutex)];
    static std::mutex* const m = ::new (static_cast<void*>(storage)) std::mutex();
    return *m;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants