Crash in loader when driver returns VK_NULL_HANDLE with VK_SUCCESS status on creating dispatchable object.

[zaiste-linganer]

There are the drivers in the wild that will allow returning VK_NULL_HANDLE with VK_SUCCESS status for vkCreate* where Vk* is dispatchable object, for example VkDevice.

The behavior from the driver is driven by the assumption that if the failure is in non-spec'd error, driver is free to ignore the errors and return VK_SUCCESS, even if VK_NULL_HANDLE is being returned for the object not correctly created.

Aside from the discussion if this behavior is correct or not, the loader shouldn't crash on that. Right now, on VK_SUCCESS it's trying to initialize dispatchable handle on VK_NULL_HANDLE, and causes Exception Access Violation.

I will post the fix for the crash in a moment, but I'd like to open the issue for the discussion if:
a) Loader should just pass returned VK_NULL to the application OR
b) Loader is free to translate it to VK_ERROR_UNKNOWN or something similar.

I'll start with the (a), but I think (b) is more viable, even though it's also more intrusive.

@charles-lunarg, WDYT?

[zaiste-linganer]

@charles-lunarg is there any chance to get the review for proposed change? Or should I tag someone else? Thanks in advance!

[charles-lunarg]

Thinking about the meaning of errors and NULL handles, if the application receives VK_SUCCESS but VkDevice is still NULL, then the next call which takes a VkDevice will crash. Put another way, this is just delaying the inevitable. I don't know how a driver is suppose to behave when called with a NULL VkDevice dispatchable handle.

So a) isn't a solution, just a crash in a different place. b) is more actionable - applications will (hopefully) catch it, and doesn't violate the spec.

My understanding of the spec is that if vkCreateDevice returns VK_SUCCESS, a VkDevice handle has been written into pDevice. This is because all functions that take a VkDevice dispatchable handle require that to be non-NULL. VUID-vkGetDeviceQueue-device-parameter is a very common VUID I see when device creation fails but user didn't check the return code of vkCreateDevice.

Can you provide more details about the drivers that are exhibiting this behavior?
Also are there more details about non-spec'd error's? That isn't something I'm aware of.

[zaiste-linganer]

I believe that it's surprising behavior, but the crash in the loader is... just bad? Loader should be opaque in this case, I believe, or explicitly turn it into the error. Crashing without giving the application chance to recover is the worst option here ;)

On our end (Android emulation platform code), we are able to catch this scenario, and act accordingly, at least collecting (and potentially giving the user) a more meaningful error.

And it doesn't really matter if it's a driver's fault, I guess, or application's fault. The loader can handle this scenario gracefully, so I believe it should, hence my proposal in #1707.

@pbialeck Can you chime in here with more explanations for the drivers side POV?

[charles-lunarg]

the crash in the loader is... just bad?

There are many places in the loader where it intentionally crashes (although usually we catch, emit a log message, then call abort()). This is intentional and is the working group agreed upon behavior.

I would very much like to see more details on the "non-spec'd error" because from my point of view, driver returning VK_SUCCESS and VK_NULL_HANDLE is violating the Vulkan spec and thus a 'spec'd error'. The loader assumes drivers follow the spec, and makes appropriate assumptions based on that, including crashing due to behavior drivers exhibit that aren't spec conformant.

Doing a bit of digging, CTS checks that the return value isn't NULL, so I would be very surprised if the driver in question passed CTS while exhibiting the behavior above.
https://github.com/KhronosGroup/VK-GL-CTS/blob/53a925d9098cd2a1856d72cdcebfccb7a833a50c/external/vulkancts/framework/vulkan/vkRef.hpp#L50C1-L58C2

Edit: Found where cts actually uses the check, which is done after using VK_CHECK to validate the return values of vkCreateDevice. https://github.com/KhronosGroup/VK-GL-CTS/blob/53a925d9098cd2a1856d72cdcebfccb7a833a50c/external/vulkancts/framework/vulkan/generated/vulkan/vkRefUtilImpl.inl#L270

[zaiste-linganer]

I may be omitted important clarification - this is a sporadic issue. Driver insists that it's due to wrong input parameters, that causes WDDM error that is not covered by existing Vulkan errors, and pretty much fails into the application's fault/VVL area of resolution on the application side.

From our end on the platform side it does not look like the issue with input parameters but rather to the state of the machine that executes call. We are trying to dig deeper into that... But if the fix is to be provided by the driver, there will still be a drivers in the wild that crashes.

Explicit abort may be fine, but access violation is never a good way to fail. I do think, though, that the loader should be either opaque here, or translate it to the proper Vulkan error, as there is no reason just to crash on that error.

[charles-lunarg]

I'm going to declare that this is a wont fix. Not the response you wanted, but the driver here shouldn't be reporting VK_SUCCESS. Is this a situation where the driver is never actually run, so there is no way to return VK_ERROR_INITIALIZATION_FAILED?

In general, the loader doesn't workaround driver bugs. In specific, only vkCreateDevice could have a reasonable workaround, returning VK_ERROR_INITIALIZATION_FAILED. vkAllocateCommandBuffers only allows returning OOM errors, and vkGetDeviceQueues which has no return value that would indicate an issue to applications.

But if the fix is to be provided by the driver, there will still be a drivers in the wild that crashes.

That is irrelevant, as the same situation exists if the loader adds mitigations. There will be systems without a mitigated loader, same as if there are systems with a driver with the bug. So I don't see how a loader mitigation is any different. On windows, the Vulkan-Loader is (typically) installed alongside a driver, so typically only systems with a new driver install would also have the loader updated.

[zaiste-linganer]

windows, the Vulkan-Loader is (typically) installed alongside a driver, so typically only systems with a new driver install would also have the loader updated.

For this exact reason (along with some local changes needed for our product) we distribute the loader ourselves. I can keep the fix in the downstream (as we do right now), but we are not the only ones affected by the issue, as far as I know, and not the only ones that provide its own loader binary.

the driver here shouldn't be reporting VK_SUCCESS

Let's keep this on hold then and I will request spec clarification. Driver team was adamant that in the situation where the error is not the drivers fault nor the system fault, it's application responsibility to handle the situation gracefully. The spec itself is not super clear on that, so let's go through the appropriate spec clarification pass here.

I don't see how a loader mitigation is any different.

Well, it's simple. If the loader does not crash, the application can handle the error gracefully. If the loader crashes, there is no such option. Consider that an ecosystem issue, more than simply "driver" or "loader" issue. If application is doing something legitimate, or at least as legitimate it can think of, it's bad for the ecosystem to generate hard crash that can be simply avoided. Now, pushing the fix to the different part of the stack is a bad behavior per se, the fix should be present in all the parts of the stack to bring out the best for the ecosystem.