Handle invalid files & symlinks properly

If loader_get_json fails due to the file missing or the file being a
stale symlink, it returns VK_ERROR_INITIALIZATION_FAILED
which loader_parse_icd_manifest passed upwards. Since the caller of
loader_parse_icd_manifest wasn't expecting that error code, it didn't
skip the ICD, causing an infinite recusion in EnumInstExtProps due
to the call to dlsym("vkEnumerateInstanceExtensionProperties");

This also required changes to callers of loader_get_json which would
propagate VK_ERROR_INITIALIZATION failed during layer searching to
no longer cause vkCreateInstance to abort if any invalid files were
found.

Added tests for symlinks as the origin of this bug is due to 'stale'
symlinks after driver installers

diff --git a/loader/loader.c b/loader/loader.c
index 831f416f5f7dc204307211a537a755c4903b9f15..c2ba5c351352bae9afa3c4b442338fbd300a599c 100644 (file)
--- a/loader/loader.c
+++ b/loader/loader.c
@@ -537,6 +537,15 @@ static VkResult loader_add_instance_extensions(const struct loader_instance *ins
         goto out;
     }
 
+    // Make sure we never call ourself by accident, this should never happen outside of error paths
+    if (fp_get_props == vkEnumerateInstanceExtensionProperties) {
+        loader_log(inst, VULKAN_LOADER_ERROR_BIT, 0,
+                   "loader_add_instance_extensions: %s's vkEnumerateInstanceExtensionProperties points to the loader, this would "
+                   "lead to infinite recursion.",
+                   lib_name);
+        goto out;
+    }
+
     res = fp_get_props(NULL, &count, NULL);
     if (res != VK_SUCCESS) {
         loader_log(inst, VULKAN_LOADER_ERROR_BIT, 0,
@@ -1332,6 +1341,15 @@ static VkResult loader_scanned_icd_add(const struct loader_instance *inst, struc
     uint32_t interface_vers;
     VkResult res = VK_SUCCESS;
 
+    // This shouldn't happen, but the check is necessary because dlopen returns a handle to the main program when
+    // filename is NULL
+    if (filename == NULL) {
+        loader_log(inst, VULKAN_LOADER_ERROR_BIT, 0, "loader_scanned_icd_add: A NULL filename was used, skipping this ICD",
+                   filename);
+        res = VK_ERROR_INCOMPATIBLE_DRIVER;
+        goto out;
+    }
+
     // TODO implement smarter opening/closing of libraries. For now this
     // function leaves libraries open and the scanned_icd_clear closes them
 #if defined(__Fuchsia__)
@@ -3235,6 +3253,8 @@ struct ICDManifestInfo {
     uint32_t version;
 };
 
+// Takes a json file, opens, reads, and parses an ICD Manifest out of it.
+// Should only return VK_SUCCESS, VK_ERROR_INCOMPATIBLE_DRIVER, or VK_ERROR_OUT_OF_HOST_MEMORY
 VkResult loader_parse_icd_manifest(const struct loader_instance *inst, char *file_str, struct ICDManifestInfo *icd,
                                    bool *skipped_portability_drivers) {
     VkResult res = VK_SUCCESS;
@@ -3251,7 +3271,11 @@ VkResult loader_parse_icd_manifest(const struct loader_instance *inst, char *fil
     }
 
     res = loader_get_json(inst, file_str, &json);
+    if (res == VK_ERROR_OUT_OF_HOST_MEMORY) {
+        goto out;
+    }
     if (res != VK_SUCCESS || NULL == json) {
+        res = VK_ERROR_INCOMPATIBLE_DRIVER;
         goto out;
     }
 
@@ -3581,14 +3605,15 @@ VkResult loader_scan_for_layers(struct loader_instance *inst, struct loader_laye
             }
 
             // Parse file into JSON struct
-            res = loader_get_json(inst, file_str, &json);
-            if (VK_ERROR_OUT_OF_HOST_MEMORY == res) {
+            VkResult local_res = loader_get_json(inst, file_str, &json);
+            if (VK_ERROR_OUT_OF_HOST_MEMORY == local_res) {
+                res = VK_ERROR_OUT_OF_HOST_MEMORY;
                 goto out;
-            } else if (VK_SUCCESS != res || NULL == json) {
+            } else if (VK_SUCCESS != local_res || NULL == json) {
                 continue;
             }
 
-            VkResult local_res = loader_add_layer_properties(inst, instance_layers, json, true, file_str);
+            local_res = loader_add_layer_properties(inst, instance_layers, json, true, file_str);
             cJSON_Delete(json);
 
             // If the error is anything other than out of memory we still want to try to load the other layers
@@ -3646,14 +3671,15 @@ VkResult loader_scan_for_layers(struct loader_instance *inst, struct loader_laye
             }
 
             // Parse file into JSON struct
-            res = loader_get_json(inst, file_str, &json);
-            if (VK_ERROR_OUT_OF_HOST_MEMORY == res) {
+            VkResult local_res = loader_get_json(inst, file_str, &json);
+            if (VK_ERROR_OUT_OF_HOST_MEMORY == local_res) {
+                res = VK_ERROR_OUT_OF_HOST_MEMORY;
                 goto out;
-            } else if (VK_SUCCESS != res || NULL == json) {
+            } else if (VK_SUCCESS != local_res || NULL == json) {
                 continue;
             }
 
-            VkResult local_res = loader_add_layer_properties(inst, instance_layers, json, false, file_str);
+            local_res = loader_add_layer_properties(inst, instance_layers, json, false, file_str);
             cJSON_Delete(json);
 
             // If the error is anything other than out of memory we still want to try to load the other layers

diff --git a/tests/framework/test_environment.cpp b/tests/framework/test_environment.cpp
index 49d067db47735a53821b46e202fba9b65a0c28e7..5ce4d889fb76491592676169dedd477f144a3c3a 100644 (file)
--- a/tests/framework/test_environment.cpp
+++ b/tests/framework/test_environment.cpp
@@ -346,8 +346,10 @@ TestLayer& TestLayerHandle::reset_layer() noexcept {
 fs::path TestLayerHandle::get_layer_full_path() noexcept { return layer_library.lib_path; }
 fs::path TestLayerHandle::get_layer_manifest_path() noexcept { return manifest_path; }
 
-FrameworkEnvironment::FrameworkEnvironment() noexcept : FrameworkEnvironment(true) {}
-FrameworkEnvironment::FrameworkEnvironment(bool enable_log) noexcept : platform_shim(&folders, enable_log), vulkan_functions() {
+FrameworkEnvironment::FrameworkEnvironment() noexcept : FrameworkEnvironment(true, true) {}
+FrameworkEnvironment::FrameworkEnvironment(bool enable_log) noexcept : FrameworkEnvironment(enable_log, true) {}
+FrameworkEnvironment::FrameworkEnvironment(bool enable_log, bool set_default_search_paths) noexcept
+    : platform_shim(&folders, enable_log), vulkan_functions() {
     // This order is important, it matches the enum ManifestLocation, used to index the folders vector
     folders.emplace_back(FRAMEWORK_BUILD_DIRECTORY, std::string("null_dir"));
     folders.emplace_back(FRAMEWORK_BUILD_DIRECTORY, std::string("icd_manifests"));
@@ -360,9 +362,11 @@ FrameworkEnvironment::FrameworkEnvironment(bool enable_log) noexcept : platform_
     folders.emplace_back(FRAMEWORK_BUILD_DIRECTORY, std::string("app_package_manifests"));
 
     platform_shim->redirect_all_paths(get_folder(ManifestLocation::null).location());
-    platform_shim->set_path(ManifestCategory::icd, get_folder(ManifestLocation::driver).location());
-    platform_shim->set_path(ManifestCategory::explicit_layer, get_folder(ManifestLocation::explicit_layer).location());
-    platform_shim->set_path(ManifestCategory::implicit_layer, get_folder(ManifestLocation::implicit_layer).location());
+    if (set_default_search_paths) {
+        platform_shim->set_path(ManifestCategory::icd, get_folder(ManifestLocation::driver).location());
+        platform_shim->set_path(ManifestCategory::explicit_layer, get_folder(ManifestLocation::explicit_layer).location());
+        platform_shim->set_path(ManifestCategory::implicit_layer, get_folder(ManifestLocation::implicit_layer).location());
+    }
 }
 
 void FrameworkEnvironment::add_icd(TestICDDetails icd_details) noexcept {

diff --git a/tests/framework/test_environment.h b/tests/framework/test_environment.h
index 261b85ff6d164b509a2a741f182acdf953957f5f..7d2e00e744d46456c1f30a5304bc583ce5ba8c3c 100644 (file)
--- a/tests/framework/test_environment.h
+++ b/tests/framework/test_environment.h
@@ -492,8 +492,9 @@ enum class ManifestLocation {
 };
 
 struct FrameworkEnvironment {
-    FrameworkEnvironment() noexcept;  // default is to enable VK_LOADER_DEBUG=all
+    FrameworkEnvironment() noexcept;  // default is to enable VK_LOADER_DEBUG=all and enable the default search paths
     FrameworkEnvironment(bool enable_log) noexcept;
+    FrameworkEnvironment(bool enable_log, bool enable_default_search_paths) noexcept;
 
     void add_icd(TestICDDetails icd_details) noexcept;
     void add_implicit_layer(ManifestLayer layer_manifest, const std::string& json_name) noexcept;

diff --git a/tests/framework/test_util.cpp b/tests/framework/test_util.cpp
index 231e81eb637f977f50bdd9be1bc344a7cb8fa22a..c98ec1ff96f15f2c43796f9bf977e046e2e816b7 100644 (file)
--- a/tests/framework/test_util.cpp
+++ b/tests/framework/test_util.cpp
@@ -471,6 +471,8 @@ path FolderManager::write_manifest(std::string const& name, std::string const& c
     file << contents << std::endl;
     return out_path;
 }
+void FolderManager::add_existing_file(std::string const& file_name) { files.emplace_back(file_name); }
+
 // close file handle, delete file, remove `name` from managed file list.
 void FolderManager::remove(std::string const& name) {
     path out_path = folder / name;

diff --git a/tests/framework/test_util.h b/tests/framework/test_util.h
index ed849cbf245adb902a0a716127c1b3ec8e5b5030..4e20eb62227a7d99a2b155d3714557a4d53feb1f 100644 (file)
--- a/tests/framework/test_util.h
+++ b/tests/framework/test_util.h
@@ -209,8 +209,12 @@ class FolderManager {
     FolderManager(FolderManager&& other) noexcept;
     FolderManager& operator=(FolderManager&& other) noexcept;
 
+    // Add a manifest to the folder
     path write_manifest(std::string const& name, std::string const& contents);
 
+    // Add an already existing file to the manager, so it will be cleaned up automatically
+    void add_existing_file(std::string const& file_name);
+
     // close file handle, delete file, remove `name` from managed file list.
     void remove(std::string const& name);
 

diff --git a/tests/loader_envvar_tests.cpp b/tests/loader_envvar_tests.cpp
index 4fa18220cd857539cb0a655225514d4e223d6c4e..d5682c0b2299c53c407a3a65b10b0dd1444867b2 100644 (file)
--- a/tests/loader_envvar_tests.cpp
+++ b/tests/loader_envvar_tests.cpp
@@ -198,6 +198,21 @@ TEST(EnvVarICDOverrideSetup, XDG) {
     check_paths(env.debug_log, ManifestCategory::implicit_layer, HOME);
     check_paths(env.debug_log, ManifestCategory::explicit_layer, HOME);
 }
+// Check that a json file in the paths don't cause the loader to crash
+TEST(EnvVarICDOverrideSetup, XDGContainsJsonFile) {
+    // Set up a layer path that includes default and user-specified locations,
+    // so that the test app can find them.  Include some badly specified elements as well.
+    // Need to redirect the 'home' directory
+    set_env_var("XDG_CONFIG_DIRS", "bad_file.json");
+
+    FrameworkEnvironment env{};
+    env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2_EXPORT_ICD_GPDPA));
+    env.get_test_icd().physical_devices.push_back({});
+
+    InstWrapper inst{env.vulkan_functions};
+    FillDebugUtilsCreateDetails(inst.create_info, env.debug_log);
+    inst.CheckCreate(VK_SUCCESS);
+}
 #endif
 
 // Test VK_ADD_DRIVER_FILES environment variable

diff --git a/tests/loader_regression_tests.cpp b/tests/loader_regression_tests.cpp
index 78c7cee43406e5d0cea4c3435c6ed0e339107f24..204a1c801e984c215d2984476492f1aa954173d0 100644 (file)
--- a/tests/loader_regression_tests.cpp
+++ b/tests/loader_regression_tests.cpp
@@ -3569,3 +3569,77 @@ TEST(DuplicateRegistryEntries, Drivers) {
         "\" from registry \"HKEY_LOCAL_MACHINE\\SOFTWARE\\Khronos\\Vulkan\\Drivers\" to the list due to duplication"));
 }
 #endif
+
+#if defined(__linux__) || defined(__FreeBSD__) || defined(__OpenBSD__)
+// Check that valid symlinks do not cause the loader to crash when directly in an XDG env-var
+TEST(ManifestDiscovery, ValidSymlinkInXDGEnvVar) {
+    FrameworkEnvironment env{true, false};
+    env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2_EXPORT_ICD_GPDPA).set_discovery_type(ManifestDiscoveryType::none));
+    env.get_test_icd().physical_devices.push_back({});
+    auto driver_path = env.get_icd_manifest_path(0);
+    std::string symlink_name = "symlink_to_driver.json";
+    fs::path symlink_path = env.get_folder(ManifestLocation::driver_env_var).location() / symlink_name;
+    env.get_folder(ManifestLocation::driver_env_var).add_existing_file(symlink_name);
+    int res = symlink(driver_path.c_str(), symlink_path.c_str());
+    ASSERT_EQ(res, 0);
+    set_env_var("XDG_CONFIG_DIRS", symlink_path.str());
+
+    InstWrapper inst{env.vulkan_functions};
+    FillDebugUtilsCreateDetails(inst.create_info, env.debug_log);
+    inst.CheckCreate();
+}
+
+// Check that valid symlinks do not cause the loader to crash
+TEST(ManifestDiscovery, ValidSymlink) {
+    FrameworkEnvironment env{true, false};
+    env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2_EXPORT_ICD_GPDPA).set_discovery_type(ManifestDiscoveryType::none));
+    env.get_test_icd().physical_devices.push_back({});
+
+    auto driver_path = env.get_icd_manifest_path(0);
+    std::string symlink_name = "symlink_to_driver.json";
+    fs::path symlink_path = env.get_folder(ManifestLocation::driver_env_var).location() / symlink_name;
+    env.get_folder(ManifestLocation::driver_env_var).add_existing_file(symlink_name);
+    int res = symlink(driver_path.c_str(), symlink_path.c_str());
+    ASSERT_EQ(res, 0);
+
+    env.platform_shim->set_path(ManifestCategory::icd, env.get_folder(ManifestLocation::driver_env_var).location());
+
+    InstWrapper inst{env.vulkan_functions};
+    FillDebugUtilsCreateDetails(inst.create_info, env.debug_log);
+    inst.CheckCreate();
+}
+
+// Check that invalid symlinks do not cause the loader to crash when directly in an XDG env-var
+TEST(ManifestDiscovery, InvalidSymlinkXDGEnvVar) {
+    FrameworkEnvironment env{true, false};
+    std::string symlink_name = "symlink_to_nothing.json";
+    fs::path symlink_path = env.get_folder(ManifestLocation::driver_env_var).location() / symlink_name;
+    fs::path invalid_driver_path = env.get_folder(ManifestLocation::driver).location() / "nothing_here.json";
+    int res = symlink(invalid_driver_path.c_str(), symlink_path.c_str());
+    ASSERT_EQ(res, 0);
+    env.get_folder(ManifestLocation::driver_env_var).add_existing_file(symlink_name);
+
+    set_env_var("XDG_CONFIG_DIRS", symlink_path.str());
+
+    InstWrapper inst{env.vulkan_functions};
+    FillDebugUtilsCreateDetails(inst.create_info, env.debug_log);
+    inst.CheckCreate(VK_ERROR_INCOMPATIBLE_DRIVER);
+}
+
+// Check that invalid symlinks do not cause the loader to crash
+TEST(ManifestDiscovery, InvalidSymlink) {
+    FrameworkEnvironment env{true, false};
+    std::string symlink_name = "symlink_to_nothing.json";
+    fs::path symlink_path = env.get_folder(ManifestLocation::driver).location() / symlink_name;
+    fs::path invalid_driver_path = env.get_folder(ManifestLocation::driver_env_var).location() / "nothing_here.json";
+    int res = symlink(invalid_driver_path.c_str(), symlink_path.c_str());
+    ASSERT_EQ(res, 0);
+    env.get_folder(ManifestLocation::driver).add_existing_file(symlink_name);
+
+    env.platform_shim->set_path(ManifestCategory::icd, env.get_folder(ManifestLocation::driver_env_var).location());
+
+    InstWrapper inst{env.vulkan_functions};
+    FillDebugUtilsCreateDetails(inst.create_info, env.debug_log);
+    inst.CheckCreate(VK_ERROR_INCOMPATIBLE_DRIVER);
+}
+#endif