Re: [Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

2018-04-24 Thread Marek Olšák 
On Tue, Apr 24, 2018 at 3:13 AM, Johan Helsing  wrote:

> Emil: Your alternative patch won't work because dri_make_current is not
> necessarily called with NULL after a buffer has been destroyed.
>
>
> The problematic sequence is a pattern we use in QtWayland:
>
>
> //create temporary context
>
> surface1 = eglCreateWindowSurface() <-- dri_drawable pointer is malloced
>
> eglMakeCurrent(surface1) <-- ctx->dPriv is set
>
>
> // ... (Get some information about available GL extensions etc)
>
>
> eglDestroySurface(surface1) <-- pointer is freed, ctx->dPriv is now
> dangling
>
> surface2 = eglCreateWindowSurface() <-- Creating a new surface. Sometimes
> it's address will be the same as the free'd pointer.
>
> eglMakeCurrent(surface2) <-- In dri_make_current, ctx->dPriv ==
> driReadPriv may return true because the pointers may be equal
>
>   => The drawable info is not updated. Width and height for the
> drawable is not set from the wl_egl_window on the first frame.
>
>
> Marek: How exactly does it crash? Are you sure firefox didn't previously
> access free'd memory through the dangling pointer and that it was just
> exposed now that the pointer is NULL?
>

ctx was a dangling pointer, which means ctx had been destroyed, and reading
ctx->dPriv crashed.

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

Re: [Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

2018-04-24 Thread Emil Velikov 
On 24 April 2018 at 08:13, Johan Helsing  wrote:
> Emil: Your alternative patch won't work because dri_make_current is not
> necessarily called with NULL after a buffer has been destroyed.
>
Interesting, the trace attached in the bugreport does a proper
makecurrent/surface dance.
Namely, MakeCurrent(..., NULL, NULL, ...) is called before DestroySurface().

Hope you can see how that may be confusing wrt the patch in question.

>
> The problematic sequence is a pattern we use in QtWayland:
>
>
> //create temporary context
>
> surface1 = eglCreateWindowSurface() <-- dri_drawable pointer is malloced
>
> eglMakeCurrent(surface1) <-- ctx->dPriv is set
>
>
> // ... (Get some information about available GL extensions etc)
>
>
> eglDestroySurface(surface1) <-- pointer is freed, ctx->dPriv is now dangling
>
As others have pointed out pointer must not be freed at this point.
We have plenty of refcounting and locking to ensure that, so I'm a bit
suspicious if this happens.

Can you observed that, or it's more of an educated guess?


> surface2 = eglCreateWindowSurface() <-- Creating a new surface. Sometimes
> it's address will be the same as the free'd pointer.
>
Repeating my earlier question  - what do you mean with "it will be the same"?
A malloc call returns the same pointer as previously freed memory, other?

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

Re: [Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

2018-04-24 Thread Michel Dänzer 

Please don't top-post.


On 2018-04-24 10:44 AM, Johan Helsing wrote:
> If the call to dri_destroy_buffer is delayed until the next eglMakeCurrent,
> that would also solve the problem (I'm not sure how that would affect other
> things, though).

Looking at the EGL spec, section 3.5.5 "Destroying Rendering Surfaces"
says about eglDestroySurface:

 All resources associated with surface which were allocated by EGL are
 marked for deletion as soon as possible.

And section 3.7.4 "Context Queries" says:

 If a current surface has been marked for deletion as a result of
 calling eglTerminate or eglDestroySurface, the handle returned by
 eglGetCurrentSurface is not valid, and cannot be passed successfully to
 any other EGL function, [...]

And in appendix E.1 "Updates to EGL 1.4":

 Changes in the revision approved on January 20, 2009:

 • Change object destruction behavior such that object handles become
   invalid immediately after an object is deleted, although the
   underlying object may remain valid if it’s current to a context.

It seems clear from this language that the surface isn't supposed to be
actually destroyed as long as it's current to the context. However, I'm
not sure offhand what needs to be done at which level to achieve that.


> 
> From: Michel Dänzer <mic...@daenzer.net>
> Sent: Tuesday, April 24, 2018 10:36:00 AM
> To: Johan Helsing; Marek Olšák
> Cc: Daniel Stone; pekka.paala...@collabora.co.uk; ML Mesa-dev
> Subject: Re: [Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed 
> dri_drawable
> 
> On 2018-04-24 09:13 AM, Johan Helsing wrote:
>> Emil: Your alternative patch won't work because dri_make_current is not 
>> necessarily called with NULL after a buffer has been destroyed.
>>
>>
>> The problematic sequence is a pattern we use in QtWayland:
>>
>>
>> //create temporary context
>>
>> surface1 = eglCreateWindowSurface() <-- dri_drawable pointer is malloced
>>
>> eglMakeCurrent(surface1) <-- ctx->dPriv is set
>>
>>
>> // ... (Get some information about available GL extensions etc)
>>
>>
>> eglDestroySurface(surface1) <-- pointer is freed, ctx->dPriv is now dangling
> 
> Is this the problem? The memory pointed to by ctx->dPriv shouldn't be
> freed as long as the surface is bound to the current context?
> 
> 
> --
> Earthling Michel Dänzer   |   http://www.amd.com
> Libre software enthusiast | Mesa and X developer
> 
> 
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

2018-04-24 Thread Daniel Stone 
Hi Johan,

On 24 April 2018 at 09:44, Johan Helsing  wrote:
> If the call to dri_destroy_buffer is delayed until the next eglMakeCurrent,
> that would also solve the problem (I'm not sure how that would affect other
> things, though).

It _must_ be:
  If the EGL surface surface is not current to any thread,
eglDestroySurface destroys it immediately. Otherwise, surface is
destroyed when it becomes not current to any thread.

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

Re: [Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

2018-04-24 Thread Johan Helsing 
If the call to dri_destroy_buffer is delayed until the next eglMakeCurrent, 
that would also solve the problem (I'm not sure how that would affect other 
things, though).


As long as dri_make_current doesn't compare against a dangling pointer, I'm 
happy.


Johan


From: Michel Dänzer <mic...@daenzer.net>
Sent: Tuesday, April 24, 2018 10:36:00 AM
To: Johan Helsing; Marek Olšák
Cc: Daniel Stone; pekka.paala...@collabora.co.uk; ML Mesa-dev
Subject: Re: [Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed 
dri_drawable

On 2018-04-24 09:13 AM, Johan Helsing wrote:
> Emil: Your alternative patch won't work because dri_make_current is not 
> necessarily called with NULL after a buffer has been destroyed.
>
>
> The problematic sequence is a pattern we use in QtWayland:
>
>
> //create temporary context
>
> surface1 = eglCreateWindowSurface() <-- dri_drawable pointer is malloced
>
> eglMakeCurrent(surface1) <-- ctx->dPriv is set
>
>
> // ... (Get some information about available GL extensions etc)
>
>
> eglDestroySurface(surface1) <-- pointer is freed, ctx->dPriv is now dangling

Is this the problem? The memory pointed to by ctx->dPriv shouldn't be
freed as long as the surface is bound to the current context?


--
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

2018-04-24 Thread Michel Dänzer 
On 2018-04-24 09:13 AM, Johan Helsing wrote:
> Emil: Your alternative patch won't work because dri_make_current is not 
> necessarily called with NULL after a buffer has been destroyed.
> 
> 
> The problematic sequence is a pattern we use in QtWayland:
> 
> 
> //create temporary context
> 
> surface1 = eglCreateWindowSurface() <-- dri_drawable pointer is malloced
> 
> eglMakeCurrent(surface1) <-- ctx->dPriv is set
> 
> 
> // ... (Get some information about available GL extensions etc)
> 
> 
> eglDestroySurface(surface1) <-- pointer is freed, ctx->dPriv is now dangling

Is this the problem? The memory pointed to by ctx->dPriv shouldn't be
freed as long as the surface is bound to the current context?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

2018-04-24 Thread Johan Helsing 
Emil: Your alternative patch won't work because dri_make_current is not 
necessarily called with NULL after a buffer has been destroyed.


The problematic sequence is a pattern we use in QtWayland:


//create temporary context

surface1 = eglCreateWindowSurface() <-- dri_drawable pointer is malloced

eglMakeCurrent(surface1) <-- ctx->dPriv is set


// ... (Get some information about available GL extensions etc)


eglDestroySurface(surface1) <-- pointer is freed, ctx->dPriv is now dangling

surface2 = eglCreateWindowSurface() <-- Creating a new surface. Sometimes it's 
address will be the same as the free'd pointer.

eglMakeCurrent(surface2) <-- In dri_make_current, ctx->dPriv == driReadPriv may 
return true because the pointers may be equal

  => The drawable info is not updated. Width and height for the drawable is 
not set from the wl_egl_window on the first frame.


Marek: How exactly does it crash? Are you sure firefox didn't previously access 
free'd memory through the dangling pointer and that it was just exposed now 
that the pointer is NULL?


Johan




From: Marek Olšák 
Sent: Tuesday, April 24, 2018 6:08:16 AM
To: Johan Helsing
Cc: ML Mesa-dev; pekka.paala...@collabora.co.uk; Daniel Stone; Charmaine Lee; 
Thomas Hellstrom; Michel Dänzer
Subject: Re: [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

FYI, the commit was causing crashes of qtcreator and firefox, so I reverted it.

Marek

On Fri, Apr 20, 2018 at 6:29 AM, Johan Klokkhammer Helsing 
> wrote:
If an EGLSurface is created, made current and destroyed, and then a second
EGLSurface is created. Then the second malloc in driCreateNewDrawable may
return the same pointer address the first surface's drawable had.
Consequently, when dri_make_current later tries to determine if it should
update the texture_stamp it compares the surface's drawable pointer against
the drawable in the last call to dri_make_current and assumes it's the same
surface (which it isn't).

When texture_stamp is left unset, then dri_st_framebuffer_validate thinks
it has already called update_drawable_info for that drawable, leaving it
unvalidated and this is when bad things starts to happen. In my case it
manifested itself by the width and height of the surface being unset.

This is fixed this by setting the pointer to NULL before freeing the
surface.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106126
Signed-off-by: Johan Klokkhammer Helsing 
>
---
 src/gallium/state_trackers/dri/dri_drawable.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/gallium/state_trackers/dri/dri_drawable.c 
b/src/gallium/state_trackers/dri/dri_drawable.c
index e5a7537e47..02328acd98 100644
--- a/src/gallium/state_trackers/dri/dri_drawable.c
+++ b/src/gallium/state_trackers/dri/dri_drawable.c
@@ -185,6 +185,7 @@ fail:
 void
 dri_destroy_buffer(__DRIdrawable * dPriv)
 {
+   struct dri_context *ctx = dri_context(dPriv->driContextPriv);
struct dri_drawable *drawable = dri_drawable(dPriv);
struct dri_screen *screen = drawable->screen;
struct st_api *stapi = screen->st_api;
@@ -202,6 +203,9 @@ dri_destroy_buffer(__DRIdrawable * dPriv)
/* Notify the st manager that this drawable is no longer valid */
stapi->destroy_drawable(stapi, >base);

+   if (ctx && ctx->dPriv == dPriv)
+  ctx->dPriv = NULL;
+
FREE(drawable);
 }

--
2.17.0


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

Re: [Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

2018-04-23 Thread Marek Olšák 
FYI, the commit was causing crashes of qtcreator and firefox, so I reverted
it.

Marek

On Fri, Apr 20, 2018 at 6:29 AM, Johan Klokkhammer Helsing <
johan.hels...@qt.io> wrote:

> If an EGLSurface is created, made current and destroyed, and then a second
> EGLSurface is created. Then the second malloc in driCreateNewDrawable may
> return the same pointer address the first surface's drawable had.
> Consequently, when dri_make_current later tries to determine if it should
> update the texture_stamp it compares the surface's drawable pointer against
> the drawable in the last call to dri_make_current and assumes it's the same
> surface (which it isn't).
>
> When texture_stamp is left unset, then dri_st_framebuffer_validate thinks
> it has already called update_drawable_info for that drawable, leaving it
> unvalidated and this is when bad things starts to happen. In my case it
> manifested itself by the width and height of the surface being unset.
>
> This is fixed this by setting the pointer to NULL before freeing the
> surface.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106126
> Signed-off-by: Johan Klokkhammer Helsing 
> ---
>  src/gallium/state_trackers/dri/dri_drawable.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/gallium/state_trackers/dri/dri_drawable.c
> b/src/gallium/state_trackers/dri/dri_drawable.c
> index e5a7537e47..02328acd98 100644
> --- a/src/gallium/state_trackers/dri/dri_drawable.c
> +++ b/src/gallium/state_trackers/dri/dri_drawable.c
> @@ -185,6 +185,7 @@ fail:
>  void
>  dri_destroy_buffer(__DRIdrawable * dPriv)
>  {
> +   struct dri_context *ctx = dri_context(dPriv->driContextPriv);
> struct dri_drawable *drawable = dri_drawable(dPriv);
> struct dri_screen *screen = drawable->screen;
> struct st_api *stapi = screen->st_api;
> @@ -202,6 +203,9 @@ dri_destroy_buffer(__DRIdrawable * dPriv)
> /* Notify the st manager that this drawable is no longer valid */
> stapi->destroy_drawable(stapi, >base);
>
> +   if (ctx && ctx->dPriv == dPriv)
> +  ctx->dPriv = NULL;
> +
> FREE(drawable);
>  }
>
> --
> 2.17.0
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

2018-04-23 Thread Emil Velikov 
Hi Johan,

On 20 April 2018 at 11:29, Johan Klokkhammer Helsing
 wrote:
> If an EGLSurface is created, made current and destroyed, and then a second
> EGLSurface is created. Then the second malloc in driCreateNewDrawable may
> return the same pointer address the first surface's drawable had.
What do you mean with "second malloc in driCreateNewDrawable"? There's
only a single malloc in the function.

Are you saying the C runtime returns identical pointers when creating
the first and second surface?

> Consequently, when dri_make_current later tries to determine if it should
> update the texture_stamp it compares the surface's drawable pointer against
> the drawable in the last call to dri_make_current and assumes it's the same
> surface (which it isn't).
>
> When texture_stamp is left unset, then dri_st_framebuffer_validate thinks
> it has already called update_drawable_info for that drawable, leaving it
> unvalidated and this is when bad things starts to happen. In my case it
> manifested itself by the width and height of the surface being unset.
>
> This is fixed this by setting the pointer to NULL before freeing the
> surface.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106126
> Signed-off-by: Johan Klokkhammer Helsing 
> ---
>  src/gallium/state_trackers/dri/dri_drawable.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/gallium/state_trackers/dri/dri_drawable.c 
> b/src/gallium/state_trackers/dri/dri_drawable.c
> index e5a7537e47..02328acd98 100644
> --- a/src/gallium/state_trackers/dri/dri_drawable.c
> +++ b/src/gallium/state_trackers/dri/dri_drawable.c
> @@ -185,6 +185,7 @@ fail:
>  void
>  dri_destroy_buffer(__DRIdrawable * dPriv)
>  {
> +   struct dri_context *ctx = dri_context(dPriv->driContextPriv);
> struct dri_drawable *drawable = dri_drawable(dPriv);
> struct dri_screen *screen = drawable->screen;
> struct st_api *stapi = screen->st_api;
> @@ -202,6 +203,9 @@ dri_destroy_buffer(__DRIdrawable * dPriv)
> /* Notify the st manager that this drawable is no longer valid */
> stapi->destroy_drawable(stapi, >base);
>
> +   if (ctx && ctx->dPriv == dPriv)
> +  ctx->dPriv = NULL;
> +
Context references in here feels wrong.

Are you sure we shouldn't be removing the context/drawable link in
dri_make_current?
After all it seems to be the one that adds it in the first place.

Something like the following:

--- a/src/gallium/state_trackers/dri/dri_context.c
+++ b/src/gallium/state_trackers/dri/dri_context.c
@@ -278,9 +278,16 @@ dri_make_current(__DRIcontext * cPriv,

++ctx->bind_count;

-   if (!draw && !read)
-  return ctx->stapi->make_current(ctx->stapi, ctx->st, NULL, NULL);
-   else if (!draw || !read)
+   if (!draw && !read) {
+  if (!ctx->stapi->make_current(ctx->stapi, ctx->st, NULL, NULL))
+ return GL_FALSE;
+
+  ctx->dPriv = NULL;
+  ctx->rPriv = NULL;
+  return GL_TRUE;
+   }
+
+   if (!draw || !read)
   return GL_FALSE;

if (ctx->dPriv != driDrawPriv) {

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

Re: [Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

2018-04-23 Thread Marek Olšák 
Pushed, thanks!

Marek

On Fri, Apr 20, 2018 at 6:29 AM, Johan Klokkhammer Helsing <
johan.hels...@qt.io> wrote:

> If an EGLSurface is created, made current and destroyed, and then a second
> EGLSurface is created. Then the second malloc in driCreateNewDrawable may
> return the same pointer address the first surface's drawable had.
> Consequently, when dri_make_current later tries to determine if it should
> update the texture_stamp it compares the surface's drawable pointer against
> the drawable in the last call to dri_make_current and assumes it's the same
> surface (which it isn't).
>
> When texture_stamp is left unset, then dri_st_framebuffer_validate thinks
> it has already called update_drawable_info for that drawable, leaving it
> unvalidated and this is when bad things starts to happen. In my case it
> manifested itself by the width and height of the surface being unset.
>
> This is fixed this by setting the pointer to NULL before freeing the
> surface.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106126
> Signed-off-by: Johan Klokkhammer Helsing 
> ---
>  src/gallium/state_trackers/dri/dri_drawable.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/gallium/state_trackers/dri/dri_drawable.c
> b/src/gallium/state_trackers/dri/dri_drawable.c
> index e5a7537e47..02328acd98 100644
> --- a/src/gallium/state_trackers/dri/dri_drawable.c
> +++ b/src/gallium/state_trackers/dri/dri_drawable.c
> @@ -185,6 +185,7 @@ fail:
>  void
>  dri_destroy_buffer(__DRIdrawable * dPriv)
>  {
> +   struct dri_context *ctx = dri_context(dPriv->driContextPriv);
> struct dri_drawable *drawable = dri_drawable(dPriv);
> struct dri_screen *screen = drawable->screen;
> struct st_api *stapi = screen->st_api;
> @@ -202,6 +203,9 @@ dri_destroy_buffer(__DRIdrawable * dPriv)
> /* Notify the st manager that this drawable is no longer valid */
> stapi->destroy_drawable(stapi, >base);
>
> +   if (ctx && ctx->dPriv == dPriv)
> +  ctx->dPriv = NULL;
> +
> FREE(drawable);
>  }
>
> --
> 2.17.0
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

2018-04-20 Thread Johan Klokkhammer Helsing 
If an EGLSurface is created, made current and destroyed, and then a second
EGLSurface is created. Then the second malloc in driCreateNewDrawable may
return the same pointer address the first surface's drawable had.
Consequently, when dri_make_current later tries to determine if it should
update the texture_stamp it compares the surface's drawable pointer against
the drawable in the last call to dri_make_current and assumes it's the same
surface (which it isn't).

When texture_stamp is left unset, then dri_st_framebuffer_validate thinks
it has already called update_drawable_info for that drawable, leaving it
unvalidated and this is when bad things starts to happen. In my case it
manifested itself by the width and height of the surface being unset.

This is fixed this by setting the pointer to NULL before freeing the
surface.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106126
Signed-off-by: Johan Klokkhammer Helsing 
---
 src/gallium/state_trackers/dri/dri_drawable.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/gallium/state_trackers/dri/dri_drawable.c 
b/src/gallium/state_trackers/dri/dri_drawable.c
index e5a7537e47..02328acd98 100644
--- a/src/gallium/state_trackers/dri/dri_drawable.c
+++ b/src/gallium/state_trackers/dri/dri_drawable.c
@@ -185,6 +185,7 @@ fail:
 void
 dri_destroy_buffer(__DRIdrawable * dPriv)
 {
+   struct dri_context *ctx = dri_context(dPriv->driContextPriv);
struct dri_drawable *drawable = dri_drawable(dPriv);
struct dri_screen *screen = drawable->screen;
struct st_api *stapi = screen->st_api;
@@ -202,6 +203,9 @@ dri_destroy_buffer(__DRIdrawable * dPriv)
/* Notify the st manager that this drawable is no longer valid */
stapi->destroy_drawable(stapi, >base);
 
+   if (ctx && ctx->dPriv == dPriv)
+  ctx->dPriv = NULL;
+
FREE(drawable);
 }
 
-- 
2.17.0

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