Handle invalid files & symlinks properlyHEADmaster

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

7 files changed, 140 insertions, 14 deletions

diff --git a/loader/loader.c b/loader/loader.c
index 831f416f5..c2ba5c351 100644
--- 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 49d067db4..5ce4d889f 100644
--- 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 261b85ff6..7d2e00e74 100644
--- 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 231e81eb6..c98ec1ff9 100644
--- 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 ed849cbf2..4e20eb622 100644
--- 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 4fa18220c..d5682c0b2 100644
--- 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 78c7cee43..204a1c801 100644
--- 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