Loader settings bug fixes and expanded test coverage

loader/loader.c

@@ -82,6 +82,8 @@ struct activated_layer_info {
     char *library;
     bool is_implicit;
     char *disable_env;
+    char *enable_name_env;
+    char *enable_value_env;
 };
 
 // thread safety lock for accessing global data structures such as "loader"
@@ -4695,6 +4697,8 @@ VkResult loader_create_instance_chain(const VkInstanceCreateInfo *pCreateInfo, c
             activated_layers[num_activated_layers].is_implicit = !(layer_prop->type_flags & VK_LAYER_TYPE_FLAG_EXPLICIT_LAYER);
             if (activated_layers[num_activated_layers].is_implicit) {
                 activated_layers[num_activated_layers].disable_env = layer_prop->disable_env_var.name;
+                activated_layers[num_activated_layers].enable_name_env = layer_prop->enable_env_var.name;
+                activated_layers[num_activated_layers].enable_value_env = layer_prop->enable_env_var.value;
             }
 
             loader_log(inst, VULKAN_LOADER_INFO_BIT | VULKAN_LOADER_LAYER_BIT, 0, "Insert instance layer \"%s\" (%s)",
@@ -4809,6 +4813,11 @@ VkResult loader_create_instance_chain(const VkInstanceCreateInfo *pCreateInfo, c
             if (activated_layers[index].is_implicit) {
                 loader_log(inst, VULKAN_LOADER_LAYER_BIT, 0, "               Disable Env Var:  %s",
                            activated_layers[index].disable_env);
+                if (activated_layers[index].enable_name_env) {
+                    loader_log(inst, VULKAN_LOADER_LAYER_BIT, 0,
+                               "               This layer was enabled because Env Var %s was set to Value %s",
+                               activated_layers[index].enable_name_env, activated_layers[index].enable_value_env);
+                }
             }
             loader_log(inst, VULKAN_LOADER_LAYER_BIT, 0, "           Manifest: %s", activated_layers[index].manifest);
             loader_log(inst, VULKAN_LOADER_LAYER_BIT, 0, "           Library:  %s", activated_layers[index].library);
@@ -6986,8 +6995,6 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_EnumerateInstanceLayerProperties(uint3
 
     LOADER_PLATFORM_THREAD_ONCE(&once_init, loader_initialize);
 
-    uint32_t copy_size;
-
     result = parse_layer_environment_var_filters(NULL, &layer_filters);
     if (VK_SUCCESS != result) {
         goto out;
@@ -7000,36 +7007,35 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_EnumerateInstanceLayerProperties(uint3
         goto out;
     }
 
-    uint32_t active_layer_count = 0;
+    uint32_t layers_to_write_out = 0;
     for (uint32_t i = 0; i < instance_layer_list.count; i++) {
         if (instance_layer_list.list[i].settings_control_value == LOADER_SETTINGS_LAYER_CONTROL_ON ||
             instance_layer_list.list[i].settings_control_value == LOADER_SETTINGS_LAYER_CONTROL_DEFAULT) {
-            active_layer_count++;
+            layers_to_write_out++;
         }
     }
 
     if (pProperties == NULL) {
-        *pPropertyCount = active_layer_count;
+        *pPropertyCount = layers_to_write_out;
         goto out;
     }
 
-    copy_size = (*pPropertyCount < active_layer_count) ? *pPropertyCount : active_layer_count;
     uint32_t output_properties_index = 0;
-    for (uint32_t i = 0; i < copy_size; i++) {
-        if (instance_layer_list.list[i].settings_control_value == LOADER_SETTINGS_LAYER_CONTROL_ON ||
-            instance_layer_list.list[i].settings_control_value == LOADER_SETTINGS_LAYER_CONTROL_DEFAULT) {
+    for (uint32_t i = 0; i < instance_layer_list.count; i++) {
+        if (output_properties_index < *pPropertyCount &&
+            (instance_layer_list.list[i].settings_control_value == LOADER_SETTINGS_LAYER_CONTROL_ON ||
+             instance_layer_list.list[i].settings_control_value == LOADER_SETTINGS_LAYER_CONTROL_DEFAULT)) {
             memcpy(&pProperties[output_properties_index], &instance_layer_list.list[i].info, sizeof(VkLayerProperties));
             output_properties_index++;
         }
     }
-
-    *pPropertyCount = copy_size;
-
-    if (copy_size < instance_layer_list.count) {
+    if (output_properties_index < layers_to_write_out) {
+        // Indicates that we had more elements to write but ran out of room
         result = VK_INCOMPLETE;
-        goto out;
     }
 
+    *pPropertyCount = output_properties_index;
+
 out:
 
     loader_delete_layer_list_and_properties(NULL, &instance_layer_list);

loader/log.c

@@ -143,9 +143,11 @@ void loader_log(const struct loader_instance *inst, VkFlags msg_type, int32_t ms
 
     // Always log to stderr if this is a fatal error
     if (0 == (msg_type & VULKAN_LOADER_FATAL_ERROR_BIT)) {
-        // Exit early if the current instance settings do not ask for logging to stderr
-        if (inst && inst->settings.settings_active && 0 == (msg_type & inst->settings.debug_level)) {
-            return;
+        if (inst && inst->settings.settings_active && inst->settings.debug_level > 0) {
+            // Exit early if the current instance settings have some debugging options but do match the current msg_type
+            if (0 == (msg_type & inst->settings.debug_level)) {
+                return;
+            }
             // Check the global settings and if that doesn't say to skip, check the environment variable
         } else if (0 == (msg_type & g_loader_debug)) {
             return;

loader/settings.c

@@ -579,6 +579,9 @@ VkResult get_settings_layers(const struct loader_instance* inst, struct loader_l
             continue;
         }
 
+        // Makes it possible to know if a new layer was added or not, since the only return value is VkResult
+        size_t count_before_adding = settings_layers->count;
+
         local_res =
             loader_add_layer_properties(inst, settings_layers, json, layer_config->treat_as_implicit_manifest, layer_config->path);
         loader_cJSON_Delete(json);
@@ -587,7 +590,11 @@ VkResult get_settings_layers(const struct loader_instance* inst, struct loader_l
         if (VK_ERROR_OUT_OF_HOST_MEMORY == local_res) {
             res = VK_ERROR_OUT_OF_HOST_MEMORY;
             goto out;
+        } else if (local_res != VK_SUCCESS || count_before_adding == settings_layers->count) {
+            // Indicates something was wrong with the layer, can't add it to the list
+            continue;
         }
+
         struct loader_layer_properties* newly_added_layer = &settings_layers->list[settings_layers->count - 1];
         newly_added_layer->settings_control_value = layer_config->control;
         // If the manifest file found has a name that differs from the one in the settings, remove this layer from consideration
@@ -618,16 +625,24 @@ VkResult get_settings_layers(const struct loader_instance* inst, struct loader_l
 }
 
 // Check if layers has an element with the same name.
+// LAYER_CONTROL_OFF layers are missing some fields, just make sure the layerName is the same
+// If layer_property is a meta layer, just make sure the layerName is the same
+// Skip comparing to UNORDERED_LAYER_LOCATION
 // If layer_property is a regular layer, check if the lib_path is the same.
-// If layer_property is a meta layer, just use the layerName
+// Make sure that the lib_name pointers are non-null before calling strcmp.
 bool check_if_layer_is_in_list(struct loader_layer_list* layer_list, struct loader_layer_properties* layer_property) {
     // If the layer is a meta layer, just check against the name
     for (uint32_t i = 0; i < layer_list->count; i++) {
         if (0 == strncmp(layer_list->list[i].info.layerName, layer_property->info.layerName, VK_MAX_EXTENSION_NAME_SIZE)) {
-            if (0 == (layer_property->type_flags & VK_LAYER_TYPE_FLAG_META_LAYER) &&
-                strcmp(layer_list->list[i].lib_name, layer_property->lib_name) == 0) {
+            if (layer_list->list[i].settings_control_value == LOADER_SETTINGS_LAYER_CONTROL_OFF) {
                 return true;
             }
+            if (VK_LAYER_TYPE_FLAG_META_LAYER == (layer_property->type_flags & VK_LAYER_TYPE_FLAG_META_LAYER)) {
+                return true;
+            }
+            if (layer_list->list[i].lib_name && layer_property->lib_name) {
+                return strcmp(layer_list->list[i].lib_name, layer_property->lib_name) == 0;
+            }
         }
     }
     return false;
@@ -726,6 +741,7 @@ VkResult enable_correct_layers_from_settings(const struct loader_instance* inst,
     if (vk_instance_layers_env != NULL) {
         vk_instance_layers_env_len = strlen(vk_instance_layers_env) + 1;
         vk_instance_layers_env_copy = loader_stack_alloc(vk_instance_layers_env_len);
+        memset(vk_instance_layers_env_copy, 0, vk_instance_layers_env_len);
 
         loader_log(inst, VULKAN_LOADER_WARN_BIT | VULKAN_LOADER_LAYER_BIT, 0, "env var \'%s\' defined and adding layers: %s",
                    ENABLED_LAYERS_ENV, vk_instance_layers_env);
@@ -761,13 +777,14 @@ VkResult enable_correct_layers_from_settings(const struct loader_instance* inst,
             loader_strncpy(vk_instance_layers_env_copy, vk_instance_layers_env_len, vk_instance_layers_env,
                            vk_instance_layers_env_len);
 
-            while (vk_instance_layers_env_copy && *vk_instance_layers_env_copy) {
-                char* next = loader_get_next_path(vk_instance_layers_env_copy);
-                if (0 == strcmp(vk_instance_layers_env_copy, props->info.layerName)) {
+            char* instance_layers_env_iter = vk_instance_layers_env_copy;
+            while (instance_layers_env_iter && *instance_layers_env_iter) {
+                char* next = loader_get_next_path(instance_layers_env_iter);
+                if (0 == strcmp(instance_layers_env_iter, props->info.layerName)) {
                     enable_layer = true;
                     break;
                 }
-                vk_instance_layers_env_copy = next;
+                instance_layers_env_iter = next;
             }
         }
 

tests/framework/shim/shim.h

@@ -134,12 +134,15 @@ struct FrameworkEnvironment;  // forward declaration
 // Necessary to have inline definitions as shim is a dll and thus functions
 // defined in the .cpp wont be found by the rest of the application
 struct PlatformShim {
-    PlatformShim() = default;
-    PlatformShim(std::vector<fs::FolderManager>* folders) : folders(folders) {}
+    PlatformShim() { fputs_stderr_log.reserve(65536); }
+    PlatformShim(std::vector<fs::FolderManager>* folders) : folders(folders) { fputs_stderr_log.reserve(65536); }
 
     // Used to get info about which drivers & layers have been added to folders
     std::vector<fs::FolderManager>* folders;
 
+    // Captures the output to stderr from fputs & fputc - aka the output of loader_log()
+    std::string fputs_stderr_log;
+
     // Test Framework interface
     void reset();
 
@@ -156,6 +159,9 @@ struct PlatformShim {
     void add_manifest(ManifestCategory category, std::filesystem::path const& path);
     void add_unsecured_manifest(ManifestCategory category, std::filesystem::path const& path);
 
+    void clear_logs() { fputs_stderr_log.clear(); }
+    bool find_in_log(std::string const& search_text) const { return fputs_stderr_log.find(search_text) != std::string::npos; }
+
 // platform specific shim interface
 #if defined(WIN32)
     // Control Platform Elevation Level

tests/framework/shim/unix_shim.cpp

@@ -63,6 +63,9 @@ FRAMEWORK_EXPORT PlatformShim* get_platform_shim(std::vector<fs::FolderManager>*
 #if defined(HAVE___SECURE_GETENV)
 #define __SECURE_GETENV_FUNC_NAME __secure_getenv
 #endif
+#define PRINTF_FUNC_NAME printf
+#define FPUTS_FUNC_NAME fputs
+#define FPUTC_FUNC_NAME fputc
 #elif defined(__APPLE__)
 #define OPENDIR_FUNC_NAME my_opendir
 #define READDIR_FUNC_NAME my_readdir
@@ -80,6 +83,8 @@ FRAMEWORK_EXPORT PlatformShim* get_platform_shim(std::vector<fs::FolderManager>*
 #define __SECURE_GETENV_FUNC_NAME my__secure_getenv
 #endif
 #endif
+#define FPUTS_FUNC_NAME my_fputs
+#define FPUTC_FUNC_NAME my_fputc
 #endif
 
 using PFN_OPENDIR = DIR* (*)(const char* path_name);
@@ -93,6 +98,8 @@ using PFN_GETEGID = gid_t (*)(void);
 #if defined(HAVE_SECURE_GETENV) || defined(HAVE___SECURE_GETENV)
 using PFN_SEC_GETENV = char* (*)(const char* name);
 #endif
+using PFN_FPUTS = int (*)(const char* str, FILE* stream);
+using PFN_FPUTC = int (*)(int c, FILE* stream);
 
 #if defined(__APPLE__)
 #define real_opendir opendir
@@ -109,6 +116,8 @@ using PFN_SEC_GETENV = char* (*)(const char* name);
 #if defined(HAVE___SECURE_GETENV)
 #define real__secure_getenv __secure_getenv
 #endif
+#define real_fputs fputs
+#define real_fputc fputc
 #else
 PFN_OPENDIR real_opendir = nullptr;
 PFN_READDIR real_readdir = nullptr;
@@ -124,6 +133,8 @@ PFN_SEC_GETENV real_secure_getenv = nullptr;
 #if defined(HAVE___SECURE_GETENV)
 PFN_SEC_GETENV real__secure_getenv = nullptr;
 #endif
+PFN_FPUTS real_fputs = nullptr;
+PFN_FPUTC real_fputc = nullptr;
 #endif
 
 FRAMEWORK_EXPORT DIR* OPENDIR_FUNC_NAME(const char* path_name) {
@@ -327,6 +338,27 @@ FRAMEWORK_EXPORT char* __SECURE_GETENV_FUNC_NAME(const char* name) {
 }
 #endif
 #endif
+
+FRAMEWORK_EXPORT int FPUTS_FUNC_NAME(const char* str, FILE* stream) {
+#if !defined(__APPLE__)
+    if (!real_fputs) real_fputs = (PFN_FPUTS)dlsym(RTLD_NEXT, "fputs");
+#endif
+    if (stream == stderr) {
+        platform_shim.fputs_stderr_log += str;
+    }
+    return real_fputs(str, stream);
+}
+
+FRAMEWORK_EXPORT int FPUTC_FUNC_NAME(int ch, FILE* stream) {
+#if !defined(__APPLE__)
+    if (!real_fputc) real_fputc = (PFN_FPUTC)dlsym(RTLD_NEXT, "fputc");
+#endif
+    if (stream == stderr) {
+        platform_shim.fputs_stderr_log += ch;
+    }
+    return real_fputc(ch, stream);
+}
+
 #if defined(__APPLE__)
 FRAMEWORK_EXPORT CFBundleRef my_CFBundleGetMainBundle() {
     static CFBundleRef global_bundle{};
@@ -365,8 +397,7 @@ struct Interposer {
 };
 
 __attribute__((used)) static Interposer _interpose_opendir MACOS_ATTRIB = {VOIDP_CAST(my_opendir), VOIDP_CAST(opendir)};
-// don't intercept readdir as it crashes when using ASAN with macOS
-// __attribute__((used)) static Interposer _interpose_readdir MACOS_ATTRIB = {VOIDP_CAST(my_readdir), VOIDP_CAST(readdir)};
+__attribute__((used)) static Interposer _interpose_readdir MACOS_ATTRIB = {VOIDP_CAST(my_readdir), VOIDP_CAST(readdir)};
 __attribute__((used)) static Interposer _interpose_closedir MACOS_ATTRIB = {VOIDP_CAST(my_closedir), VOIDP_CAST(closedir)};
 __attribute__((used)) static Interposer _interpose_access MACOS_ATTRIB = {VOIDP_CAST(my_access), VOIDP_CAST(access)};
 __attribute__((used)) static Interposer _interpose_fopen MACOS_ATTRIB = {VOIDP_CAST(my_fopen), VOIDP_CAST(fopen)};
@@ -383,6 +414,8 @@ __attribute__((used)) static Interposer _interpose__secure_getenv MACOS_ATTRIB =
                                                                                   VOIDP_CAST(__secure_getenv)};
 #endif
 #endif
+__attribute__((used)) static Interposer _interpose_fputs MACOS_ATTRIB = {VOIDP_CAST(my_fputs), VOIDP_CAST(fputs)};
+__attribute__((used)) static Interposer _interpose_fputc MACOS_ATTRIB = {VOIDP_CAST(my_fputc), VOIDP_CAST(fputc)};
 __attribute__((used)) static Interposer _interpose_CFBundleGetMainBundle MACOS_ATTRIB = {VOIDP_CAST(my_CFBundleGetMainBundle),
                                                                                          VOIDP_CAST(CFBundleGetMainBundle)};
 __attribute__((used)) static Interposer _interpose_CFBundleCopyResourcesDirectoryURL MACOS_ATTRIB = {

tests/framework/shim/windows_shim.cpp

@@ -31,6 +31,9 @@
 #include <ntstatus.h>
 #endif
 
+#include <windows.h>
+#include <debugapi.h>
+
 #include "shim.h"
 
 #include "detours.h"
@@ -405,6 +408,15 @@ LONG WINAPI ShimGetPackagePathByFullName(_In_ PCWSTR packageFullName, _Inout_ UI
     return 0;
 }
 
+using PFN_OutputDebugStringA = void(__stdcall *)(LPCSTR lpOutputString);
+static PFN_OutputDebugStringA fp_OutputDebugStringA = OutputDebugStringA;
+
+void __stdcall intercept_OutputDebugStringA(LPCSTR lpOutputString) {
+    if (lpOutputString != nullptr) {
+        platform_shim.fputs_stderr_log += lpOutputString;
+    }
+}
+
 // Initialization
 void WINAPI DetourFunctions() {
     if (!gdi32_dll) {
@@ -454,6 +466,7 @@ void WINAPI DetourFunctions() {
     DetourAttach(&(PVOID &)fpRegCloseKey, (PVOID)ShimRegCloseKey);
     DetourAttach(&(PVOID &)fpGetPackagesByPackageFamily, (PVOID)ShimGetPackagesByPackageFamily);
     DetourAttach(&(PVOID &)fpGetPackagePathByFullName, (PVOID)ShimGetPackagePathByFullName);
+    DetourAttach(&(PVOID &)fp_OutputDebugStringA, (PVOID)intercept_OutputDebugStringA);
     LONG error = DetourTransactionCommit();
 
     if (error != NO_ERROR) {
@@ -483,6 +496,7 @@ void DetachFunctions() {
     DetourDetach(&(PVOID &)fpRegCloseKey, (PVOID)ShimRegCloseKey);
     DetourDetach(&(PVOID &)fpGetPackagesByPackageFamily, (PVOID)ShimGetPackagesByPackageFamily);
     DetourDetach(&(PVOID &)fpGetPackagePathByFullName, (PVOID)ShimGetPackagePathByFullName);
+    DetourDetach(&(PVOID &)fp_OutputDebugStringA, (PVOID)intercept_OutputDebugStringA);
     DetourTransactionCommit();
 }
 

tests/framework/test_environment.cpp

@@ -198,11 +198,13 @@ InstWrapper& InstWrapper::operator=(InstWrapper&& other) noexcept {
 }
 
 void InstWrapper::CheckCreate(VkResult result_to_check) {
+    handle_assert_null(inst);
     ASSERT_EQ(result_to_check, functions->vkCreateInstance(create_info.get(), callbacks, &inst));
     functions->load_instance_functions(inst);
 }
 
 void InstWrapper::CheckCreateWithInfo(InstanceCreateInfo& create_info, VkResult result_to_check) {
+    handle_assert_null(inst);
     ASSERT_EQ(result_to_check, functions->vkCreateInstance(create_info.get(), callbacks, &inst));
     functions->load_instance_functions(inst);
 }
@@ -297,10 +299,12 @@ DeviceWrapper& DeviceWrapper::operator=(DeviceWrapper&& other) noexcept {
 }
 
 void DeviceWrapper::CheckCreate(VkPhysicalDevice phys_dev, VkResult result_to_check) {
+    handle_assert_null(dev);
     ASSERT_EQ(result_to_check, functions->vkCreateDevice(phys_dev, create_info.get(), callbacks, &dev));
 }
 
 VkResult CreateDebugUtilsMessenger(DebugUtilsWrapper& debug_utils) {
+    handle_assert_null(debug_utils.messenger);
     return debug_utils.local_vkCreateDebugUtilsMessengerEXT(debug_utils.inst, debug_utils.get(), debug_utils.callbacks,
                                                             &debug_utils.messenger);
 }

tests/framework/test_util.h

@@ -268,7 +268,13 @@ inline loader_platform_dl_handle loader_platform_open_library(const char* libPat
 inline void loader_platform_open_library_print_error(std::filesystem::path const& libPath) {
     std::wcerr << "Unable to open library: " << libPath << " due to: " << dlerror() << "\n";
 }
-inline void loader_platform_close_library(loader_platform_dl_handle library) { dlclose(library); }
+inline void loader_platform_close_library(loader_platform_dl_handle library) {
+    char* loader_disable_dynamic_library_unloading_env_var = getenv("VK_LOADER_DISABLE_DYNAMIC_LIBRARY_UNLOADING");
+    if (NULL == loader_disable_dynamic_library_unloading_env_var ||
+        0 != strncmp(loader_disable_dynamic_library_unloading_env_var, "1", 2)) {
+    }
+    dlclose(library);
+}
 inline void* loader_platform_get_proc_address(loader_platform_dl_handle library, const char* name) {
     assert(library);
     assert(name);