gpu: vulkan: Fix double free of debug report callback after failed init

If VulkanInstance fails to initialize, Destroy() will be called twice, once in VulkanImplementation::InitializeVulkanInstance() and once in the destructor. The VkInstanceKHR is not double freed because there is a null check. Add a null check for the debug report callback, and also remove the explicit Destroy() calls since there isn't a care where we'll retry a failed vulkan initialization with the same object. Bug: 921276 Test: Run on fuchsia with partial vulkan layer/ext naming transition Change-Id: I02b3b7dfba5bcebdc2d4010bcb2f2690f125e78e Reviewed-on: https://chromium-review.googlesource.com/c/1407254 Commit-Queue: Michael Spang <[email protected]> Reviewed-by: Antoine Labour <[email protected]> Reviewed-by: Sergey Ulanov <[email protected]> Cr-Commit-Position: refs/heads/master@{#622613}
chensowf · Jan 14, 2019 · 263ece6 · 263ece6
1 parent 263ba0a
commit 263ece6
Show file tree

Hide file tree

Showing 7 changed files with 18 additions and 37 deletions.
diff --git a/gpu/vulkan/android/vulkan_implementation_android.cc b/gpu/vulkan/android/vulkan_implementation_android.cc
@@ -35,20 +35,15 @@ bool VulkanImplementationAndroid::InitializeVulkanInstance() {
   if (!vulkan_function_pointers->vulkan_loader_library_)
     return false;
 
-  if (!vulkan_instance_.Initialize(required_extensions, {})) {
-    vulkan_instance_.Destroy();
+  if (!vulkan_instance_.Initialize(required_extensions, {}))
     return false;
-  }
 
   // Initialize platform function pointers
   vkCreateAndroidSurfaceKHR_ =
       reinterpret_cast<PFN_vkCreateAndroidSurfaceKHR>(vkGetInstanceProcAddr(
           vulkan_instance_.vk_instance(), "vkCreateAndroidSurfaceKHR"));
-  if (!vkCreateAndroidSurfaceKHR_) {
-    LOG(ERROR) << "vkCreateAndroidSurfaceKHR not found";
-    vulkan_instance_.Destroy();
+  if (!vkCreateAndroidSurfaceKHR_)
     return false;
-  }
 
   return true;
 }

diff --git a/gpu/vulkan/vulkan_instance.cc b/gpu/vulkan/vulkan_instance.cc
@@ -228,21 +228,25 @@ void VulkanInstance::Destroy() {
             vkGetInstanceProcAddr(vk_instance_,
                                   "vkDestroyDebugReportCallbackEXT"));
     DCHECK(vkDestroyDebugReportCallbackEXT);
-    if (error_callback_ != VK_NULL_HANDLE)
+    if (error_callback_ != VK_NULL_HANDLE) {
       vkDestroyDebugReportCallbackEXT(vk_instance_, error_callback_, nullptr);
-    if (warning_callback_ != VK_NULL_HANDLE)
+      error_callback_ = VK_NULL_HANDLE;
+    }
+    if (warning_callback_ != VK_NULL_HANDLE) {
       vkDestroyDebugReportCallbackEXT(vk_instance_, warning_callback_, nullptr);
+      warning_callback_ = VK_NULL_HANDLE;
+    }
   }
 #endif
   if (vk_instance_ != VK_NULL_HANDLE) {
     vkDestroyInstance(vk_instance_, nullptr);
+    vk_instance_ = VK_NULL_HANDLE;
   }
   VulkanFunctionPointers* vulkan_function_pointers =
       gpu::GetVulkanFunctionPointers();
   if (vulkan_function_pointers->vulkan_loader_library_)
     base::UnloadNativeLibrary(vulkan_function_pointers->vulkan_loader_library_);
   vulkan_function_pointers->vulkan_loader_library_ = nullptr;
-  vk_instance_ = VK_NULL_HANDLE;
 }
 
 }  // namespace gpu
diff --git a/gpu/vulkan/vulkan_instance.h b/gpu/vulkan/vulkan_instance.h
@@ -29,15 +29,15 @@ class VULKAN_EXPORT VulkanInstance {
   bool Initialize(const std::vector<const char*>& required_extensions,
                   const std::vector<const char*>& required_layers);
 
-  void Destroy();
-
   const gfx::ExtensionSet& enabled_extensions() const {
     return enabled_extensions_;
   }
 
   VkInstance vk_instance() { return vk_instance_; }
 
  private:
+  void Destroy();
+
   VkInstance vk_instance_ = VK_NULL_HANDLE;
   gfx::ExtensionSet enabled_extensions_;
   bool debug_report_enabled_ = false;

diff --git a/gpu/vulkan/win32/vulkan_implementation_win32.cc b/gpu/vulkan/win32/vulkan_implementation_win32.cc
@@ -30,10 +30,8 @@ bool VulkanImplementationWin32::InitializeVulkanInstance() {
   if (!vulkan_function_pointers->vulkan_loader_library_)
     return false;
 
-  if (!vulkan_instance_.Initialize(required_extensions, {})) {
-    vulkan_instance_.Destroy();
+  if (!vulkan_instance_.Initialize(required_extensions, {}))
     return false;
-  }
 
   // Initialize platform function pointers
   vkGetPhysicalDeviceWin32PresentationSupportKHR_ =
@@ -43,7 +41,6 @@ bool VulkanImplementationWin32::InitializeVulkanInstance() {
               "vkGetPhysicalDeviceWin32PresentationSupportKHR"));
   if (!vkGetPhysicalDeviceWin32PresentationSupportKHR_) {
     LOG(ERROR) << "vkGetPhysicalDeviceWin32PresentationSupportKHR not found";
-    vulkan_instance_.Destroy();
     return false;
   }
 
@@ -52,7 +49,6 @@ bool VulkanImplementationWin32::InitializeVulkanInstance() {
           vulkan_instance_.vk_instance(), "vkCreateWin32SurfaceKHR"));
   if (!vkCreateWin32SurfaceKHR_) {
     LOG(ERROR) << "vkCreateWin32SurfaceKHR not found";
-    vulkan_instance_.Destroy();
     return false;
   }
 

