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

[Jason Ekstrand]

Pushed with my R-B

On Thu, Dec 22, 2016 at 2:03 PM, Arda Coskunses 
wrote:

> Without this check driver crash when application window
> closed unexpectedly.
> Acked-by: Edward O'Callaghan 
> ---
>  src/vulkan/wsi/wsi_common_x11.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/vulkan/wsi/wsi_common_x11.c b/src/vulkan/wsi/wsi_common_
> x11.c
> index 037aa50..345740e 100644
> --- a/src/vulkan/wsi/wsi_common_x11.c
> +++ b/src/vulkan/wsi/wsi_common_x11.c
> @@ -261,6 +261,10 @@ VkBool32 wsi_get_physical_device_xcb_
> presentation_support(
> struct wsi_x11_connection *wsi_conn =
>wsi_x11_get_connection(wsi_device, alloc, connection);
>
> +   if (!wsi_conn) {
> +  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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Arda Coskunses]

Without this check driver crash when application window
closed unexpectedly.
Acked-by: Edward O'Callaghan 
---
 src/vulkan/wsi/wsi_common_x11.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/vulkan/wsi/wsi_common_x11.c b/src/vulkan/wsi/wsi_common_x11.c
index 037aa50..345740e 100644
--- a/src/vulkan/wsi/wsi_common_x11.c
+++ b/src/vulkan/wsi/wsi_common_x11.c
@@ -261,6 +261,10 @@ VkBool32 wsi_get_physical_device_xcb_presentation_support(
struct wsi_x11_connection *wsi_conn =
   wsi_x11_get_connection(wsi_device, alloc, connection);
 
+   if (!wsi_conn) {
+  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 
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 
> 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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jason Ekstrand]

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
vkGetPhysicalDeviceSurfaceSupportKHR,

   - *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 
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_
> presentation_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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Eero Tamminen]

Hi,

I think it fixed one of the segfaults with closed vkreplay window, but 
it still segfaults. For details, see backtrace here:

https://github.com/LunarG/VulkanTools/issues/124

- Eero

On 20.12.2016 05:59, Arda Coskunses 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_presentation_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;



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Edward O'Callaghan]

Acked-by: Edward O'Callaghan 

On 12/20/2016 02:59 PM, Arda Coskunses 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_presentation_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;
> 



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Arda Coskunses]

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_presentation_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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev