Merge pull request #252 from brycehutchings/loader_multithreaded_protections

Add locking to protect against multithreaded conformance test usage.

Author: rpavlik

changes/sdk/pr.252.gh.OpenXR-SDK-Source.md

@@ -0,0 +1 @@
+Loader: Improved locking around a few areas of the loader that aren't robust against usual concurrent calls.

src/loader/loader_core.cpp

@@ -32,16 +32,12 @@
 #include <utility>
 #include <vector>
 
-// Global lock to prevent reading JSON manifest files at the same time.
-std::mutex &GetLoaderJsonMutex() {
-    static std::mutex loader_json_mutex;
-    return loader_json_mutex;
-}
-
-// Global lock to prevent simultaneous instance creation/destruction
-std::mutex &GetInstanceCreateDestroyMutex() {
-    static std::mutex instance_create_destroy_mutex;
-    return instance_create_destroy_mutex;
+// Global loader lock to:
+//   1. Ensure ActiveLoaderInstance get and set operations are done atomically.
+//   2. Ensure RuntimeInterface isn't used to unload the runtime while the runtime is in use.
+std::mutex &GetGlobalLoaderMutex() {
+    static std::mutex loader_mutex;
+    return loader_mutex;
 }
 
 // Prototypes for the debug utils calls used internally.
@@ -90,7 +86,7 @@ static XRAPI_ATTR XrResult XRAPI_CALL LoaderXrEnumerateApiLayerProperties(uint32
     LoaderLogger::LogVerboseMessage("xrEnumerateApiLayerProperties", "Entering loader trampoline");
 
     // Make sure only one thread is attempting to read the JSON files at a time.
-    std::unique_lock<std::mutex> json_lock(GetLoaderJsonMutex());
+    std::unique_lock<std::mutex> loader_lock(GetGlobalLoaderMutex());
 
     XrResult result = ApiLayerInterface::GetApiLayerProperties("xrEnumerateApiLayerProperties", propertyCapacityInput,
                                                                propertyCountOutput, properties);
@@ -123,8 +119,8 @@ LoaderXrEnumerateInstanceExtensionProperties(const char *layerName, uint32_t pro
     XrResult result;
 
     {
-        // Make sure only one thread is attempting to read the JSON files at a time.
-        std::unique_lock<std::mutex> json_lock(GetLoaderJsonMutex());
+        // Make sure the runtime isn't unloaded while this call is in progress.
+        std::unique_lock<std::mutex> loader_lock(GetGlobalLoaderMutex());
 
         // Get the layer extension properties
         result = ApiLayerInterface::GetInstanceExtensionProperties("xrEnumerateInstanceExtensionProperties", layerName,
@@ -238,7 +234,8 @@ static XRAPI_ATTR XrResult XRAPI_CALL LoaderXrCreateInstance(const XrInstanceCre
         return XR_ERROR_VALIDATION_FAILURE;
     }
 
-    std::unique_lock<std::mutex> instance_lock(GetInstanceCreateDestroyMutex());
+    // Make sure the ActiveLoaderInstance::IsAvailable check is done atomically with RuntimeInterface::LoadRuntime.
+    std::unique_lock<std::mutex> instance_lock(GetGlobalLoaderMutex());
 
     // Check if there is already an XrInstance that is alive. If so, another instance cannot be created.
     // The loader does not support multiple simultaneous instances because the loader is intended to be
@@ -255,7 +252,6 @@ static XRAPI_ATTR XrResult XRAPI_CALL LoaderXrCreateInstance(const XrInstanceCre
 
     // Make sure only one thread is attempting to read the JSON files and use the instance.
     {
-        std::unique_lock<std::mutex> json_lock(GetLoaderJsonMutex());
         // Load the available runtime
         result = RuntimeInterface::LoadRuntime("xrCreateInstance");
         if (XR_FAILED(result)) {
@@ -326,7 +322,8 @@ static XRAPI_ATTR XrResult XRAPI_CALL LoaderXrDestroyInstance(XrInstance instanc
         return XR_ERROR_HANDLE_INVALID;
     }
 
-    std::unique_lock<std::mutex> loader_instance_lock(GetInstanceCreateDestroyMutex());
+    // Make sure the runtime isn't unloaded while it is being used by xrEnumerateInstanceExtensionProperties.
+    std::unique_lock<std::mutex> loader_lock(GetGlobalLoaderMutex());
 
     LoaderInstance *loader_instance;
     XrResult result = ActiveLoaderInstance::Get(&loader_instance, "xrDestroyInstance");

src/loader/loader_logger.cpp

@@ -136,14 +136,19 @@ LoaderLogger::LoaderLogger() {
     }
 }
 
-void LoaderLogger::AddLogRecorder(std::unique_ptr<LoaderLogRecorder>&& recorder) { _recorders.push_back(std::move(recorder)); }
+void LoaderLogger::AddLogRecorder(std::unique_ptr<LoaderLogRecorder>&& recorder) {
+    std::unique_lock<std::shared_timed_mutex> lock(_mutex);
+    _recorders.push_back(std::move(recorder));
+}
 
 void LoaderLogger::AddLogRecorderForXrInstance(XrInstance instance, std::unique_ptr<LoaderLogRecorder>&& recorder) {
+    std::unique_lock<std::shared_timed_mutex> lock(_mutex);
     _recordersByInstance[instance].insert(recorder->UniqueId());
     _recorders.emplace_back(std::move(recorder));
 }
 
 void LoaderLogger::RemoveLogRecorder(uint64_t unique_id) {
+    std::unique_lock<std::shared_timed_mutex> lock(_mutex);
     vector_remove_if_and_erase(
         _recorders, [=](std::unique_ptr<LoaderLogRecorder> const& recorder) { return recorder->UniqueId() == unique_id; });
     for (auto& recorders : _recordersByInstance) {
@@ -155,6 +160,7 @@ void LoaderLogger::RemoveLogRecorder(uint64_t unique_id) {
 }
 
 void LoaderLogger::RemoveLogRecordersForXrInstance(XrInstance instance) {
+    std::unique_lock<std::shared_timed_mutex> lock(_mutex);
     if (_recordersByInstance.find(instance) != _recordersByInstance.end()) {
         auto recorders = _recordersByInstance[instance];
         vector_remove_if_and_erase(_recorders, [=](std::unique_ptr<LoaderLogRecorder> const& recorder) {
@@ -179,6 +185,7 @@ bool LoaderLogger::LogMessage(XrLoaderLogMessageSeverityFlagBits message_severit
     callback_data.session_labels = names_and_labels.labels.empty() ? nullptr : names_and_labels.labels.data();
     callback_data.session_labels_count = static_cast<uint8_t>(names_and_labels.labels.size());
 
+    std::shared_lock<std::shared_timed_mutex> lock(_mutex);
     bool exit_app = false;
     for (std::unique_ptr<LoaderLogRecorder>& recorder : _recorders) {
         if ((recorder->MessageSeverities() & message_severity) == message_severity &&
@@ -201,6 +208,7 @@ bool LoaderLogger::LogDebugUtilsMessage(XrDebugUtilsMessageSeverityFlagsEXT mess
     data_.WrapCallbackData(&augmented_data, callback_data);
 
     // Loop through the recorders
+    std::shared_lock<std::shared_timed_mutex> lock(_mutex);
     for (std::unique_ptr<LoaderLogRecorder>& recorder : _recorders) {
         // Only send the message if it's a debug utils recorder and of the type the recorder cares about.
         if (recorder->Type() != XR_LOADER_LOG_DEBUG_UTILS ||

src/loader/loader_logger.hpp

@@ -17,6 +17,7 @@
 #include <vector>
 #include <set>
 #include <map>
+#include <shared_mutex>
 
 #include <openxr/openxr.h>
 
@@ -173,6 +174,8 @@ class LoaderLogger {
    private:
     LoaderLogger();
 
+    std::shared_timed_mutex _mutex;
+
     // List of *all* available recorder objects (including created specifically for an Instance)
     std::vector<std::unique_ptr<LoaderLogRecorder>> _recorders;