Vulkan-docs: TOCTOU race condition on minImageExtent/maxImageExtent?

[haasn]

Some platforms, like Xcb/Xlib/Win32, require that minImageExtent == maxImageExtent == currentExtent. However, this creates some problematic race conditions in practice.

The problem, specifically, is that the window's extent can change (e.g. due to the window manager) in between the calls to vkGetPhysicalDeviceSurfaceCapabilitiesKHR and vkCreateSwapchainKHR. When this happens, we might end up creating a swapchain with an imageExtent that is outside of the bounds of the permitted min/max at the time of the actual swapchain creation. As far as I can tell, this is technically undefined behavior? (In practice, one of the validation layers likes to error out on swapchain creation, so this is not just a theoretical concern - we have to work around it by suppressing errors related to swapchain creation from that validation layer).

Even ignoring split second race conditions, I'm not sure how a field like minImageExtent or maxImageExtent being "dynamic" is supposed to make sense at all. The specification for vkCreateSwapchainKHR only says:

imageExtent must be between minImageExtent and maxImageExtent, inclusive, where minImageExtent and maxImageExtent are members of the VkSurfaceCapabilitiesKHR structure returned by vkGetPhysicalDeviceSurfaceCapabilitiesKHR for the surface

but the fact that minImageExtent and maxImageExtent can change over time means this wording is vague. There's no clear link between any particular call to vkGetPhysicalDeviceSurfaceCapabilitiesKHR and the actual swapchain creation.

Obviously the intent was to mean "at the time of swapchain creation", but this leads back into aforementioned race condition, because there's always going to ba delay in between those two calls.

I'm also not sure what purpose this design is supposed to serve. There's nothing stopping users from making swapchains with the "wrong" size, either in practice or in theory. In fact, doing so can sometimes be beneficial. The obvious solution to me would be to remove the min == max == current assumption and just have min and max always represent the (unchanging) hardware capabilities on all platforms.

[krOoze]

practically a dup of more general #388. Hopefully it gets prioritized though.
We also got into it at https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/1340. Quick fix would possibly be to remove the VU; the driver should rely on VK_ERROR_OUT_OF_DATE_KHR given the possiblity of the "race condition" described above.

[haasn]

FWIW I sent a patchset to mesa that makes it return SUBOPTIMAL in the case of this "race condition". (That patchset also tried changing the min/max extent as per my recommendation, but they correctly pointed out that the current spec forbids this)

(Side note: OUT_OF_DATE seems like an excessively harsh error to return for size mismatches. Using SUBOPTIMAL instead allows applications to handle resizes much more smoothly and gracefully)

Simply removing the validation check, combined with the above patchset, allows this to be a non-issue in practice - but the theoretical problem remains (as tracked by #388).

[krOoze]

(Side note: OUT_OF_DATE seems like an excessively harsh error to return for size mismatches. Using SUBOPTIMAL instead allows applications to handle resizes much more smoothly and gracefully)

Depends what you mean by "gracefully". There would be artifacts if the framebuffer size does not match the window. They intentionally removed that possibility in spec on some platforms, because the output is practically useless(, though theoretically it is possible to present into it, I think). The expectation of SUBOPTIMAL is that it is somewhat usable, though the details are kept to the impl (EDIT: except Android and Stadia, where there is guaranteed scale).

The idea is that you get the VK_ERROR_OUT_OF_DATE_KHR at some point and if so, restart the swapchain recreation. Apps should already have code to handle VK_ERROR_OUT_OF_DATE_KHR.

Simply removing the validation check, combined with the above patchset, allows this to be a non-issue in practice - but the theoretical problem remains

Then the spec only needs to state the obvious that the window size can change at any time in platform specific manner.

[haasn]

Depends what you mean by "gracefully". There would be artifacts if the framebuffer size does not match the window.

In practice, the artifacts of framebuffer size mismatch are maybe a few pixels of lost content / black borders here and there (usually on the border you're resizing from), whereas the artifacts of excessive swapchain recreation is the entire window becoming visually frozen while resizing.

It's possible to become so insistent on only rendering 'perfect' images that you end up not rendering anything at all. A few pixels of artifacts here and there are much more graceful than great swathes of unupdated, corrupted framebuffer contents that are the typical result of a failure to repaint in a timely fashion.

[krOoze]

Black frame can actually be better than a mangled image in an unspecified way...

[jekstrand]

imageExtent must be between minImageExtent and maxImageExtent, inclusive, where minImageExtent and maxImageExtent are members of the VkSurfaceCapabilitiesKHR structure returned by vkGetPhysicalDeviceSurfaceCapabilitiesKHR for the surface

but the fact that minImageExtent and maxImageExtent can change over time means this wording is vague. There's no clear link between any _particular_ call to vkGetPhysicalDeviceSurfaceCapabilitiesKHR and the actual swapchain creation.

Obviously the intent was to mean "at the time of swapchain creation", but this leads back into aforementioned race condition, because there's always going to ba delay in between those two calls.

Yes, as-stated that's racy and impossible to actually satisfy reliably in the middle of a resize. I believe the way this should have been spec'd and the way you should have interpreted it is something more like this:

imageExtent should be between minImageExtent and maxImageExtent, inclusive, where minImageExtent and maxImageExtent are members of the VkSurfaceCapabilitiesKHR structure returned by vkGetPhysicalDeviceSurfaceCapabilitiesKHR for the surface. If imageExtent is not between minImageExtent and maxImageExtent at any time, the swapchain will return VK_ERROR_OUT_OF_DATE

That may not be quite precise enough for spec text but I think it's pretty close and carries the intent.