Add device dispatch table magic value check

The device dispatch table has a magic value to ensure consistency, but
there was no message logged in case the magic value was corrupted. This
commit adds that check, fixes the message being printed for the instance
magic value check, and adds tests to ensure they are contining to be
checked in the future.

diff --git a/docs/LoaderLayerInterface.md b/docs/LoaderLayerInterface.md
index a4406955cbd5c5246d456338ca1458fcc27a53a8..2ee6144197860f8580064de7389689d68804ee14 100644 (file)
--- a/docs/LoaderLayerInterface.md
+++ b/docs/LoaderLayerInterface.md
@@ -2500,6 +2500,22 @@ Android Vulkan documentation</a>.
     <td>Yes</td>
     <td><small>N/A</small></td>
   </tr>
+  <tr>
+  <td><small><b>LLP_LAYER_22</b></small></td>
+    <td>During <i>vkCreateDevice</i>, a layer <b>must not</b> modify the
+        <i>pDevice</i> pointer during prior to calling down to the lower
+        layers.<br/>
+        This is because the loader passes information in this pointer that is
+        necessary for the initialization code in the loader's terminator
+        function.<br/>
+        Instead, if the layer is overriding the <i>pDevice</i> pointer, it
+        <b>must</b> do so only after the call to the lower layers returns.
+    </td>
+    <td>The loader will likely crash.</td>
+    <td>No</td>
+    <td>Yes</td>
+    <td><small>N/A</small></td>
+  </tr>
 </table>
 
 

diff --git a/loader/generated/vk_loader_extensions.c b/loader/generated/vk_loader_extensions.c
index 97df84de097fb9cdb75af159efc8cf98c7421b69..411f9729c6b02134d29511a20fe2620018467c7f 100644 (file)
--- a/loader/generated/vk_loader_extensions.c
+++ b/loader/generated/vk_loader_extensions.c
@@ -316,7 +316,7 @@ VKAPI_ATTR bool VKAPI_CALL loader_icd_init_entries(struct loader_icd_term *icd_t
 VKAPI_ATTR void VKAPI_CALL loader_init_device_dispatch_table(struct loader_dev_dispatch_table *dev_table, PFN_vkGetDeviceProcAddr gpa,
                                                              VkDevice dev) {
     VkLayerDispatchTable *table = &dev_table->core_dispatch;
-    assert(table->magic == DEVICE_DISP_TABLE_MAGIC_NUMBER);
+    if (table->magic != DEVICE_DISP_TABLE_MAGIC_NUMBER) { abort(); }
     for (uint32_t i = 0; i < MAX_NUM_UNKNOWN_EXTS; i++) dev_table->ext_dispatch[i] = (PFN_vkDevExt)vkDevExtError;
 
     // ---- Core 1_0 commands

diff --git a/loader/loader.c b/loader/loader.c
index 52ff44c136e02a18e459ca271aff7bb6963a317b..389cbfce816be2d3ca41d61a1d6561dfbea5e56c 100644 (file)
--- a/loader/loader.c
+++ b/loader/loader.c
@@ -5210,7 +5210,7 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateInstance(const VkInstanceCreateI
         loader_log(ptr_instance, VULKAN_LOADER_WARN_BIT, 0,
                    "terminator_CreateInstance: Instance pointer (%p) has invalid MAGIC value 0x%08x. Instance value possibly "
                    "corrupted by active layer (Policy #LLP_LAYER_21).  ",
-                   ptr_instance->magic);
+                   ptr_instance, ptr_instance->magic);
     }
 
     // Save the application version if it has been modified - layers sometimes needs features in newer API versions than
@@ -5593,6 +5593,17 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateDevice(VkPhysicalDevice physical
     VkBaseOutStructure *caller_dgci_container = NULL;
     VkDeviceGroupDeviceCreateInfoKHR *caller_dgci = NULL;
 
+    if (NULL == dev) {
+        loader_log(icd_term->this_instance, VULKAN_LOADER_WARN_BIT, 0,
+                   "terminator_CreateDevice: Loader device pointer null encountered.  Possibly set by active layer. (Policy "
+                   "#LLP_LAYER_22)");
+    } else if (LOADER_MAGIC_NUMBER != dev->loader_dispatch.core_dispatch.magic) {
+        loader_log(icd_term->this_instance, VULKAN_LOADER_WARN_BIT, 0,
+                   "terminator_CreateDevice: Device pointer (%p) has invalid MAGIC value 0x%08x. Device value possibly "
+                   "corrupted by active layer (Policy #LLP_LAYER_22).  ",
+                   dev, dev->loader_dispatch.core_dispatch.magic);
+    }
+
     dev->phys_dev_term = phys_dev_term;
 
     icd_exts.list = NULL;

diff --git a/loader/trampoline.c b/loader/trampoline.c
index fe4acc95409de4825e7fd436d871523ea55b50e1..65809efb5a6efb7b657e01d871b5687d549f6c10 100644 (file)
--- a/loader/trampoline.c
+++ b/loader/trampoline.c
@@ -97,10 +97,8 @@ LOADER_EXPORT VKAPI_ATTR PFN_vkVoidFunction VKAPI_CALL vkGetInstanceProcAddr(VkI
         struct loader_instance *ptr_instance = loader_get_instance(instance);
         // If we've gotten here and the pointer is NULL, it's invalid
         if (ptr_instance == NULL) {
-            loader_log(NULL,
-                       VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT |
-                           VULKAN_LOADER_VALIDATION_BIT,
-                       0, "vkGetInstanceProcAddr: Invalid instance [VUID-vkGetInstanceProcAddr-instance-parameter]");
+            loader_log(NULL, VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, 0,
+                       "vkGetInstanceProcAddr: Invalid instance [VUID-vkGetInstanceProcAddr-instance-parameter]");
             abort(); /* Intentionally fail so user can correct issue. */
         }
         // Return trampoline code for non-global entrypoints including any extensions.
@@ -770,10 +768,8 @@ LOADER_EXPORT VKAPI_ATTR void VKAPI_CALL vkDestroyInstance(VkInstance instance,
 
     ptr_instance = loader_get_instance(instance);
     if (ptr_instance == NULL) {
-        loader_log(
-            NULL,
-            VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT,
-            0, "vkDestroyInstance: Invalid instance [VUID-vkDestroyInstance-instance-parameter]");
+        loader_log(NULL, VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, 0,
+                   "vkDestroyInstance: Invalid instance [VUID-vkDestroyInstance-instance-parameter]");
         loader_platform_thread_unlock_mutex(&loader_lock);
         abort(); /* Intentionally fail so user can correct issue. */
     }
@@ -829,20 +825,15 @@ LOADER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkEnumeratePhysicalDevices(VkInstan
 
     inst = loader_get_instance(instance);
     if (NULL == inst) {
-        loader_log(
-            NULL,
-            VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT,
-            0, "vkEnumeratePhysicalDevices: Invalid instance [VUID-vkEnumeratePhysicalDevices-instance-parameter]");
+        loader_log(NULL, VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, 0,
+                   "vkEnumeratePhysicalDevices: Invalid instance [VUID-vkEnumeratePhysicalDevices-instance-parameter]");
         abort(); /* Intentionally fail so user can correct issue. */
     }
 
     if (NULL == pPhysicalDeviceCount) {
-        loader_log(
-            inst,
-            VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT,
-            0,
-            "vkEnumeratePhysicalDevices: Received NULL pointer for physical device count return value. "
-            "[VUID-vkEnumeratePhysicalDevices-pPhysicalDeviceCount-parameter]");
+        loader_log(inst, VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, 0,
+                   "vkEnumeratePhysicalDevices: Received NULL pointer for physical device count return value. "
+                   "[VUID-vkEnumeratePhysicalDevices-pPhysicalDeviceCount-parameter]");
         res = VK_ERROR_INITIALIZATION_FAILED;
         goto out;
     }
@@ -870,9 +861,8 @@ LOADER_EXPORT VKAPI_ATTR void VKAPI_CALL vkGetPhysicalDeviceFeatures(VkPhysicalD
     VkPhysicalDevice unwrapped_phys_dev = loader_unwrap_physical_device(physicalDevice);
     if (VK_NULL_HANDLE == unwrapped_phys_dev) {
         loader_log(
-            NULL,
-            VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT,
-            0, "vkGetPhysicalDeviceFeatures: Invalid physicalDevice [VUID-vkGetPhysicalDeviceFeatures-physicalDevice-parameter]");
+            NULL, VULKAN_LOADER_FATAL_ERROR_BIT | VULKAN_LOADER_ERROR_BIT | VULKAN_LOADER_VALIDATION_BIT, 0,
+            "vkGetPhysicalDeviceFeatures: Invalid physicalDevice [VUID-vkGetPhysicalDeviceFeatures-physicalDevice-parameter]");
         abort(); /* Intentionally fail so user can correct issue. */
     }
     disp = loader_get_instance_layer_dispatch(physicalDevice);

diff --git a/scripts/loader_extension_generator.py b/scripts/loader_extension_generator.py
index c1c70e794004bf45de6232d68cee53e5ee8d94a8..c52ca8862570b555fe1db11959afa8f6312fbdca 100644 (file)
--- a/scripts/loader_extension_generator.py
+++ b/scripts/loader_extension_generator.py
@@ -780,7 +780,7 @@ class LoaderExtensionOutputGenerator(OutputGenerator):
                 tables += 'VKAPI_ATTR void VKAPI_CALL loader_init_device_dispatch_table(struct loader_dev_dispatch_table *dev_table, PFN_vkGetDeviceProcAddr gpa,\n'
                 tables += '                                                             VkDevice dev) {\n'
                 tables += '    VkLayerDispatchTable *table = &dev_table->core_dispatch;\n'
-                tables += '    assert(table->magic == DEVICE_DISP_TABLE_MAGIC_NUMBER);\n'
+                tables += '    if (table->magic != DEVICE_DISP_TABLE_MAGIC_NUMBER) { abort(); }\n'
                 tables += '    for (uint32_t i = 0; i < MAX_NUM_UNKNOWN_EXTS; i++) dev_table->ext_dispatch[i] = (PFN_vkDevExt)vkDevExtError;\n'
 
             elif x == 1:

diff --git a/tests/framework/layer/test_layer.cpp b/tests/framework/layer/test_layer.cpp
index 2764f665c15c3d3e2513de0c58bb9ded81fd3cd0..14f0ebd6032ad5b943f872517154e0779074a37f 100644 (file)
--- a/tests/framework/layer/test_layer.cpp
+++ b/tests/framework/layer/test_layer.cpp
@@ -277,6 +277,10 @@ VKAPI_ATTR VkResult VKAPI_CALL test_vkCreateInstance(const VkInstanceCreateInfo*
     }
     const VkInstanceCreateInfo* create_info_pointer = use_modified_create_info ? &instance_create_info : pCreateInfo;
 
+    if (layer.clobber_pInstance) {
+        memset(*pInstance, 0, 128);
+    }
+
     // Continue call down the chain
     VkResult result = fpCreateInstance(create_info_pointer, pAllocator, pInstance);
     if (result != VK_SUCCESS) {
@@ -401,6 +405,10 @@ VKAPI_ATTR VkResult VKAPI_CALL test_vkCreateDevice(VkPhysicalDevice physicalDevi
     // Advance the link info for the next element on the chain
     chain_info->u.pLayerInfo = chain_info->u.pLayerInfo->pNext;
 
+    if (layer.clobber_pDevice) {
+        memset(*pDevice, 0, 128);
+    }
+
     VkResult result = fpCreateDevice(physicalDevice, pCreateInfo, pAllocator, pDevice);
     if (result != VK_SUCCESS) {
         return result;

diff --git a/tests/framework/layer/test_layer.h b/tests/framework/layer/test_layer.h
index 856f713ffdf522a3ff8bec0edef06b824b226360..d6d086c14d2fa688e7bd6402e5579905bbac72dc 100644 (file)
--- a/tests/framework/layer/test_layer.h
+++ b/tests/framework/layer/test_layer.h
@@ -190,6 +190,11 @@ struct TestLayer {
 
     BUILDER_VALUE(TestLayer, bool, check_if_EnumDevExtProps_is_same_as_queried_function, false)
 
+    // Clober the data pointed to by pInstance to overwrite the magic value
+    BUILDER_VALUE(TestLayer, bool, clobber_pInstance, false)
+    // Clober the data pointed to by pDevice to overwrite the magic value
+    BUILDER_VALUE(TestLayer, bool, clobber_pDevice, false)
+
     PFN_vkGetInstanceProcAddr next_vkGetInstanceProcAddr = VK_NULL_HANDLE;
     PFN_GetPhysicalDeviceProcAddr next_GetPhysicalDeviceProcAddr = VK_NULL_HANDLE;
     PFN_vkGetDeviceProcAddr next_vkGetDeviceProcAddr = VK_NULL_HANDLE;

diff --git a/tests/loader_regression_tests.cpp b/tests/loader_regression_tests.cpp
index 23e20135fec0d952de348a1a99e3fc498d1575d0..bcc7b0064066a997e9f09e522393146f981cb6a0 100644 (file)
--- a/tests/loader_regression_tests.cpp
+++ b/tests/loader_regression_tests.cpp
@@ -4069,3 +4069,80 @@ TEST(Layer, pfnNextGetInstanceProcAddr_should_not_return_layers_own_functions) {
     DeviceWrapper dev{inst};
     dev.CheckCreate(phys_devs.at(0));
 }
+
+TEST(Layer, LLP_LAYER_21) {
+    FrameworkEnvironment env{};
+    env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2)).add_physical_device({});
+
+    env.add_implicit_layer(ManifestLayer{}.add_layer(ManifestLayer::LayerDescription{}
+                                                         .set_name("implicit_layer_name")
+                                                         .set_lib_path(TEST_LAYER_PATH_EXPORT_VERSION_2)
+                                                         .set_disable_environment("DISABLE_ME")),
+                           "implicit_test_layer.json");
+    env.get_test_layer(0).set_clobber_pInstance(true);
+
+    InstWrapper inst{env.vulkan_functions};
+    FillDebugUtilsCreateDetails(inst.create_info, env.debug_log);
+#if defined(WIN32)
+#if defined(_WIN64)
+    ASSERT_DEATH(
+        inst.CheckCreate(),
+        testing::ContainsRegex(
+            R"(terminator_CreateInstance: Instance pointer \(................\) has invalid MAGIC value 0x00000000. Instance value )"
+            R"(possibly corrupted by active layer \(Policy #LLP_LAYER_21\))"));
+#else
+    ASSERT_DEATH(
+        inst.CheckCreate(),
+        testing::ContainsRegex(
+            R"(terminator_CreateInstance: Instance pointer \(........\) has invalid MAGIC value 0x00000000. Instance value )"
+            R"(possibly corrupted by active layer \(Policy #LLP_LAYER_21\))"));
+#endif
+#else
+    ASSERT_DEATH(
+        inst.CheckCreate(),
+        testing::ContainsRegex(
+            R"(terminator_CreateInstance: Instance pointer \(0x[0-9A-Fa-f]+\) has invalid MAGIC value 0x00000000. Instance value )"
+            R"(possibly corrupted by active layer \(Policy #LLP_LAYER_21\))"));
+#endif
+}
+
+TEST(Layer, LLP_LAYER_22) {
+    FrameworkEnvironment env{};
+    env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2)).add_physical_device({});
+
+    env.add_implicit_layer(ManifestLayer{}.add_layer(ManifestLayer::LayerDescription{}
+                                                         .set_name("implicit_layer_name")
+                                                         .set_lib_path(TEST_LAYER_PATH_EXPORT_VERSION_2)
+                                                         .set_disable_environment("DISABLE_ME")),
+                           "implicit_test_layer.json");
+    env.get_test_layer(0).set_clobber_pDevice(true);
+
+    InstWrapper inst{env.vulkan_functions};
+    inst.create_info.add_extension("VK_EXT_debug_utils");
+    inst.CheckCreate();
+
+    DebugUtilsWrapper log{inst};
+    CreateDebugUtilsMessenger(log);
+
+    DeviceWrapper dev{inst};
+#if defined(WIN32)
+#if defined(_WIN64)
+    ASSERT_DEATH(
+        dev.CheckCreate(inst.GetPhysDev()),
+        testing::ContainsRegex(
+            R"(terminator_CreateDevice: Device pointer \(................\) has invalid MAGIC value 0x00000000. Device value )"
+            R"(possibly corrupted by active layer \(Policy #LLP_LAYER_22\))"));
+#else
+    ASSERT_DEATH(dev.CheckCreate(inst.GetPhysDev()),
+                 testing::ContainsRegex(
+                     R"(terminator_CreateDevice: Device pointer \(........\) has invalid MAGIC value 0x00000000. Device value )"
+                     R"(possibly corrupted by active layer \(Policy #LLP_LAYER_22\))"));
+#endif
+#else
+    ASSERT_DEATH(
+        dev.CheckCreate(inst.GetPhysDev()),
+        testing::ContainsRegex(
+            R"(terminator_CreateDevice: Device pointer \(0x[0-9A-Fa-f]+\) has invalid MAGIC value 0x00000000. Device value )"
+            R"(possibly corrupted by active layer \(Policy #LLP_LAYER_22\))"));
+#endif
+}