commit acee31423468efdb650be9dc9172b30cc2015a4e
author Brandon Jones <bajones@chromium.org> Mon Nov 06 21:11:30 2023 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Nov 06 21:11:30 2023 +0000
tree 390653b17733be812affb748c32060ae5115f8a9
parent 8596535bf4a8664d9e1b2b93c66c26b287ee6893

Explicitly handle NULL Vulkan debug message IDs

The Vulkan spec states that the pMessageIdName for a debug message may
be NULL. We were previously passing it into a LogMessage (which wraps an
std::ostream) unchecked, but passing a NULL into an ostream is undefined
behavior. This was likely leading to crashes on some devices.

This patch updates the logging behavior to check for NULL and take a
different path if encountered to avoid triggering undefined behavior.

Bug: chromium:1497136
Change-Id: I431539dc21f51fa48bb754f5bc9c85acf2ce8f04
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/159040
Reviewed-by: Austin Eng <enga@chromium.org>
Auto-Submit: Brandon Jones <bajones@chromium.org>
Commit-Queue: Austin Eng <enga@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
(cherry picked from commit 3a8735eb1fff33fe303b099623c870facb9f7268)
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/159032
Commit-Queue: Brandon Jones <bajones@chromium.org>
Kokoro: Brandon Jones <bajones@chromium.org>

src/dawn/native/vulkan/BackendVk.cpp

diff --git a/src/dawn/native/vulkan/BackendVk.cpp b/src/dawn/native/vulkan/BackendVk.cpp
index 419c29a..6aba430 100644
--- a/src/dawn/native/vulkan/BackendVk.cpp
+++ b/src/dawn/native/vulkan/BackendVk.cpp
@@ -161,9 +161,10 @@
 
 // Suppress validation errors that are known. Returns false in that case.
 bool ShouldReportDebugMessage(const char* messageId, const char* message) {
-    // pMessageIdName may be NULL
-    if (messageId == nullptr) {
-        return true;
+    // If a driver gives us a NULL pMessage (which would be a violation of the Vulkan spec)
+    // then ignore this message.
+    if (message == nullptr) {
+        return false;
     }
 
     // Some Vulkan drivers send "error" messages of "VK_SUCCESS" when zero devices are
@@ -175,6 +176,12 @@
         return false;
     }
 
+    // The Vulkan spec does allow pMessageIdName to be NULL, but it may still contain a valid
+    // message. Since we can't compare it with our skipped message list allow it through.
+    if (messageId == nullptr) {
+        return true;
+    }
+
     for (const SkippedMessage& msg : kSkippedMessages) {
         if (strstr(messageId, msg.messageId) != nullptr &&
             strstr(message, msg.messageContents) != nullptr) {
@@ -184,6 +191,21 @@
     return true;
 }
 
+void LogCallbackData(LogSeverity severity,
+                     const VkDebugUtilsMessengerCallbackDataEXT* pCallbackData) {
+    LogMessage log = LogMessage(severity);
+
+    // pMessageIdName may be NULL, according to the Vulkan spec. Passing NULL into an ostream is
+    // undefined behavior, so we'll handle that scenario separately.
+    if (pCallbackData->pMessageIdName != nullptr) {
+        log << pCallbackData->pMessageIdName;
+    } else {
+        log << "nullptr";
+    }
+
+    log << ": " << pCallbackData->pMessage;
+}
+
 VKAPI_ATTR VkBool32 VKAPI_CALL
 OnDebugUtilsCallback(VkDebugUtilsMessageSeverityFlagBitsEXT messageSeverity,
                      VkDebugUtilsMessageTypeFlagsEXT /* messageTypes */,
@@ -194,7 +216,7 @@
     }
 
     if (!(messageSeverity & VK_DEBUG_UTILS_MESSAGE_SEVERITY_ERROR_BIT_EXT)) {
-        dawn::WarningLog() << pCallbackData->pMessageIdName << ": " << pCallbackData->pMessage;
+        LogCallbackData(LogSeverity::Warning, pCallbackData);
         return VK_FALSE;
     }
 
@@ -220,7 +242,7 @@
 
     // We get to this line if no device was associated with the message. Crash so that the failure
     // is loud and makes tests fail in Debug.
-    dawn::ErrorLog() << pCallbackData->pMessageIdName << ": " << pCallbackData->pMessage;
+    LogCallbackData(LogSeverity::Error, pCallbackData);
     DAWN_ASSERT(false);
 
     return VK_FALSE;