diff --git a/gpu/vulkan/x/vulkan_implementation_x11.cc b/gpu/vulkan/x/vulkan_implementation_x11.cc
@@ -36,10 +36,8 @@ bool VulkanImplementationX11::InitializeVulkanInstance() {
   if (!vulkan_function_pointers->vulkan_loader_library_)
     return false;
 
-  if (!vulkan_instance_.Initialize(required_extensions, {})) {
-    vulkan_instance_.Destroy();
+  if (!vulkan_instance_.Initialize(required_extensions, {}))
     return false;
-  }
 
   // Initialize platform function pointers
   vkGetPhysicalDeviceXlibPresentationSupportKHR_ =
@@ -49,7 +47,6 @@ bool VulkanImplementationX11::InitializeVulkanInstance() {
               "vkGetPhysicalDeviceXlibPresentationSupportKHR"));
   if (!vkGetPhysicalDeviceXlibPresentationSupportKHR_) {
     LOG(ERROR) << "vkGetPhysicalDeviceXlibPresentationSupportKHR not found";
-    vulkan_instance_.Destroy();
     return false;
   }
 
@@ -58,7 +55,6 @@ bool VulkanImplementationX11::InitializeVulkanInstance() {
           vulkan_instance_.vk_instance(), "vkCreateXlibSurfaceKHR"));
   if (!vkCreateXlibSurfaceKHR_) {
     LOG(ERROR) << "vkCreateXlibSurfaceKHR not found";
-    vulkan_instance_.Destroy();
     return false;
   }
 

diff --git a/ui/ozone/platform/drm/gpu/vulkan_implementation_gbm.cc b/ui/ozone/platform/drm/gpu/vulkan_implementation_gbm.cc
@@ -31,27 +31,21 @@ bool VulkanImplementationGbm::InitializeVulkanInstance() {
       "VK_KHR_external_fence_capabilities",
       "VK_KHR_get_physical_device_properties2",
   };
-  if (!vulkan_instance_.Initialize(required_extensions, {})) {
-    vulkan_instance_.Destroy();
+  if (!vulkan_instance_.Initialize(required_extensions, {}))
     return false;
-  }
 
   vkGetPhysicalDeviceExternalFencePropertiesKHR_ =
       reinterpret_cast<PFN_vkGetPhysicalDeviceExternalFencePropertiesKHR>(
           vkGetInstanceProcAddr(
               vulkan_instance_.vk_instance(),
               "vkGetPhysicalDeviceExternalFencePropertiesKHR"));
-  if (!vkGetPhysicalDeviceExternalFencePropertiesKHR_) {
-    vulkan_instance_.Destroy();
+  if (!vkGetPhysicalDeviceExternalFencePropertiesKHR_)
     return false;
-  }
 
   vkGetFenceFdKHR_ = reinterpret_cast<PFN_vkGetFenceFdKHR>(
       vkGetInstanceProcAddr(vulkan_instance_.vk_instance(), "vkGetFenceFdKHR"));
-  if (!vkGetFenceFdKHR_) {
-    vulkan_instance_.Destroy();
+  if (!vkGetFenceFdKHR_)
     return false;
-  }
 
   return true;
 }

diff --git a/ui/ozone/platform/scenic/vulkan_implementation_scenic.cc b/ui/ozone/platform/scenic/vulkan_implementation_scenic.cc
@@ -51,19 +51,15 @@ bool VulkanImplementationScenic::InitializeVulkanInstance() {
   std::vector<const char*> required_layers = {
       "VK_LAYER_FUCHSIA_imagepipe_swapchain",
   };
-  if (!vulkan_instance_.Initialize(required_extensions, required_layers)) {
-    vulkan_instance_.Destroy();
+  if (!vulkan_instance_.Initialize(required_extensions, required_layers))
     return false;
-  }
 
   vkCreateImagePipeSurfaceFUCHSIA_ =
       reinterpret_cast<PFN_vkCreateImagePipeSurfaceFUCHSIA>(
           vkGetInstanceProcAddr(vulkan_instance_.vk_instance(),
                                 "vkCreateImagePipeSurfaceFUCHSIA"));
-  if (!vkCreateImagePipeSurfaceFUCHSIA_) {
-    vulkan_instance_.Destroy();
+  if (!vkCreateImagePipeSurfaceFUCHSIA_)
     return false;
-  }
 
   return true;
 }