From facee8b3b0b15fc345cacab9a3a028636b131995 Mon Sep 17 00:00:00 2001 From: Ian Elliott Date: Thu, 5 May 2016 14:06:53 -0600 Subject: layers: Fix "count" tests in swapchain layer. The swapchain layer was attempting to test if the application gave a value for a *Count variable (e.g. pSurfaceFormatCount for the vkGetPhysicalDeviceSurfaceFormatsKHR() function) that was larger than what that function returned previously (i.e. when the non-count variable was NULL). However, the test was made after calling down the call-chain, which may modify the value given by the application. This test is now made before calling down the chain. In addition, a new test is made that tries to ensure that the application did call the function with the non-Count variable equal to NULL **before** calling the function with a non-NULL non-Count variable. --- layers/swapchain.cpp | 127 ++++++++++++++++++++++------------ layers/swapchain.h | 7 ++ layers/vk_validation_layer_details.md | 1 + 3 files changed, 89 insertions(+), 46 deletions(-) diff --git a/layers/swapchain.cpp b/layers/swapchain.cpp index 8efa8883..998e3ebf 100644 --- a/layers/swapchain.cpp +++ b/layers/swapchain.cpp @@ -1138,6 +1138,23 @@ vkGetPhysicalDeviceSurfaceFormatsKHR(VkPhysicalDevice physicalDevice, VkSurfaceK } if (!pSurfaceFormatCount) { skipCall |= LOG_ERROR_NULL_POINTER(VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, physicalDevice, "pSurfaceFormatCount"); + } else if (pPhysicalDevice && pSurfaceFormats) { + // Compare the preliminary value of *pSurfaceFormatCount with the + // value this time: + if (pPhysicalDevice->surfaceFormatCount == 0) { + // Since we haven't recorded a preliminary value of + // *pSurfaceFormatCount, that likely means that the application + // didn't previously call this function with a NULL value of + // pSurfaceFormats: + skipCall |= LOG_ERROR_ZERO_PRIOR_COUNT( + VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, + physicalDevice, "pSurfaceFormatCount", "pSurfaceFormats"); + } else if (*pSurfaceFormatCount > pPhysicalDevice->surfaceFormatCount) { + skipCall |= LOG_ERROR_INVALID_COUNT( + VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, + physicalDevice, "pSurfaceFormatCount", "pSurfaceFormats", + *pSurfaceFormatCount, pPhysicalDevice->surfaceFormatCount); + } } if (!skipCall) { @@ -1152,23 +1169,18 @@ vkGetPhysicalDeviceSurfaceFormatsKHR(VkPhysicalDevice physicalDevice, VkSurfaceK if ((result == VK_SUCCESS) && pPhysicalDevice && !pSurfaceFormats && pSurfaceFormatCount) { // Record the result of this preliminary query: pPhysicalDevice->surfaceFormatCount = *pSurfaceFormatCount; - } else if ((result == VK_SUCCESS) && pPhysicalDevice && pSurfaceFormats && pSurfaceFormatCount) { - // Compare the preliminary value of *pSurfaceFormatCount with the - // value this time: - if (*pSurfaceFormatCount > pPhysicalDevice->surfaceFormatCount) { - LOG_ERROR_INVALID_COUNT(VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, physicalDevice, "pSurfaceFormatCount", - "pSurfaceFormats", *pSurfaceFormatCount, pPhysicalDevice->surfaceFormatCount); - } else if (*pSurfaceFormatCount > 0) { - // Record the result of this query: - pPhysicalDevice->surfaceFormatCount = *pSurfaceFormatCount; - pPhysicalDevice->pSurfaceFormats = (VkSurfaceFormatKHR *)malloc(*pSurfaceFormatCount * sizeof(VkSurfaceFormatKHR)); - if (pPhysicalDevice->pSurfaceFormats) { - for (uint32_t i = 0; i < *pSurfaceFormatCount; i++) { - pPhysicalDevice->pSurfaceFormats[i] = pSurfaceFormats[i]; - } - } else { - pPhysicalDevice->surfaceFormatCount = 0; + } else if ((result == VK_SUCCESS) && pPhysicalDevice && + pSurfaceFormats && pSurfaceFormatCount && + (*pSurfaceFormatCount > 0)) { + // Record the result of this query: + pPhysicalDevice->surfaceFormatCount = *pSurfaceFormatCount; + pPhysicalDevice->pSurfaceFormats = (VkSurfaceFormatKHR *)malloc(*pSurfaceFormatCount * sizeof(VkSurfaceFormatKHR)); + if (pPhysicalDevice->pSurfaceFormats) { + for (uint32_t i = 0; i < *pSurfaceFormatCount; i++) { + pPhysicalDevice->pSurfaceFormats[i] = pSurfaceFormats[i]; } + } else { + pPhysicalDevice->surfaceFormatCount = 0; } } return result; @@ -1194,6 +1206,23 @@ vkGetPhysicalDeviceSurfacePresentModesKHR(VkPhysicalDevice physicalDevice, VkSur } if (!pPresentModeCount) { skipCall |= LOG_ERROR_NULL_POINTER(VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, physicalDevice, "pPresentModeCount"); + } else if (pPhysicalDevice && pPresentModes) { + // Compare the preliminary value of *pPresentModeCount with the + // value this time: + if (pPhysicalDevice->presentModeCount == 0) { + // Since we haven't recorded a preliminary value of + // *pPresentModeCount, that likely means that the application + // didn't previously call this function with a NULL value of + // pPresentModes: + skipCall |= LOG_ERROR_ZERO_PRIOR_COUNT( + VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, + physicalDevice, "pPresentModeCount", "pPresentModes"); + } else if (*pPresentModeCount > pPhysicalDevice->presentModeCount) { + skipCall |= LOG_ERROR_INVALID_COUNT( + VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, + physicalDevice, "pPresentModeCount", "pPresentModes", + *pPresentModeCount, pPhysicalDevice->presentModeCount); + } } if (!skipCall) { @@ -1208,23 +1237,18 @@ vkGetPhysicalDeviceSurfacePresentModesKHR(VkPhysicalDevice physicalDevice, VkSur if ((result == VK_SUCCESS) && pPhysicalDevice && !pPresentModes && pPresentModeCount) { // Record the result of this preliminary query: pPhysicalDevice->presentModeCount = *pPresentModeCount; - } else if ((result == VK_SUCCESS) && pPhysicalDevice && pPresentModes && pPresentModeCount) { - // Compare the preliminary value of *pPresentModeCount with the - // value this time: - if (*pPresentModeCount > pPhysicalDevice->presentModeCount) { - LOG_ERROR_INVALID_COUNT(VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, physicalDevice, "pPresentModeCount", - "pPresentModes", *pPresentModeCount, pPhysicalDevice->presentModeCount); - } else if (*pPresentModeCount > 0) { - // Record the result of this query: - pPhysicalDevice->presentModeCount = *pPresentModeCount; - pPhysicalDevice->pPresentModes = (VkPresentModeKHR *)malloc(*pPresentModeCount * sizeof(VkPresentModeKHR)); - if (pPhysicalDevice->pPresentModes) { - for (uint32_t i = 0; i < *pPresentModeCount; i++) { - pPhysicalDevice->pPresentModes[i] = pPresentModes[i]; - } - } else { - pPhysicalDevice->presentModeCount = 0; + } else if ((result == VK_SUCCESS) && pPhysicalDevice && + pPresentModes && pPresentModeCount && + (*pPresentModeCount > 0)) { + // Record the result of this query: + pPhysicalDevice->presentModeCount = *pPresentModeCount; + pPhysicalDevice->pPresentModes = (VkPresentModeKHR *)malloc(*pPresentModeCount * sizeof(VkPresentModeKHR)); + if (pPhysicalDevice->pPresentModes) { + for (uint32_t i = 0; i < *pPresentModeCount; i++) { + pPhysicalDevice->pPresentModes[i] = pPresentModes[i]; } + } else { + pPhysicalDevice->presentModeCount = 0; } } return result; @@ -1665,6 +1689,23 @@ vkGetSwapchainImagesKHR(VkDevice device, VkSwapchainKHR swapchain, uint32_t *pSw SwpSwapchain *pSwapchain = &my_data->swapchainMap[swapchain]; if (!pSwapchainImageCount) { skipCall |= LOG_ERROR_NULL_POINTER(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT, device, "pSwapchainImageCount"); + } else if (pSwapchain && pSwapchainImages) { + // Compare the preliminary value of *pSwapchainImageCount with the + // value this time: + if (pSwapchain->imageCount == 0) { + // Since we haven't recorded a preliminary value of + // *pSwapchainImageCount, that likely means that the application + // didn't previously call this function with a NULL value of + // pSwapchainImages: + skipCall |= LOG_ERROR_ZERO_PRIOR_COUNT( + VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT, + device, "pSwapchainImageCount", "pSwapchainImages"); + } else if (*pSwapchainImageCount > pSwapchain->imageCount) { + skipCall |= LOG_ERROR_INVALID_COUNT( + VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT, + device, "pSwapchainImageCount", "pSwapchainImages", + *pSwapchainImageCount, pSwapchain->imageCount); + } } if (!skipCall) { @@ -1678,20 +1719,14 @@ vkGetSwapchainImagesKHR(VkDevice device, VkSwapchainKHR swapchain, uint32_t *pSw if ((result == VK_SUCCESS) && pSwapchain && !pSwapchainImages && pSwapchainImageCount) { // Record the result of this preliminary query: pSwapchain->imageCount = *pSwapchainImageCount; - } else if ((result == VK_SUCCESS) && pSwapchain && pSwapchainImages && pSwapchainImageCount) { - // Compare the preliminary value of *pSwapchainImageCount with the - // value this time: - if (*pSwapchainImageCount > pSwapchain->imageCount) { - LOG_ERROR_INVALID_COUNT(VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT, device, "pSwapchainImageCount", "pSwapchainImages", - *pSwapchainImageCount, pSwapchain->imageCount); - } else if (*pSwapchainImageCount > 0) { - // Record the images and their state: - pSwapchain->imageCount = *pSwapchainImageCount; - for (uint32_t i = 0; i < *pSwapchainImageCount; i++) { - pSwapchain->images[i].image = pSwapchainImages[i]; - pSwapchain->images[i].pSwapchain = pSwapchain; - pSwapchain->images[i].acquiredByApp = false; - } + } else if ((result == VK_SUCCESS) && pSwapchain && pSwapchainImages && + pSwapchainImageCount && (*pSwapchainImageCount > 0)) { + // Record the images and their state: + pSwapchain->imageCount = *pSwapchainImageCount; + for (uint32_t i = 0; i < *pSwapchainImageCount; i++) { + pSwapchain->images[i].image = pSwapchainImages[i]; + pSwapchain->images[i].pSwapchain = pSwapchain; + pSwapchain->images[i].acquiredByApp = false; } } return result; diff --git a/layers/swapchain.h b/layers/swapchain.h index 21a0dc6f..acbdaf48 100644 --- a/layers/swapchain.h +++ b/layers/swapchain.h @@ -61,6 +61,7 @@ typedef enum _SWAPCHAIN_ERROR { SWAPCHAIN_INDEX_TOO_LARGE, // Index is too large for swapchain SWAPCHAIN_INDEX_NOT_IN_USE, // vkQueuePresentKHR() given index that is not acquired by app SWAPCHAIN_BAD_BOOL, // VkBool32 that doesn't have value of VK_TRUE or VK_FALSE (e.g. is a non-zero form of true) + SWAPCHAIN_PRIOR_COUNT, // Query must be called first to get value of pCount, then called second time SWAPCHAIN_INVALID_COUNT, // Second time a query called, the pCount value didn't match first time SWAPCHAIN_WRONG_STYPE, // The sType for a struct has the wrong value SWAPCHAIN_WRONG_NEXT, // The pNext for a struct is not NULL @@ -92,6 +93,12 @@ typedef enum _SWAPCHAIN_ERROR { "was returned when %s was NULL.", \ __FUNCTION__, (obj2), (obj), (val), (val2), (obj2)) \ : VK_FALSE +#define LOG_ERROR_ZERO_PRIOR_COUNT(objType, type, obj, obj2) \ + (my_data) ? log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, (objType), (uint64_t)(obj), 0, \ + SWAPCHAIN_PRIOR_COUNT, LAYER_NAME, "%s() called with non-NULL %s; but no prior " \ + "positive value has been seen for %s.", \ + __FUNCTION__, (obj), (obj2)) \ + : VK_FALSE #define LOG_ERROR_WRONG_STYPE(objType, type, obj, val) \ (my_data) ? log_msg(my_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, (objType), (uint64_t)(obj), 0, SWAPCHAIN_WRONG_STYPE, \ LAYER_NAME, "%s() called with the wrong value for %s->sType " \ diff --git a/layers/vk_validation_layer_details.md b/layers/vk_validation_layer_details.md index 53882412..bf1c4ac8 100644 --- a/layers/vk_validation_layer_details.md +++ b/layers/vk_validation_layer_details.md @@ -379,6 +379,7 @@ This layer is a work in progress. VK_LAYER_LUNARG_swapchain layer is intended to | Index too large | Validates that an image index is within the number of images in a swapchain | INDEX_TOO_LARGE | vkQueuePresentKHR | NA | None | | Can't present a non-owned image | Validates that application only presents images that it owns | INDEX_NOT_IN_USE | vkQueuePresentKHR | NA | None | | A VkBool32 must have values of VK_TRUE or VK_FALSE | Validates that a VkBool32 must have values of VK_TRUE or VK_FALSE | BAD_BOOL | vkCreateSwapchainKHR | NA | None | +| pCount must be set by the API before the other pointer is non-NULL | Validates that app queries for the value of pCount before trying to set it | PRIOR_COUNT | vkGetPhysicalDeviceSurfaceFormatsKHR vkGetPhysicalDeviceSurfacePresentModesKHR vkGetSwapchainImagesKHR | NA | None | | pCount must point to same value regardless of whether other pointer is NULL | Validates that app doesn't change value of pCount returned by a query | INVALID_COUNT | vkGetPhysicalDeviceSurfaceFormatsKHR vkGetPhysicalDeviceSurfacePresentModesKHR vkGetSwapchainImagesKHR | NA | None | | Valid sType | Validates that a struct has correct value for sType | WRONG_STYPE | vkCreateSwapchainKHR vkQueuePresentKHR | NA | None | | Valid pNext | Validates that a struct has NULL for the value of pNext | WRONG_NEXT | vkCreateSwapchainKHR vkQueuePresentKHR | NA | None | -- cgit v1.2.3