Re: [Mesa-dev] [PATCH] egl/wayland-egl: Fix for segfault in dri2_wl_destroy_surface.

[Pekka Paalanen]

On Wed, 24 Aug 2016 10:23:11 +0100
Emil Velikov  wrote:

> On 24 August 2016 at 08:48, Stencel, Joanna  wrote:
> > I couldn't find any clear requirement about order of destroys in EGL (or 
> > wayland) specification.
> > Also, in EGL spec (3.7.3, about eglMakeCurrent) one can find:
> >
> > " If a native window underlying either draw or read is no longer valid, an
> > EGL_BAD_NATIVE_WINDOW error is generated."
> > "If a native window or pixmap underlying the draw or read surfaces is
> > destroyed, rendering and readback are handled as above."
> >
> > So it seems that in general destroying native window underlying existing 
> > surface can be a case
> > and should be handled.
> > I agree that it's reasonable to call eglDestroySurface() first and probably 
> > most people do this.
> > However, I think that different user's usage shouldn't cause a crash (or 
> > memory issues).
> >  
> Completely agree - crashing (esp in the driver) isn't what we want.
> Seems like we don't handle the case you mentioned. Care to spin a
> patch ?
> 
> > Could you explain what you call a memory leak here? Pointer which is 
> > nullified points to already
> > free'd structure of wayland window.
> >  
> Hmm you're right - it's the EGL implementation's back pointer that
> gets nullified and not the original one used by the wayland-egl (as I
> originally thought).
> 
> With that said, I've rebased the patch on top of master added the tags
> and pushed to master.

Hi,

I was about to scream that the patch changes the public stable ABI, but
no, it does not and all seems fine. The reason for my mistake is that
there are actually two different "native window" types at play here:
wl_surface and wl_egl_surface. Of course, strictly from EGL point of
view, there is only wl_egl_surface. Here is some background information
in case you are interested for the other case.

You are handling the premature destruction of wl_egl_surface which is
very nice.

Handling the premature destruction of wl_surface is practically
impossible. The story for that can be found in the thread starting at:
https://lists.freedesktop.org/archives/wayland-devel/2016-May/029134.html
and continues in:
https://lists.freedesktop.org/archives/wayland-devel/2016-June/029339.html


Thanks,
pq


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

Re: [Mesa-dev] [PATCH] egl/wayland-egl: Fix for segfault in dri2_wl_destroy_surface.

[Emil Velikov]

On 24 August 2016 at 08:48, Stencel, Joanna  wrote:
> I couldn't find any clear requirement about order of destroys in EGL (or 
> wayland) specification.
> Also, in EGL spec (3.7.3, about eglMakeCurrent) one can find:
>
> " If a native window underlying either draw or read is no longer valid, an
> EGL_BAD_NATIVE_WINDOW error is generated."
> "If a native window or pixmap underlying the draw or read surfaces is
> destroyed, rendering and readback are handled as above."
>
> So it seems that in general destroying native window underlying existing 
> surface can be a case
> and should be handled.
> I agree that it's reasonable to call eglDestroySurface() first and probably 
> most people do this.
> However, I think that different user's usage shouldn't cause a crash (or 
> memory issues).
>
Completely agree - crashing (esp in the driver) isn't what we want.
Seems like we don't handle the case you mentioned. Care to spin a
patch ?

> Could you explain what you call a memory leak here? Pointer which is 
> nullified points to already
> free'd structure of wayland window.
>
Hmm you're right - it's the EGL implementation's back pointer that
gets nullified and not the original one used by the wayland-egl (as I
originally thought).

With that said, I've rebased the patch on top of master added the tags
and pushed to master.

Thanks for the contribution !
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] egl/wayland-egl: Fix for segfault in dri2_wl_destroy_surface.

[Stencel, Joanna]

I couldn't find any clear requirement about order of destroys in EGL (or 
wayland) specification. 
Also, in EGL spec (3.7.3, about eglMakeCurrent) one can find:

" If a native window underlying either draw or read is no longer valid, an
EGL_BAD_NATIVE_WINDOW error is generated."
"If a native window or pixmap underlying the draw or read surfaces is
destroyed, rendering and readback are handled as above."

So it seems that in general destroying native window underlying existing 
surface can be a case
and should be handled. 
I agree that it's reasonable to call eglDestroySurface() first and probably 
most people do this.
However, I think that different user's usage shouldn't cause a crash (or memory 
issues).

Could you explain what you call a memory leak here? Pointer which is nullified 
points to already
free'd structure of wayland window. 

Joanna 

-Original Message-
From: Emil Velikov [mailto:emil.l.veli...@gmail.com] 
Sent: Tuesday, August 23, 2016 6:42 PM
To: Stencel, Joanna <joanna.sten...@intel.com>
Cc: ML mesa-dev <mesa-dev@lists.freedesktop.org>
Subject: Re: [Mesa-dev] [PATCH] egl/wayland-egl: Fix for segfault in 
dri2_wl_destroy_surface.

On 22 August 2016 at 08:48, Stencel, Joanna <joanna.sten...@intel.com> wrote:
> Segfault occurs when destroying EGL surface attached to already 
> destroyed Wayland window. The fix is to set to NULL the pointer of 
> surface's native window when wl_egl_destroy_window() is called.
>
Are you sure one is not supposed to call eglDestroySurface() first and then 
wl_egl_window_destroy() ? As-is the patch "fixes" a crash (by creating a leak) 
caused by user misuse. Should we a) leave the crash to teach people about bugs 
in their code, b) plug the crash yet cause a leak, or c) plus the crash w/o 
causing a leak.

I'm leaning towards a) or c) since b) only papers over things.
-Emil


Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial 
Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | 
Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i 
moze zawierac informacje poufne. W razie przypadkowego otrzymania tej 
wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; 
jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole 
use of the intended recipient(s). If you are not the intended recipient, please 
contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] egl/wayland-egl: Fix for segfault in dri2_wl_destroy_surface.

[Emil Velikov]

On 22 August 2016 at 08:48, Stencel, Joanna  wrote:
> Segfault occurs when destroying EGL surface attached to already destroyed
> Wayland window. The fix is to set to NULL the pointer of surface's
> native window when wl_egl_destroy_window() is called.
>
Are you sure one is not supposed to call eglDestroySurface() first and
then wl_egl_window_destroy() ? As-is the patch "fixes" a crash (by
creating a leak) caused by user misuse. Should we a) leave the crash
to teach people about bugs in their code, b) plug the crash yet cause
a leak, or c) plus the crash w/o causing a leak.

I'm leaning towards a) or c) since b) only papers over things.
-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] egl/wayland-egl: Fix for segfault in dri2_wl_destroy_surface.

[Eric Engestrom]

On Mon, Aug 22, 2016 at 09:48:50AM +0200, Stencel, Joanna wrote:
> Segfault occurs when destroying EGL surface attached to already destroyed
> Wayland window. The fix is to set to NULL the pointer of surface's
> native window when wl_egl_destroy_window() is called.
> 
> Signed-off-by: Stencel, Joanna 

LGTM
Reviewed-by: Eric Engestrom 

> ---
>  src/egl/drivers/dri2/platform_wayland.c| 15 +--
>  src/egl/wayland/wayland-egl/wayland-egl-priv.h |  1 +
>  src/egl/wayland/wayland-egl/wayland-egl.c  |  3 +++
>  3 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_wayland.c 
> b/src/egl/drivers/dri2/platform_wayland.c
> index d2f47cc..821d7c4 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -119,6 +119,13 @@ resize_callback(struct wl_egl_window *wl_win, void *data)
> (*dri2_dpy->flush->invalidate)(dri2_surf->dri_drawable);
>  }
>  
> +static void
> +destroy_window_callback(void *data)
> +{
> +   struct dri2_egl_surface *dri2_surf = data;
> +   dri2_surf->wl_win = NULL;
> +}
> +
>  /**
>   * Called via eglCreateWindowSurface(), drv->API.CreateWindowSurface().
>   */
> @@ -160,6 +167,7 @@ dri2_wl_create_surface(_EGLDriver *drv, _EGLDisplay *disp,
>  
> dri2_surf->wl_win->private = dri2_surf;
> dri2_surf->wl_win->resize_callback = resize_callback;
> +   dri2_surf->wl_win->destroy_window_callback = destroy_window_callback;
>  
> dri2_surf->base.Width = window->width;
> dri2_surf->base.Height = window->height;
> @@ -258,8 +266,11 @@ dri2_wl_destroy_surface(_EGLDriver *drv, _EGLDisplay 
> *disp, _EGLSurface *surf)
> if (dri2_surf->throttle_callback)
>wl_callback_destroy(dri2_surf->throttle_callback);
>  
> -   dri2_surf->wl_win->private = NULL;
> -   dri2_surf->wl_win->resize_callback = NULL;
> +   if (dri2_surf->wl_win) {
> +  dri2_surf->wl_win->private = NULL;
> +  dri2_surf->wl_win->resize_callback = NULL;
> +  dri2_surf->wl_win->destroy_window_callback = NULL;
> +   }
>  
> free(surf);
>  
> diff --git a/src/egl/wayland/wayland-egl/wayland-egl-priv.h 
> b/src/egl/wayland/wayland-egl/wayland-egl-priv.h
> index 74a1552..eae1224 100644
> --- a/src/egl/wayland/wayland-egl/wayland-egl-priv.h
> +++ b/src/egl/wayland/wayland-egl/wayland-egl-priv.h
> @@ -27,6 +27,7 @@ struct wl_egl_window {
>  
>   void *private;
>   void (*resize_callback)(struct wl_egl_window *, void *);
> + void (*destroy_window_callback)(void *);
>  };
>  
>  #ifdef  __cplusplus
> diff --git a/src/egl/wayland/wayland-egl/wayland-egl.c 
> b/src/egl/wayland/wayland-egl/wayland-egl.c
> index 80a5be5..4a4701a 100644
> --- a/src/egl/wayland/wayland-egl/wayland-egl.c
> +++ b/src/egl/wayland/wayland-egl/wayland-egl.c
> @@ -66,6 +66,7 @@ wl_egl_window_create(struct wl_surface *surface,
>   egl_window->surface = surface;
>   egl_window->private = NULL;
>   egl_window->resize_callback = NULL;
> + egl_window->destroy_window_callback = NULL;
>   wl_egl_window_resize(egl_window, width, height, 0, 0);
>   egl_window->attached_width  = 0;
>   egl_window->attached_height = 0;
> @@ -76,6 +77,8 @@ wl_egl_window_create(struct wl_surface *surface,
>  WL_EGL_EXPORT void
>  wl_egl_window_destroy(struct wl_egl_window *egl_window)
>  {
> + if (egl_window->destroy_window_callback)
> + egl_window->destroy_window_callback(egl_window->private);
>   free(egl_window);
>  }
>  
> -- 
> 1.9.1
> 
> 
> 
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII 
> Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 
> 957-07-52-316 | Kapital zakladowy 200.000 PLN.
> 
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i 
> moze zawierac informacje poufne. W razie przypadkowego otrzymania tej 
> wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; 
> jakiekolwiek
> przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the 
> sole use of the intended recipient(s). If you are not the intended recipient, 
> please contact the sender and delete all copies; any review or distribution by
> others is strictly prohibited.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] egl/wayland-egl: Fix for segfault in dri2_wl_destroy_surface.

[Stencel, Joanna]

Segfault occurs when destroying EGL surface attached to already destroyed
Wayland window. The fix is to set to NULL the pointer of surface's
native window when wl_egl_destroy_window() is called.

Signed-off-by: Stencel, Joanna 
---
 src/egl/drivers/dri2/platform_wayland.c| 15 +--
 src/egl/wayland/wayland-egl/wayland-egl-priv.h |  1 +
 src/egl/wayland/wayland-egl/wayland-egl.c  |  3 +++
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_wayland.c 
b/src/egl/drivers/dri2/platform_wayland.c
index d2f47cc..821d7c4 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -119,6 +119,13 @@ resize_callback(struct wl_egl_window *wl_win, void *data)
(*dri2_dpy->flush->invalidate)(dri2_surf->dri_drawable);
 }
 
+static void
+destroy_window_callback(void *data)
+{
+   struct dri2_egl_surface *dri2_surf = data;
+   dri2_surf->wl_win = NULL;
+}
+
 /**
  * Called via eglCreateWindowSurface(), drv->API.CreateWindowSurface().
  */
@@ -160,6 +167,7 @@ dri2_wl_create_surface(_EGLDriver *drv, _EGLDisplay *disp,
 
dri2_surf->wl_win->private = dri2_surf;
dri2_surf->wl_win->resize_callback = resize_callback;
+   dri2_surf->wl_win->destroy_window_callback = destroy_window_callback;
 
dri2_surf->base.Width = window->width;
dri2_surf->base.Height = window->height;
@@ -258,8 +266,11 @@ dri2_wl_destroy_surface(_EGLDriver *drv, _EGLDisplay 
*disp, _EGLSurface *surf)
if (dri2_surf->throttle_callback)
   wl_callback_destroy(dri2_surf->throttle_callback);
 
-   dri2_surf->wl_win->private = NULL;
-   dri2_surf->wl_win->resize_callback = NULL;
+   if (dri2_surf->wl_win) {
+  dri2_surf->wl_win->private = NULL;
+  dri2_surf->wl_win->resize_callback = NULL;
+  dri2_surf->wl_win->destroy_window_callback = NULL;
+   }
 
free(surf);
 
diff --git a/src/egl/wayland/wayland-egl/wayland-egl-priv.h 
b/src/egl/wayland/wayland-egl/wayland-egl-priv.h
index 74a1552..eae1224 100644
--- a/src/egl/wayland/wayland-egl/wayland-egl-priv.h
+++ b/src/egl/wayland/wayland-egl/wayland-egl-priv.h
@@ -27,6 +27,7 @@ struct wl_egl_window {
 
void *private;
void (*resize_callback)(struct wl_egl_window *, void *);
+   void (*destroy_window_callback)(void *);
 };
 
 #ifdef  __cplusplus
diff --git a/src/egl/wayland/wayland-egl/wayland-egl.c 
b/src/egl/wayland/wayland-egl/wayland-egl.c
index 80a5be5..4a4701a 100644
--- a/src/egl/wayland/wayland-egl/wayland-egl.c
+++ b/src/egl/wayland/wayland-egl/wayland-egl.c
@@ -66,6 +66,7 @@ wl_egl_window_create(struct wl_surface *surface,
egl_window->surface = surface;
egl_window->private = NULL;
egl_window->resize_callback = NULL;
+   egl_window->destroy_window_callback = NULL;
wl_egl_window_resize(egl_window, width, height, 0, 0);
egl_window->attached_width  = 0;
egl_window->attached_height = 0;
@@ -76,6 +77,8 @@ wl_egl_window_create(struct wl_surface *surface,
 WL_EGL_EXPORT void
 wl_egl_window_destroy(struct wl_egl_window *egl_window)
 {
+   if (egl_window->destroy_window_callback)
+   egl_window->destroy_window_callback(egl_window->private);
free(egl_window);
 }
 
-- 
1.9.1



Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial 
Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | 
Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i 
moze zawierac informacje poufne. W razie przypadkowego otrzymania tej 
wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; 
jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole 
use of the intended recipient(s). If you are not the intended recipient, please 
contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

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