[Mesa-dev] [PATCH] vulkan/wsi/x11: don't crash on null wsi x11 connection

[Jason Ekstrand]

On second thought... I'm not sure how this is possible without an app bug
or genuine out-of-memory (not likely).  We never query any X11 objects
other than visuals and those shouldn't be changing out from under us.  The
only way I can see this happening without out-of-memory is if the app has a
race and close the connection on us.  If that happens, it's an app bug.

I'd probably be ok with handling the out-of-memory case by silently
failing.  However, I don't think a warning printf is really appropriate in
that case.  If an app is actually hitting this, they have a bug.

On Thu, Dec 22, 2016 at 1:00 PM, Jason Ekstrand <jason at jlekstrand.net>
wrote:

> I think it's right to return false if wsi_conn is NULL, but I believe
> there is also an application error here.
>
> From the Vulkan 1.0.37 spec valid usage for vkGetPhysicalDeviceSurfaceSupp
> ortKHR,
>
>    - *connection* must point to a valid X11 *xcb_connection_t*.
>    - *window* must be a valid X11 *xcb_window_t*.
>
> Also,
>
> "Some Vulkan functions may send protocol over the specified xcb
> connection when using a swapchain or presentable images created from a
> VkSurface referring to an xcb window. Applications must therefore ensure
> the xcb connection is available to Vulkan for the duration of any functions
> that manipulate such swapchains or their presentable images, and any
> functions that build or queue command buffers that operate on such
> presentable images."
>
> This seems to imply to me that it's the application's responsibility to
> ensure that the XCB connection and window it passes in are valid and remain
> valid for the duration of the call.
>
> On the flip side, this is X11 and it's inherently racy.  It's possible for
> some other client to delete our window out from under us so we should
> probably handle that.  However, it's not our job to handle races inside the
> same process.
> In short, I'm happy to take a patch that does
>
> if (!wsi_conn)
>    return false;
>
> because it handles the out-of-memory and crazy X races case, but you
> should also fix your app.
>
> On Mon, Dec 19, 2016 at 7:59 PM, Arda Coskunses <acoskunses at gmail.com>
> wrote:
>
>> Without this check driver crash when application window
>> closed unexpectedly.
>> ---
>>  src/vulkan/wsi/wsi_common_x11.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/src/vulkan/wsi/wsi_common_x11.c
>> b/src/vulkan/wsi/wsi_common_x11.c
>> index 25ba0c1..afb7809 100644
>> --- a/src/vulkan/wsi/wsi_common_x11.c
>> +++ b/src/vulkan/wsi/wsi_common_x11.c
>> @@ -261,6 +261,11 @@ VkBool32 wsi_get_physical_device_xcb_pr
>> esentation_support(
>>     struct wsi_x11_connection *wsi_conn =
>>        wsi_x11_get_connection(wsi_device, alloc, connection);
>>
>> +   if (!wsi_conn) {
>> +      fprintf(stderr, "vulkan: wsi connection lost\n");
>> +      return false;
>> +   }
>> +
>>     if (!wsi_conn->has_dri3) {
>>        fprintf(stderr, "vulkan: No DRI3 support\n");
>>        return false;
>> --
>> 2.7.4
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161222/fcd694fa/attachment-0001.html>